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

User Visit Review Page: Filters and Sortable fields #482

Open
wants to merge 3 commits into
base: pkv/pm-agree-count-fixes
Choose a base branch
from

Conversation

pxwxnvermx
Copy link
Contributor

Product Description

This PR adds filters and sortable fields for the User Visit Review page.

image

Technical Summary

Ticket Link

Safety Assurance

Safety story

The features included in this PR are deployed and tested on staging.

Automated test coverage

None

QA Plan

None

Labels & Review

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

@pxwxnvermx pxwxnvermx self-assigned this Jan 28, 2025
@pxwxnvermx pxwxnvermx changed the base branch from main to pkv/pm-agree-count-fixes January 28, 2025 07:58
@pxwxnvermx pxwxnvermx marked this pull request as ready for review January 28, 2025 08:51
Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

Generally looks good, but a few questions about behavior

)
)
for field_name in self.form.fields.keys():
self.form.fields[field_name].widget.attrs.update({"@change": "$refs.reviewFilterForm.submit()"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this submit the form or just refresh it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this submits the review filter form whenever any filter in that form is changed.

@@ -58,15 +59,26 @@ def show_warning(record):

class UserVisitReviewFilter(FilterSet):
review_status = ChoiceFilter(choices=VisitReviewStatus.choices, empty_label="All Reviews")
user = ModelChoiceFilter(queryset=User.objects.none(), empty_label="All Users", to_field_name="username")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Im pretty confused by this user filter queryset behavior. Can you explain a bit more what is occurring? It is set to empty, then overwritten on init, but where do the values in the queryset for id__in come from, and why does it need to be done in such a way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ModelChoiceFilter filter requires a query set of objects to be instantiated with, its a filter wrapper for ModelChoiceField, so I have used a .none() query set object to instantiate that filter as none query set don't make any queries to database. Further the self.queryset used in the id__in is the query set for UserVisits that this UserVisitReviewFilter works on and is passed into the filter when it is instantiated in the user_visit_review view. I hope this sounds good and please suggest any improvements you have to improve this.

Copy link
Member

@sravfeyn sravfeyn left a comment

Choose a reason for hiding this comment

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

LGTM


def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.filters["user"].queryset = User.objects.filter(id__in=self.queryset.values_list("user_id", flat=True))
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to pass any org/program filtering on this queryset?

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