Skip to content
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

Update Task model validation to prevent status being NULL #72

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

kjsanger
Copy link
Member

Update Task model validation to prevent status being NULL

Change the eq method of Task so that it doesn't raise ValidationError when the "other" object is a dict which isn't a valid task. It should simply return that they are not equal.

Simplified the eq method a bit by removing redundant "else"s.

Some expected test results needed changing to accommodate the new Task status.

Change the eq method of Task so that it doesn't raise ValidationError
when the "other" object is a dict which isn't a valid task. It should
simply return that they are not equal.

Simplified the eq method a bit by removing redundant "else"s.

Some expected test results needed changing to accommodate the new Task
status.
@kjsanger kjsanger requested review from mgcam and nerdstrike July 11, 2024 13:26
Avoid setting a default value for status to ensure that it must be set
explicitly. The create_task method in the database layer now has
exclusive control over whether a default (of PENDING) is set, which it
does only when a new task record is created, not when an exisiting
task record is updated.
@kjsanger
Copy link
Member Author

I should add that the reason I'm suggesting a change is that by sending a POST with a status accidentally missing, I managed to set all the tasks to a NULL status in the database and then consequently couldn't find them.

Copy link
Contributor

@nerdstrike nerdstrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice bug! This is an improvement.

Copy link
Member

@mgcam mgcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawback of this solution - we'll have to set the task status explicitly when we create a new task. We might improve later, will do for now to prevent accidently erasing the status in the database record.

@mgcam mgcam merged commit 7995e13 into wtsi-npg:devel Jul 12, 2024
1 check passed
mgcam added a commit to mgcam/npg_porch_cli that referenced this pull request Jul 17, 2024
Following wtsi-npg/npg_porch#72,
Status code 422 "Unprocessable Entity" error is received
from the server if the status is not set.
@kjsanger kjsanger deleted the model-avoid-null-status branch July 17, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants