Feature/problem status#108
Conversation
| from problem.models import Problem, Sentence | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
I've lifted these fixtures here so they are available to more test files.
| expect(emitted).toEqual([{ id: 123, hidden: true }]); | ||
| }); | ||
|
|
||
| it('should show the alert banner only when the problem is hidden', () => { |
There was a problem hiding this comment.
The alert banner is no longer part of the VisibilityToggleComponent proper. It's now part of the ProblemDetails component. No functional changes.
There was a problem hiding this comment.
I like that you also moved the test there. 👍
| expect(emitted).toEqual([{ id: 123, hidden: true }]); | ||
| }); | ||
|
|
||
| it('should show the alert banner only when the problem is hidden', () => { |
There was a problem hiding this comment.
I like that you also moved the test there. 👍
| @keyframes growIn { | ||
| from { | ||
| opacity: 0; | ||
| max-height: 0; | ||
| padding-top: 0; | ||
| padding-bottom: 0; | ||
| margin-bottom: 0; | ||
| overflow: hidden; | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| max-height: 200px; | ||
| overflow: hidden; | ||
| } | ||
| } | ||
|
|
||
| .alert.alert-info { | ||
| animation: growIn 0.3s ease-out; | ||
| } |
There was a problem hiding this comment.
This looks very familiar. Didn't you add a similar piece of SCSS code in another recent PR?
| gold: false, | ||
| status: ProblemStatus.BRONZE |
There was a problem hiding this comment.
This is not a big deal and also not a change request, but it's worth mentioning that you can keep the diff smaller by always including a trailing comma. That also simplifies editing in cases where you want to change the order of lines.
| if not isinstance(gold, bool): | ||
| return Response( | ||
| {"detail": "'gold' must be a boolean."}, | ||
| status=HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
Shouldn't this validation come with DRF out of the box?
| entity1="dog", | ||
| entity2="canine", | ||
| relationship=KnowledgeBaseAnnotation.Relationship.EQUAL, |
There was a problem hiding this comment.
Zoology nitpick: dogs are a subset of canines.
| assert serializer.data["status"] == Problem.Status.BRONZE | ||
| assert serializer.data["gold"] is False |
There was a problem hiding this comment.
Why go with == on one line and is on the next?
| </div> | ||
| @if (currentUser?.canChangeProblemStatus) { | ||
| <la-gold-toggle [problem]="problem()" /> | ||
| } @if (currentUser?.canChangeProblemVisibility) { |
There was a problem hiding this comment.
If I understand correctly, these are two independent @ifs. So maybe distance the second conditional from the closing brace of the first.
| } @if (currentUser?.canChangeProblemVisibility) { | |
| } | |
| @if (currentUser?.canChangeProblemVisibility) { |
| @let canChangeVisibility = canChangeVisibility$ | ||
| | async; |
There was a problem hiding this comment.
I think in this case, the added newline makes the code less readable.
2 participants
(Depends on #106, which should be merged first.)
This PR introduces two new fields/properties on the Problem model:
Problem.gold(DB field) indicates whether the problem has been marked with 'gold' status by a master annotator.Problem.status(computed) indicates the current status of a problem, which determines which users are able to see it, cf. the overview in README.md.Only Master Annotators are able to toggle 'gold' status on a problem. I've added a modal that provides information about the various statuses and their effects. This is shown when the circle-info icon (see screenshot below).