Feature/problem labels#64
Conversation
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
I have read the code but did not try out the features locally.
I like the comprehensive tests and documentation and the clear evidence that you aimed to work with standalone functions where possible. I have a few comments, but it all looks very reasonable to me.
| attached_at = models.DateTimeField(auto_now_add=True) | ||
| attached_by = models.ForeignKey( | ||
| settings.AUTH_USER_MODEL, | ||
| on_delete=models.CASCADE, | ||
| related_name="labelings_attached", | ||
| ) | ||
|
|
||
| removed_at = models.DateTimeField( | ||
| null=True, | ||
| blank=True, | ||
| help_text="When this label was removed from the problem.", | ||
| ) | ||
| removed_by = models.ForeignKey( | ||
| settings.AUTH_USER_MODEL, | ||
| on_delete=models.CASCADE, | ||
| related_name="labelings_removed", | ||
| null=True, | ||
| blank=True, | ||
| help_text="User who removed this label.", | ||
| ) |
There was a problem hiding this comment.
Not a request for change, just a remark: another way to organize this would be to create a separate Event model for changes made by users. It would have a type (such as "deleted"), a link to the affected model, a user and a date/time. Maybe not worth worrying about for this project, but when there many models that can be edited, this could help to keep the models leaner.
There was a problem hiding this comment.
Sounds very scalable! 👍
| public loadingLabels$ = this.availableLabels$.pipe( | ||
| map(() => false), | ||
| startWith(true) | ||
| ); |
There was a problem hiding this comment.
This looks a bit hacky, though I don't have a ready suggestion for a different approach. Which other options did you consider, if any?
There was a problem hiding this comment.
The easiest solution here is to use the tap() operator and set a loading state variable to false once a request comes in. This is the imperative approach, relying on an unnamed side effect to update loading state. With more complex code it becomes difficult to keep track of what changes this loading boolean. In general I try to avoid tap() outside of debug logging.
Another solution is to wrap both the data and the loading state in a new object and to emit two instances of that object: one with { loading: true, data: null } followed by one with { loading: false, data: ... }. This is more declarative but more verbose and complicated. We have adopted this solution in the Lettercraft SearchService
The version in this file strikes a good position in the middle: it is declarative/reactive (which I prefer) yet does not suffer from the complexity of a switchMap into a concat(of(...), ...) as seen in Lettercraft.
There was a problem hiding this comment.
Other question: does the state of label loading need to be a separate observable? Couldn't you just use @let loadingLabels = (selectedLabels$ | async == null)?
There was a problem hiding this comment.
You mean availableLabels$? With the new @let syntax I guess you could. I'm not sure whether you'd need to add a startWith(null) so the variable actually exists and has a null value before the first (real) data emission. (My database/branches are a bit messy right now so testing it would take quite some time.)
I don't have a real preference for either solution.
There was a problem hiding this comment.
Er yes, availableLabels$. undefined == null is true, so I don't think you would need to startWith(null). The only way this could go wrong, is if the initializer of availableLabels$ only runs after the component is first rendered, but that is probably not the case since otherwise you also couldn't access loadingLabels$.
2 participants
Closes #60
This is one key aspect of annotations accounted for. We may also be able to reuse this system to mark problems as 'gold' or 'hidden', although I still think these properties are best kept independent.
The fixtures in
backend/annotation/fixtures/initial.jsonare for testing convenience only, so you don't have to add labels yourself every time you flush your local dev DB. They are not intended to be loaded in on the server.