-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#336] Archiefactiedatum validation [Backend] #337
[#336] Archiefactiedatum validation [Backend] #337
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 77.44% 77.60% +0.16%
==========================================
Files 223 223
Lines 7058 7109 +51
Branches 133 133
==========================================
+ Hits 5466 5517 +51
Misses 1468 1468
Partials 124 124 ☔ View full report in Codecov by Sentry. |
33f11a8
to
aa27bd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, if answer is no: approved anyway
@@ -249,6 +250,11 @@ def update(self, request, *args, **kwargs): | |||
return super().update(request, *args, **kwargs) | |||
|
|||
def perform_destroy(self, instance: DestructionList) -> None: | |||
if not instance.all_items_can_be_deleted(): | |||
raise ValidationError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a ZaakArchiefactiedatumInFuture
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I raise here the ZaakArchiefactiedatumInFuture
then it will give a 500 error response, but if I raise the ValidationError
then it will respond with a 400 status to the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Partly fixes #336