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

Filter by Statuses #28

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Filter by Statuses #28

merged 6 commits into from
Jan 15, 2025

Conversation

SandyRogers
Copy link
Member

This PR:

  • adds a new model manager mixin providing convenience queryset methods to filter object querysets by combinations of keys in their status fields.
    • this can work for any JSON Field on a model that contains keys pointing to bool values, but typically that is a status field like analysis.status == {"analysis_started": True, "analysis_failed": False}.
    • Usage is like study.analyses.filter_by_statuses([analysis.AnalysisStates.ANALYSIS_STARTED, "some_custom_key"]).exclude_by_statuses([analysis.AnalysisStates.ANALYSIED_BLOCKED])
  • fixes a bug where filtering by Enums requires explicitly calling .value – this seems to be a change when moving to Python 3.12.

TODO in future:

  • It would make sense to also move the status field definitions to the same base model file as this manager, including helpers like the mark_status_as methods, and perhaps a utility for handling default state setting.
  • Use this on Assembly too.

Copy link
Contributor

@MGS-sails MGS-sails left a comment

Choose a reason for hiding this comment

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

thanks Sandy.
This seems like it's going to enhance code reusability. Happy to approve. But I have the following thoughts for future considerations:
I was wondering why we need to check for possible null values for the status fields.
Would be possible for us to enforce a rule that status fields in our DB should not be nullable by design ? or perhaps we could even try to make them more uniform. That way we would have less conditions to check in this Mixin Manager. Or is it that perhaps the structure of the fields is not always in our control.

@SandyRogers
Copy link
Member Author

thanks Sandy. This seems like it's going to enhance code reusability. Happy to approve. But I have the following thoughts for future considerations: I was wondering why we need to check for possible null values for the status fields. Would be possible for us to enforce a rule that status fields in our DB should not be nullable by design ? or perhaps we could even try to make them more uniform. That way we would have less conditions to check in this Mixin Manager. Or is it that perhaps the structure of the fields is not always in our control.

Thanks @MGS-sails !
Good challenge - the main reason for the options for null-handling at query time, is to cater for the possibility than more keys are added/changed at later dates. In theory we could do data migrations for all existing objects to update status fields with the "current" key sets... but I reckoned from experience that we will sometimes have a reason not to / be unaware. This way it is up to each query to decide how those missing ones are handled, rather than up to a migrator to think of all the possible places a new default might matter.

Copy link
Contributor

@KateSakharova KateSakharova left a comment

Choose a reason for hiding this comment

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

Rewrote my PR based on that. Thank you!

@KateSakharova KateSakharova merged commit 8014248 into main Jan 15, 2025
2 checks passed
@KateSakharova KateSakharova deleted the analysis-state-filtering branch January 15, 2025 16:49
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