improve report filtering related to system forms #31654
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was only filtering on a selection of system forms and is also unnecessary since there are already other filters that won't match system forms.
snopoke
added
awaiting QA
snopoke
requested review from
esoergel,
proteusvacuum,
joxl and
orangejenny
as code owners
snopoke
changed the title
snopoke
changed the title
orangejenny
approved these changes
May 23, 2022
esoergel
reviewed
May 30, 2022
| assert re.match(r'\w+$', name), \ | ||
| "Names must be valid python variable names, was {}".format(name) | ||
| self.name = name | ||
| self.body = { | ||
| "field": field, | ||
| "size": size if size is not None else SIZE_LIMIT, | ||
| } | ||
| if missing: | ||
| self.body["missing"] = missing |
There was a problem hiding this comment.
That's neat - I didn't know this was part of the query language: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html#_missing_value_5
|
|
||
| # filter results by app and xmlns if applicable | ||
| if FormsByApplicationFilter.has_selections(self.request): | ||
| form_values = list(self.all_relevant_forms.values()) | ||
| if form_values: | ||
| query = query.OR(*[self._form_filter(f) for f in form_values]) | ||
| else: |
There was a problem hiding this comment.
aside: FormsByApplicationFilter.has_selections counts "Show Advanced Options" as a selection, which is a little unintuitive in my opinion.
There was a problem hiding this comment.
I agree but if you to select it and not change anything else then it does change the results to only include 'known forms'. I toyed with changing but the added complexity didn't seem worth it in the context of this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Product Description
Changes to the way we filter forms by application and user.
User filter changes
Change made to allow 'system' forms to be displayed which don't usually have a user associated with them:
App filter changes
Dropdown options for 'unknown forms'
app_idfieldapp_idfieldWhen the default options are selected ("Show Forms in all Applications")
Unknown user system form filter
Spec: https://docs.google.com/document/d/1t97k35y9juT7Wl_suOiWi-NYP3cpiaV4hUmZ0j2e-e4/edit#heading=h.lqyt6iorkjwv
Jira: https://dimagi-dev.atlassian.net/browse/USH-1844
Technical Summary
The biggest change was to make the app / xml ES aggregation include forms with no
app_id: 54c51aaA few other filter changes.
Safety Assurance
Safety story
I consider this quite safe. It does alter a system report but only minor logic changes have been done.
Automated test coverage
None
QA Plan
Will be QA'd on staging.
Rollback instructions
Labels & Review