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.

Sign up for GitHub

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

Jump to bottom

Add backend logic for column filtering #35846

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Add backend logic for column filtering #35846

wants to merge 19 commits into from
+902 −24

Conversation

biyeun
Copy link
Member

@biyeun biyeun commented Feb 25, 2025

Technical Summary

This PR implements the backend logic for filtering a CaseSearchES query based on the BulkEditColumnFilters associated with a BulkEditSession.

Review commit-by-commit.

  • I tried my best to reorder commits in the most narrative order, making each commit small and bite-sized
  • please don't be intimidated by number of commits or number of lines changed (there are a lot of tests that contribute to the bulk of this PR's size)

While working on this I discovered a few issues with our current model definitions:

  • xpath_query does not support an "ends with" so I replaced it with not(starts-with(...))
  • for improved clarity, I added separate lte (<=) and gte (>=) match types
  • xpath_query does not support <, >, <=, and >= for text, only dates and numbers
  • xpath_query has some limitations due to our xpath parser if the value contains mixed quotes (" and '), so I made sure to check for this.

I believe we can and should move away from xpath_query when we implement forms, as there is no equivalent filter available for FormES. I opted to use xpath_query, even with its limitations, because it was the fastest way forward to a usable feature for most scenarios if we're just dealing with case data.

Additionally, I added utilities to BulkEditSession to:

  • add column filters
  • reorder column filters
  • and retrieve a filtered query set

We then use the filtered query set from the session in CleanCasesTableView.

Feature Flag

DATA_CLEANING_CASES

Safety Assurance

Safety story

safe. everything here is feature flagged and not part of production workflows

Automated test coverage

Yes, and lots of new tests added.

QA Plan

Not needed at this point

Migrations

  • The migrations in this code can be safely applied first independently of the code

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

biyeun added 15 commits February 25, 2025 21:02
@biyeun
CaseSearchES xpath_query does not support ends-with, only starts-with
23746c5
@biyeun
empty vs null is not supported in CaseSearchES
fc65bc9 
instead, CaseSearchES supports the concept of empty (both '' and unset values)
and missing (unset values only). Update NULL, NOT_NULL to be missing, not missing
@biyeun
make separate lte, gte match type choices
2f96aa1 
this will make determining any related operators more straightforward
additionally, CaseSearchES xpath_query does not support <, >, <=, >= comparitors for text...only date and number
@biyeun
move empty, missing match types to own category applying to all data …
3986d67 
…types
@biyeun
sort DataTypes into filter categories so that correct filter choices …
a58d11d 
…are displayed for a given data type
@biyeun
add validation utility (for forms) for data_type match_type combinations
a60b119 
(with tests)
@biyeun
add filter_query utility to BulkEditColumnFilters
d4ff670
@biyeun
add utility for retrieving xpath_expression for BulkEditColumnFilter
cb26c25 
applies to match_types that are not in FilterMatchTypes.ALL_DATA_TYPES_CHOICES
@biyeun
add utility to session for adding column filters
df53fe4 
with tests
@biyeun
add unique ids to reorderable BulkEdit Columns and Filters
7073f98 
this makes reordering easier (as we just need to pass in reordered list of UUIDs to update the index)
Additionally, it is more secure than just passing the pk, as that shows how many BulkEditColumns are available in the db.
@biyeun
add a migration for all of the most recent model updates
c2d86c2 
- column_id to BulkEditColumn
- filter_id to BulkEditColumnFilter
- updates to FilterMatchType choices
@biyeun
add utility to session for reordering column filters
f96a28d 
with tests
@biyeun
add utility for getting the queryset from the session
a6e45ff 
(with tests)
@biyeun
update table view to use session.get_queryset()
7b28932
@biyeun
replacing with more fake email (whoops)
a02022a
@biyeun biyeun added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Feb 25, 2025
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Feb 25, 2025
nospame
nospame reviewed Feb 26, 2025
View reviewed changes
Copy link
Contributor

@nospame nospame left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't feel like I have a full understanding of how this will be used, but the code all looks good to me. Couple nitpicks, with the note on UnsupportedFilterValueException as the only one I'd for sure like to see fixed before approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nospame nospame nospame left review comments

@mjriley mjriley Awaiting requested review from mjriley

@orangejenny orangejenny Awaiting requested review from orangejenny

@jingcheng16 jingcheng16 Awaiting requested review from jingcheng16

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@biyeun @nospame @dimagimon