Community Library Notifications Filtering #5566
Conversation
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks, Eddie! Looking good! Just one major concern now that we are treating this date_updated field, we need to use the auto_now arg.
| indexes = [ | ||
| # Useful for cursor pagination | ||
| models.Index(fields=["-date_created"], name="submission_date_created_idx"), | ||
| models.Index(fields=["-date_updated"], name="submission_date_updated_idx"), | ||
| ] |
There was a problem hiding this comment.
Since we are already specifying this index in the field definition, I think we can remove this one here.
| import django_filters | ||
| from django.utils import timezone | ||
| from django_filters import DateTimeFilter | ||
| from django_filters import MultipleChoiceFilter | ||
| from django_filters.rest_framework import DjangoFilterBackend | ||
| from rest_framework.decorators import action | ||
| from rest_framework.filters import SearchFilter |
There was a problem hiding this comment.
Could we import these (and the FilterSet class) from django_filters.rest_framework instead? Since these aaddmore context for DRF
|
|
||
| date_updated__lte = DateTimeFilter(field_name="date_updated", lookup_expr="lte") | ||
| date_updated__gte = DateTimeFilter(field_name="date_updated", lookup_expr="gte") | ||
| status__in = MultipleChoiceFilter( |
There was a problem hiding this comment.
(Apologies, I didn't note this 😅) But could we create a ChoiceInFilter class for this instead? Something like this
This way ⬆️ we can pass these statuses in a single query param like status="STATUS_A,STATUS_B" instead of using multiple query params like status="STATUS_A"&status="STATUS_B", which is not the most idiomatic way to handle in the frontend.
| categories = models.JSONField(blank=True, null=True) | ||
| date_created = models.DateTimeField(auto_now_add=True) | ||
| date_resolved = models.DateTimeField(blank=True, null=True) | ||
| date_updated = models.DateTimeField(blank=True, null=True, db_index=True) |
There was a problem hiding this comment.
Here, we also need an auto_now=True argument so that this field gets updated automatically every time an instance is updated
| date_created=datetime.datetime(2023, 1, 1, tzinfo=pytz.utc), | ||
| ) | ||
| self.old_pending_submission.date_updated = datetime.datetime( | ||
| 2023, 1, 1, tzinfo=pytz.utc |
There was a problem hiding this comment.
With the update on the date_updated field, we will need to mock the django.utils.timezone.now function to set this date_updated.
|
|
||
|
|
||
| def get_resolved_by_name(item): | ||
| if item.get("resolved_by__first_name") and item.get("resolved_by__last_name"): |
There was a problem hiding this comment.
I think we can skip this validation, just in case we have just the name or last name of a user.
There was a problem hiding this comment.
I updated the function, now this function will return:
- If both names exist: returns "First Last"
- If only first name exists: returns "First"
- If only last name exists: returns "Last"
- If neither exists: returns None
| date_updated = timezone.now() | ||
| submission = serializer.save( | ||
| date_resolved=date_resolved, | ||
| date_updated=date_updated, |
There was a problem hiding this comment.
With the changes on the auto_now we now may not even need to set this date here :D
| "resolved_by_id", | ||
| "resolved_by__first_name", | ||
| "resolved_by__last_name", | ||
| ) |
There was a problem hiding this comment.
Oh, something I did not see when planning this issue, we'll need to return the channel__name here too to get the channel name directly. And add this to the field_map to return this field without the double underscore. Thanks!
|
Thanks @taoerman! It seems like there are some failing tests on the last commit. Could you take a look at it, please? Thanks! |
…t-to-support-notifications-filtering merge.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
This looks good, thanks @taoerman! Just one last thing: Could we consolidate the migrations 0159 and 0160 into just one migration? They are both related, and the reason why they are separated is because of the code review, so logically as the result of this PR, there should only be one. Thanks!
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks a lot @taoerman! LGTM!
2 participants
Summary
Renames date_resolved to Test Filtering And Search with database indexes for performance, adds FilterSet with date range and status filters plus channel name search, and makes resolved_by visible to editors.
References
Fixed #5456
Reviewer guidance
Run unit tests.