-
Notifications
You must be signed in to change notification settings - Fork 9
Fix Active Dref Operations List n+1 query issue (active-dref) #2759
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -252,24 +252,41 @@ class Meta: | |
|
|
||
| @extend_schema_field(MiniOperationalUpdateActiveSerializer(many=True)) | ||
| def get_operational_update_details(self, obj): | ||
| queryset = DrefOperationalUpdate.objects.filter(dref_id=obj.id).order_by("-created_at") | ||
| prefetched = getattr(obj, "prefetched_operational_updates", None) | ||
| queryset = prefetched if prefetched is not None else obj.drefoperationalupdate_set.order_by("-created_at") | ||
| return MiniOperationalUpdateActiveSerializer(queryset, many=True).data | ||
|
|
||
| @extend_schema_field(MiniDrefFinalReportActiveSerializer) | ||
| def get_final_report_details(self, obj): | ||
| queryset = DrefFinalReport.objects.filter(dref_id=obj.id).first() | ||
| return MiniDrefFinalReportActiveSerializer(queryset).data | ||
| prefetched = getattr(obj, "prefetched_final_report", None) | ||
| if prefetched is not None: | ||
| if isinstance(prefetched, list): | ||
| final_report = prefetched[0] if prefetched else None | ||
| else: | ||
| final_report = prefetched | ||
| else: | ||
| try: | ||
| final_report = obj.dreffinalreport | ||
| except DrefFinalReport.DoesNotExist: | ||
| final_report = None | ||
| if final_report is None: | ||
| return None | ||
| return MiniDrefFinalReportActiveSerializer(final_report).data | ||
|
|
||
| def get_has_ops_update(self, obj) -> bool: | ||
| op_count_count = obj.drefoperationalupdate_set.count() | ||
| if op_count_count > 0: | ||
| return True | ||
| return False | ||
| prefetched = getattr(obj, "prefetched_operational_updates", None) | ||
| if prefetched is not None: | ||
| return len(prefetched) > 0 | ||
| return obj.drefoperationalupdate_set.exists() | ||
|
|
||
| def get_has_final_report(self, obj) -> bool: | ||
| if hasattr(obj, "dreffinalreport"): | ||
| return True | ||
| return False | ||
| prefetched = getattr(obj, "prefetched_final_report", None) | ||
| if prefetched is not None: | ||
| return bool(prefetched) | ||
| try: | ||
| return obj.dreffinalreport is not None | ||
| except DrefFinalReport.DoesNotExist: | ||
| return False | ||
|
|
||
| def get_application_type(self, obj) -> str: | ||
| return "DREF" | ||
|
|
@@ -278,10 +295,28 @@ def get_application_type_display(self, obj) -> str: | |
| return gettext("DREF application") | ||
|
|
||
| def get_unpublished_op_update_count(self, obj) -> int: | ||
| prefetched = getattr(obj, "prefetched_operational_updates", None) | ||
| if prefetched is not None: | ||
| return sum(1 for op in prefetched if op.status != Dref.Status.APPROVED) | ||
| return DrefOperationalUpdate.objects.filter(dref_id=obj.id).exclude(status=Dref.Status.APPROVED).count() | ||
|
|
||
| def get_unpublished_final_report_count(self, obj) -> int: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this field? Given that DrefFinalReport is a OneToOneField, the count can only ever be 0 or 1, so it doesn't seem to provide much additional information. If it's required for backward compatibility, we could annotate it in the queryset instead of calculating it in the serializer. |
||
| return DrefFinalReport.objects.filter(dref_id=obj.id).exclude(status=Dref.Status.APPROVED).count() | ||
| prefetched = getattr(obj, "prefetched_final_report", None) | ||
| if prefetched is not None: | ||
| if isinstance(prefetched, list): | ||
| if not prefetched: | ||
| return 0 | ||
| final_report = prefetched[0] | ||
| else: | ||
| final_report = prefetched | ||
| if final_report is None: | ||
| return 0 | ||
| return 0 if final_report.status == Dref.Status.APPROVED else 1 | ||
| try: | ||
| final_report = obj.dreffinalreport | ||
| except DrefFinalReport.DoesNotExist: | ||
| return 0 | ||
| return 0 if final_report.status == Dref.Status.APPROVED else 1 | ||
|
|
||
|
|
||
| class PlannedInterventionSerializer( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||
| import django.utils.timezone as timezone | ||||||
| from django.contrib.auth.models import Permission | ||||||
| from django.db import models, transaction | ||||||
| from django.db.models import Prefetch | ||||||
| from django.http import HttpResponse | ||||||
| from django.templatetags.static import static | ||||||
| from django.utils.translation import gettext | ||||||
|
|
@@ -345,7 +346,23 @@ class ActiveDrefOperationsViewSet(viewsets.ReadOnlyModelViewSet): | |||||
| permission_classes = [permissions.IsAuthenticated, DenyGuestUserPermission] | ||||||
| filterset_class = ActiveDrefFilterSet | ||||||
| queryset = ( | ||||||
| Dref.objects.prefetch_related("planned_interventions", "needs_identified", "national_society_actions", "users") | ||||||
| Dref.objects.select_related("country") | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| .prefetch_related( | ||||||
| "planned_interventions", | ||||||
| "needs_identified", | ||||||
| "national_society_actions", | ||||||
| "users", | ||||||
| Prefetch( | ||||||
| "dreffinalreport", | ||||||
| queryset=DrefFinalReport.objects.all(), | ||||||
| to_attr="prefetched_final_report", | ||||||
| ), | ||||||
|
Comment on lines
+355
to
+359
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is 1:1 Field, So no need to Prefetch DrefFinalReport. |
||||||
| Prefetch( | ||||||
| "drefoperationalupdate_set", | ||||||
| queryset=DrefOperationalUpdate.objects.order_by("-created_at"), | ||||||
| to_attr="prefetched_operational_updates", | ||||||
| ), | ||||||
| ) | ||||||
| .order_by("-created_at") | ||||||
| .filter(is_active=True) | ||||||
| .distinct() | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can shift most of these checks on the query level.