Uh oh!
There was an error while loading. Please reload this page.
CommunityLibrarySubmission model - #5156
Conversation
AlexVelezLl
left a comment
There was a problem hiding this comment.
This is looking great @Jakoma02! Thanks! Just a couple of nitpick comments, but this is looking great! :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| method is NOT called automatically on every save. | ||
| """ | ||
| super().clean() | ||
| if not self.channel.editors.filter(pk=self.author.pk).exists(): |
There was a problem hiding this comment.
We usually do these validations at api level, but can imagine the benefits of doing this at model level. This doesn't seem to be a common pattern in our codebase thought, would like to hear @rtibbles thoughts, as we may also need to do the check of the version being version > 0 and version <= channel.version
There was a problem hiding this comment.
When we have done these kinds of validations on the model itself, we have usually done it in the save method rather than the clean method - for precisely the reasons noted in the docstring - clean is not called on every save, but only when using a Django model form (which we rarely use).
If we want to ensure that this is being used every time we save a model, we should put it in the save method instead, otherwise, I think it would sit better in the API endpoint.
There was a problem hiding this comment.
Okay, if you agree, I will move the logic to the save method.
There was a problem hiding this comment.
Yes, lets move this logic to the save method, along with the version check. Thanks!
There was a problem hiding this comment.
Thank youu! <3 Just one little thing before we merge (I promise this is the last one 😅):
Could you remove the translate function from the error messages? We use to return these validation error messages without translating them e.g. here.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AlexVelezLl
left a comment
There was a problem hiding this comment.
Code changes looks good and tests are passing, the first lines of code of this project are ready to go! Thanks a lot @Jakoma02! Merging!
Summary
I implemented CommunityLibrarySubmission model and tests for it according to specs in #5145.
I have tested the changes by running
pytesttests and making sure that the new tests pass and the existing tests do not break, and I have made sure that the devserver still starts.References
Resolves#5145.
Reviewer guidance
I have decided to override the
cleanmethod to validate that the author of the submission should be an editor of the corresponding channel. This cannot be achieved using a database constraint because traversing related fields is not supported inside them. This way, the constraint will be checked when using forms and when explicitly callingfull_clean()(not on every save). Please check that this solution matches your expectations.