[OSDEV-2068] Enable users to download their own data without impacts to free & purchased data download allowances

[VadimKovalenkoSNF]

Fix OSDEV-2068

1. Introduced is_same_contributor_for_queryset() method. It can check contributor id based on request.user.contributor.id on BE, no need to parse URL query parameters. API user distinguished by token (if passed).
2. Applied BE logic to exclude users from their list's data.
3. Added unit tests and updated release notes.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/react/src/components/DownloadButtonWithFlags.jsx (2)

2-2: PropTypes: allow facilitiesCount to be null to match upstream data shape

Callers pass facilitiesCount = get(data, 'count', null), which can be null. PropTypes.number will warn for null even though you coerce to 0 via the local count. Allow null in PropTypes to eliminate noisy warnings.

-import { bool, number, func } from 'prop-types';
+import { bool, number, func, oneOfType, oneOf } from 'prop-types';

-    facilitiesCount: number,
+    facilitiesCount: oneOfType([number, oneOf([null])]),

Also applies to: 42-42

---

7-13: Consistency nit: consider aligning prop names with downstream “isEmbedded”

Minor naming drift: this component uses embed while DownloadFacilitiesButton refers to isEmbedded. For readability and grep-ability, consider renaming embed -> isEmbedded in this wrapper in a follow-up PR (requires updating call sites).

Also applies to: 40-46, 48-51

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b41c20a and 729f8fb.

📒 Files selected for processing (2)

• src/django/api/views/facility/facilities_view_set.py (2 hunks)
• src/react/src/components/DownloadButtonWithFlags.jsx (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/django/api/views/facility/facilities_view_set.py

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/react/src/components/DownloadButtonWithFlags.jsx (4)

src/django/api/views/facility/facilities_view_set.py (1)

• count (849-858)

src/react/src/components/FilterSidebarFacilitiesTab.jsx (1)

• facilitiesCount (317-317)

src/react/src/components/NonVectorTileFilterSidebarFacilitiesTab.jsx (1)

• facilitiesCount (201-201)

src/react/src/components/DownloadFacilitiesButton.jsx (1)

• DownloadFacilitiesButton (58-195)

🔇 Additional comments (3)

src/react/src/components/DownloadButtonWithFlags.jsx (3)

15-16: Nice: safe normalization of facilitiesCount

Using nullish coalescing to normalize facilitiesCount prevents disabled/upgrade logic from tripping on nulls. LGTM.

---

17-37: Double-check business rules: embedded vs. instance vs. allowance gating

Behavior summary:

• Public instance (alternative branch): disable only when embed && count > FACILITIES_DOWNLOAD_LIMIT; otherwise use userAllowedRecords for upgrade, bypassed when isSameContributor.
• Private instance (children branch): ignore allowances entirely; hard-disable when count > FACILITIES_DOWNLOAD_LIMIT.

Confirm this matches the intended policy (especially the embed path ignoring allowances and the private-instance hard cap regardless of isSameContributor). If the intent is different, we can help tweak the gating.

---

1-54: Prop rename check complete: no obsolete isAllUserContributed and all DownloadButtonWithFlags call sites use isSameContributor.

• src/react/src/components/FilterSidebarFacilitiesTab.jsx (lines 340–343)
• src/react/src/components/NonVectorTileFilterSidebarFacilitiesTab.jsx (lines 213–216)

No remaining instances of isAllUserContributed.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/react/src/components/DownloadButtonWithFlags.jsx (1)

20-31: Prop forwarding fixed: isSameContributor now reaches DownloadFacilitiesButton in both branches

This resolves the earlier inconsistency and ensures tooltip/UX logic downstream receives the same-contributor context.

Also applies to: 34-40

🧹 Nitpick comments (1)

src/react/src/components/DownloadButtonWithFlags.jsx (1)

14-14: Use nullish coalescing for clarity

Minor readability nit: prefer the nullish coalescing operator.

-    const count = facilitiesCount == null ? 0 : facilitiesCount;
+    const count = facilitiesCount ?? 0;

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 729f8fb and e8adcfb.

📒 Files selected for processing (1)

• src/react/src/components/DownloadButtonWithFlags.jsx (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

🧬 Code Graph Analysis (1)

src/react/src/components/DownloadButtonWithFlags.jsx (4)

src/django/api/views/facility/facilities_view_set.py (1)

• count (849-858)

src/react/src/components/FilterSidebarFacilitiesTab.jsx (1)

• facilitiesCount (317-317)

src/react/src/components/NonVectorTileFilterSidebarFacilitiesTab.jsx (1)

• facilitiesCount (201-201)

src/react/src/components/DownloadFacilitiesButton.jsx (1)

• DownloadFacilitiesButton (58-195)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-flake8-linter
• GitHub Check: run-countries-code-quality
• GitHub Check: run-integration-test-code-quality
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-dd-code-quality
• GitHub Check: run-django-code-quality
• GitHub Check: run-fe-code-quality
• GitHub Check: get-base-branch-django-cov
• GitHub Check: get-base-branch-fe-cov

🔇 Additional comments (1)

src/react/src/components/DownloadButtonWithFlags.jsx (1)

17-33: Confirm FeatureFlag branch semantics align with public vs. private behavior

FeatureFlag renders its children when the PRIVATE_INSTANCE flag is active (or the user is the same contributor), and the alternative otherwise. In this component:

• Children branch (<DownloadFacilitiesButton> rendered when PRIVATE_INSTANCE is enabled or contributor):
  • disabled if count > FACILITIES_DOWNLOAD_LIMIT
  • userAllowedRecords = FACILITIES_DOWNLOAD_LIMIT
  • no upgrade prop passed

• Alternative branch (rendered when PRIVATE_INSTANCE is disabled and not contributor):
  • disabled if embed && count > FACILITIES_DOWNLOAD_LIMIT
  • userAllowedRecords = userAllowedRecords (dynamic)
  • upgrade = !embed && !isSameContributor && count > userAllowedRecords

Please confirm that:

1. Private instances should indeed allow downloads up to the static FACILITIES_DOWNLOAD_LIMIT without an “upgrade” prompt.
2. Public instances should enforce the user-specific limit and show the upgrade path as coded.

If this deviates from product intent, adjust the thresholds or the inclusion of the upgrade prop accordingly.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/django/api/services/facilities_download_service.py (1)

103-115: Fix duplicate method: runtime TypeError and lost bypass logic

register_download_if_needed is defined twice. The second definition overrides the staticmethod above and only accepts two args, but callers pass three (including is_same_contributor). This will raise TypeError at runtime and also drop the bypass logic for same-contributor downloads.

Apply this unified implementation and remove the duplicate:

@@
-    @staticmethod
-    def register_download_if_needed(
-        limit: FacilityDownloadLimit,
-        record_count: int,
-        is_same_contributor: bool = False
-    ):
-        if is_same_contributor:
-            return
-
-    def register_download_if_needed(limit, record_count):
-        if limit:
-            limit.register_download(record_count)
+    @staticmethod
+    def register_download_if_needed(
+        limit: FacilityDownloadLimit,
+        record_count: int,
+        is_same_contributor: bool = False,
+    ):
+        if is_same_contributor or not limit or record_count <= 0:
+            return
+        limit.register_download(record_count)

🧹 Nitpick comments (2)

src/django/api/services/facilities_download_service.py (1)

98-101: Unused helper: either wire in or remove

check_pagination() isn’t called anywhere. Either invoke it where appropriate (e.g., after fetch_page_and_cache) or drop it to reduce dead code.

src/django/api/tests/test_facilities_download_viewset.py (1)

524-696: Add an over-quota-but-owned test

Add a test where the user has zero remaining quota but all results are owned; expect 200 and no deduction.

Proposed test to append:

+    @patch(
+        'api.constants.FacilitiesDownloadSettings.'
+        'FREE_FACILITIES_DOWNLOAD_LIMIT',
+        0,
+    )
+    def test_user_over_quota_can_download_when_all_results_owned(self):
+        user = self.create_user()
+        self.login_user(user)
+        contributor = Contributor.objects.create(
+            admin=user, name="C", contrib_type="Brand / Retailer"
+        )
+        # Pre-create a limit with zero remaining
+        limit = FacilityDownloadLimit.objects.create(
+            user=user, free_download_records=0, paid_download_records=0
+        )
+        with patch(
+            'api.services.facilities_download_service.'
+            'FacilitiesDownloadService.get_filtered_queryset'
+        ) as mock_get_queryset:
+            mock_queryset = MagicMock()
+            owned = MagicMock()
+            owned.contributors = [{'id': contributor.id}]
+            mock_queryset.__iter__.return_value = [owned] * 5
+            mock_queryset.count.return_value = 5
+            mock_get_queryset.return_value = mock_queryset
+            resp = self.get_facility_downloads(
+                {'contributors': [str(contributor.id)]}
+            )
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+            limit.refresh_from_db()
+            self.assertEqual(limit.free_download_records, 0)
+            self.assertEqual(limit.paid_download_records, 0)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e8adcfb and 55a6a6f.

📒 Files selected for processing (4)

• doc/release/RELEASE-NOTES.md (1 hunks)
• src/django/api/facilities_download_view_set.py (4 hunks)
• src/django/api/services/facilities_download_service.py (1 hunks)
• src/django/api/tests/test_facilities_download_viewset.py (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• doc/release/RELEASE-NOTES.md

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/services/facilities_download_service.py
• src/django/api/facilities_download_view_set.py

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/services/facilities_download_service.py
• src/django/api/facilities_download_view_set.py

🧬 Code graph analysis (3)

src/django/api/tests/test_facilities_download_viewset.py (1)

src/django/api/models/user.py (1)

• User (71-213)

src/django/api/services/facilities_download_service.py (1)

src/django/api/models/facility_download_limit.py (1)

• FacilityDownloadLimit (14-102)

src/django/api/facilities_download_view_set.py (2)

src/django/api/serializers/facility/utils.py (1)

• is_same_contributor_for_queryset (351-367)

src/django/api/services/facilities_download_service.py (4)

• FacilitiesDownloadService (40-257)
• register_download_if_needed (104-110)
• register_download_if_needed (112-114)
• send_email_if_needed (117-169)

🪛 Ruff (0.12.2)

src/django/api/services/facilities_download_service.py

100-100: Avoid specifying long messages outside the exception class

(TRY003)

---

105-105: Unused static method argument: limit

(ARG004)

---

106-106: Unused static method argument: record_count

(ARG004)

src/django/api/facilities_download_view_set.py

49-49: Undefined name queryset

(F821)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: run-integration-test-code-quality
• GitHub Check: run-flake8-linter
• GitHub Check: run-dd-code-quality
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-countries-code-quality
• GitHub Check: run-django-code-quality
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: get-base-branch-django-cov
• GitHub Check: run-fe-code-quality
• GitHub Check: get-base-branch-fe-cov

🔇 Additional comments (12)

src/django/api/facilities_download_view_set.py (3)

56-59: Good: limits bypassed for same-contributor results

Skipping limit fetch when is_same_contributor is true matches the PR objective.

---

89-96: Response shape looks good

rows, headers, and the new is_same_contributor flag are clearly surfaced under results.

---

115-121: Reconfirm call after service fix

Once the service method is unified, this 3-arg call will work and preserve the bypass behavior. Verify end-to-end that last-page registration is skipped when is_same_contributor is true.

src/django/api/tests/test_facilities_download_viewset.py (9)

5-5: Import MagicMock: OK

Needed for queryset stubbing.

---

8-8: Import Contributor: OK

Required to set up ownership scenarios.

---

422-422: countries as list: OK

Matches serializer expectations for list params.

---

524-553: Covers all-owned-by-user path

Test correctly asserts is_same_contributor true.

---

555-588: Mixed contributors path covered

Asserts false correctly.

---

590-599: No contributor on user path covered

Correctly expects false.

---

601-633: AND logic with multiple contributors covered

Good positive case.

---

635-662: Empty queryset behavior covered

Returns false as expected.

---

664-696: OR logic with multiple contributors covered

Good positive case.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/django/api/facilities_download_view_set.py (1)

92-96: New is_same_contributor field in payload — confirmed handled elsewhere

This addition is fine. Prior comments indicate tests and CSV export were updated to account for this field.

🧹 Nitpick comments (2)

src/django/api/facilities_download_view_set.py (2)

12-12: Prefer DB-level ownership check over Python iteration

Import is fine, but the current utility iterates the entire queryset, which is costly on large result sets. Implement this check with a single EXISTS query against contributors_id to leverage the GIN index.

Proposed change in src/django/api/serializers/facility/utils.py (outside this file):

-def is_same_contributor_for_queryset(queryset: Iterable, request) -> bool:
-    contributor = getattr(request.user, 'contributor', None)
-    if not contributor:
-        return False
-    current_user_contributor_id = contributor.id
-
-    found_any_facility = False
-    for facility in queryset:
-        found_any_facility = True
-        facility_contributor_ids = [
-            contributor.get('id') for contributor in facility.contributors
-            if contributor.get('id') is not None
-        ]
-        if current_user_contributor_id not in facility_contributor_ids:
-            return False
-
-    return found_any_facility
+def is_same_contributor_for_queryset(queryset, request) -> bool:
+    contributor = getattr(request.user, 'contributor', None)
+    if not contributor:
+        return False
+    cid = contributor.id
+    # True if there are no facilities outside the user's contributor set
+    return not queryset.exclude(contributors_id__contains=[cid]).exists()

---

48-51: Avoid evaluating the full queryset pre-pagination

Calling is_same_contributor_for_queryset(base_qs, request) currently walks the entire queryset in Python. With the DB-level refactor suggested above, this becomes a single efficient EXISTS query.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 55a6a6f and b725c55.

📒 Files selected for processing (1)

• src/django/api/facilities_download_view_set.py (4 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/facilities_download_view_set.py

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/facilities_download_view_set.py

🧬 Code graph analysis (1)

src/django/api/facilities_download_view_set.py (3)

src/django/api/serializers/facility/utils.py (1)

• is_same_contributor_for_queryset (351-367)

src/react/src/util/util.js (1)

• headers (1742-1744)

src/django/api/services/facilities_download_service.py (4)

• FacilitiesDownloadService (40-257)
• register_download_if_needed (104-110)
• register_download_if_needed (112-114)
• send_email_if_needed (117-169)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: get-base-branch-fe-cov
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: run-countries-code-quality
• GitHub Check: get-base-branch-django-cov
• GitHub Check: run-integration-test-code-quality
• GitHub Check: run-fe-code-quality
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-django-code-quality

🔇 Additional comments (3)

src/django/api/facilities_download_view_set.py (3)

56-59: Download gating logic reads correctly

The limit is fetched only when not private, not embed mode, and not the same contributor. This aligns with “allow own-data downloads even if the quota is exhausted.”

---

89-89: Row extraction change LGTM

The list comprehension is clear and matches the serializer shape.

---

112-126: Fix overridden method signature and prevent quota overdrafts

• Service: remove the second, 2-arg register_download_if_needed(limit, record_count) in src/django/api/services/facilities_download_service.py (lines 112–114) so it no longer overrides the intended staticmethod, and consolidate into a single staticmethod with signature (limit, record_count, is_same_contributor) that caps record_count by (limit.free_download_records + limit.paid_download_records) before calling limit.register_download(...).
• ViewSet: in src/django/api/facilities_download_view_set.py (around line 116), compute

  remaining_quota = limit.free_download_records + limit.paid_download_records
  to_register = min(total_records, remaining_quota)

  and pass to_register (not total_records) to FacilitiesDownloadService.register_download_if_needed.

⛔ Skipped due to learnings

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/django/api/services/facilities_download_service.py (1)

76-96: Include is_same_contributor in enforce_limits call
Update enforce_limits signature and bypass logic as proposed, and propagate the is_same_contributor flag in the view set at src/django/api/facilities_download_view_set.py lines 68–71:

-        FacilitiesDownloadService.enforce_limits(
-            qs=base_qs, limit=limit, is_first_page=is_first_page
-        )
+        FacilitiesDownloadService.enforce_limits(
+            qs=base_qs,
+            limit=limit,
+            is_first_page=is_first_page,
+            is_same_contributor=is_same_contributor,
+        )

Without this propagation the bypass for “own data” downloads won’t take effect.

🧹 Nitpick comments (2)

src/django/api/services/facilities_download_service.py (2)

98-101: Return a field-specific ValidationError and clarify the param name.

Improves API ergonomics and keeps responses consistent.

-    def check_pagination(page_queryset):
-        if page_queryset is None:
-            raise ValidationError("Invalid pageSize parameter")
-        return page_queryset
+    def check_pagination(page_items):
+        if page_items is None:
+            # DRF will render this as {"pageSize": ["Invalid pageSize parameter"]}
+            raise ValidationError({"pageSize": ["Invalid pageSize parameter"]})
+        return page_items

Optional typing to add at top-level (outside this hunk):

from typing import Optional, Sequence, Any  # near other imports

---

103-113: Type hint Optional for limit and tighten early-return logic.

Matches get_or_create_user_download_limit Optional return; keeps mypy happy.

-    def register_download_if_needed(
-        limit: FacilityDownloadLimit,
-        record_count: int,
-        is_same_contributor: bool = False
-    ):
-        if is_same_contributor:
-            return
-        if limit:
-            limit.register_download(record_count)
+    def register_download_if_needed(
+        limit: 'Optional[FacilityDownloadLimit]',
+        record_count: int,
+        is_same_contributor: bool = False
+    ) -> None:
+        if is_same_contributor or not limit:
+            return
+        limit.register_download(record_count)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b725c55 and 94bd8d4.

📒 Files selected for processing (1)

• src/django/api/services/facilities_download_service.py (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/services/facilities_download_service.py

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/services/facilities_download_service.py

🧬 Code graph analysis (1)

src/django/api/services/facilities_download_service.py (1)

src/django/api/models/facility_download_limit.py (1)

• FacilityDownloadLimit (14-102)

🪛 Ruff (0.12.2)

src/django/api/services/facilities_download_service.py

100-100: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: run-flake8-linter
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-integration-test-code-quality
• GitHub Check: run-countries-code-quality
• GitHub Check: run-django-code-quality
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-fe-code-quality
• GitHub Check: run-dd-code-quality
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: get-base-branch-django-cov

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/django/api/services/facilities_download_service.py (1)

76-96: Skip enforcing download limits for contributors’ own data
In src/django/api/facilities_download_view_set.py, wrap the call to FacilitiesDownloadService.enforce_limits so it only runs when is_same_contributor is False. For example:

if not is_same_contributor:
    FacilitiesDownloadService.enforce_limits(
        qs=base_qs, limit=limit, is_first_page=is_first_page
    )

This ensures users downloading their own records aren’t blocked by quota checks.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 94bd8d4 and 90c88e4.

📒 Files selected for processing (1)

• src/django/api/services/facilities_download_service.py (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/services/facilities_download_service.py

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/services/facilities_download_service.py

🧬 Code graph analysis (1)

src/django/api/services/facilities_download_service.py (1)

src/django/api/models/facility_download_limit.py (2)

• FacilityDownloadLimit (14-102)
• register_download (56-85)

🪛 Ruff (0.12.2)

src/django/api/services/facilities_download_service.py

100-100: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-flake8-linter
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: run-countries-code-quality
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-dd-code-quality
• GitHub Check: run-integration-test-code-quality
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-django-code-quality
• GitHub Check: get-base-branch-django-cov
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: run-fe-code-quality

🔇 Additional comments (1)

src/django/api/services/facilities_download_service.py (1)

98-101: Verify helper usage and update error message
No references to check_pagination were found; ensure this static helper is actually invoked or remove it. If retained, apply:

-    if page_queryset is None:
-        raise ValidationError("Invalid pageSize parameter")
+    if page_queryset is None:
+        raise ValidationError("Invalid pagination parameters")

[roman-stolar]

LGTM

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/django/api/services/facilities_download_service.py (2)

98-101: Simplify/align pagination validation and message constant.

This helper only guards against None and otherwise returns the input. Either inline this check at the call site or expand it to validate bounds (min/max pageSize) using the existing serializer. Also, prefer a central constant (e.g., APIErrorMessages.INVALID_PAGE_SIZE) to satisfy TRY003 and keep messages consistent.

-    def check_pagination(page_queryset):
-        if page_queryset is None:
-            raise ValidationError("Invalid pageSize parameter")
+    def check_pagination(page_queryset):
+        if page_queryset is None:
+            raise ValidationError(APIErrorMessages.INVALID_PAGE_SIZE)
         return page_queryset

If INVALID_PAGE_SIZE doesn't exist, I can add it to api.constants.

---

103-120: Type hint should accept None; clamp negative counts; add explicit return.

The caller may pass None for limit; reflect that in the signature. Minor cleanup: coerce and clamp in one line; add -> None for clarity.

-    def register_download_if_needed(
-        limit: FacilityDownloadLimit,
+    def register_download_if_needed(
+        limit: Optional[FacilityDownloadLimit],
         records_returned: int,
         is_same_contributor: bool = False
-    ):
+    ) -> None:
         if is_same_contributor or not limit:
             return
-        try:
-            count = int(records_returned)
-        except (TypeError, ValueError):
-            count = 0
-
-        if count <= 0:
+        try:
+            count = max(int(records_returned or 0), 0)
+        except (TypeError, ValueError):
+            count = 0
+        if count == 0:
             return
         limit.register_download(count)

Add: from typing import Optional at the top of this module.

src/django/api/facilities_download_view_set.py (1)

48-51: Avoid full queryset iteration; use DB-side existence checks.

is_same_contributor_for_queryset iterates the entire queryset, which can be very large. Compute this via a targeted exists() query on contributors_id to avoid loading all rows.

-        is_same_contributor = is_same_contributor_for_queryset(
-            base_qs,
-            request
-        )
+        # Optimize: DB-level check instead of iterating the whole queryset
+        contributor = getattr(request.user, 'contributor', None)
+        if contributor and base_qs.exists():
+            has_foreign = base_qs.exclude(contributors_id__contains=[contributor.id]).exists()
+            is_same_contributor = not has_foreign
+        else:
+            is_same_contributor = False

If contributors_id isn’t an ArrayField here, I can adapt the predicate accordingly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3828351 and fbe14e3.

📒 Files selected for processing (2)

• src/django/api/facilities_download_view_set.py (4 hunks)
• src/django/api/services/facilities_download_service.py (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/facilities_download_view_set.py
• src/django/api/services/facilities_download_service.py

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/facilities_download_view_set.py
• src/django/api/services/facilities_download_service.py

🧬 Code graph analysis (2)

src/django/api/facilities_download_view_set.py (2)

src/django/api/serializers/facility/utils.py (1)

• is_same_contributor_for_queryset (351-367)

src/django/api/services/facilities_download_service.py (3)

• FacilitiesDownloadService (40-262)
• register_download_if_needed (104-119)
• send_email_if_needed (122-174)

src/django/api/services/facilities_download_service.py (1)

src/django/api/models/facility_download_limit.py (2)

• FacilityDownloadLimit (14-102)
• register_download (56-85)

🪛 Ruff (0.12.2)

src/django/api/services/facilities_download_service.py

100-100: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: get-base-branch-django-cov
• GitHub Check: run-fe-code-quality
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-django-code-quality
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-countries-code-quality
• GitHub Check: run-integration-test-code-quality
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: run-flake8-linter
• GitHub Check: run-dd-code-quality
• GitHub Check: get-base-branch-dd-cov

🔇 Additional comments (3)

src/django/api/facilities_download_view_set.py (3)

55-59: Gating logic reads well and matches intent.

Skipping limit retrieval for private/embed/same-contributor scenarios is clear and avoids unnecessary DB hits.

---

89-89: LGTM: clear row extraction.

The comprehension is concise and explicit about the serialized shape.

---

92-96: Including is_same_contributor in the payload is appropriate.

This enables frontend gating while preserving backward compatibility (field addition). Assuming tests/CSV export were updated, this looks good.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (8)

src/django/api/tests/test_facilities_download_viewset.py (7)

520-523: API user loop test is fine; consider asserting non-deduction explicitly

Optionally assert FacilityDownloadLimit remains None/unchanged to harden the contract.

---

524-553: Stubbing only get_filtered_queryset risks serializer breakage; also assert quota bypass

The serializer may access many fields not present on MagicMocks. To avoid brittle tests and focus on the flag, also stub fetch_page_and_cache to return an empty page. Add an assertion that quotas aren’t decremented when is_same_contributor is True.

Apply this minimal guard around the request:

         with patch(
             'api.services.facilities_download_service.'
             'FacilitiesDownloadService.get_filtered_queryset'
-        ) as mock_get_queryset:
+        ) as mock_get_queryset, patch(
+            'api.services.facilities_download_service.'
+            'FacilitiesDownloadService.fetch_page_and_cache',
+            return_value=([], True)
+        ):

And add a dedicated quota-bypass test:

+    @patch(
+        'api.constants.FacilitiesDownloadSettings.FREE_FACILITIES_DOWNLOAD_LIMIT',
+        FREE_FACILITIES_DOWNLOAD_LIMIT,
+    )
+    def test_is_same_contributor_bypasses_quota_even_when_exhausted(self):
+        user = self.create_user()
+        self.login_user(user)
+        contributor = Contributor.objects.create(
+            admin=user, name="Test Contributor", contrib_type="Brand / Retailer"
+        )
+        FacilityDownloadLimit.objects.create(
+            user=user, free_download_records=0, paid_download_records=0
+        )
+        with patch(
+            'api.services.facilities_download_service.FacilitiesDownloadService.get_filtered_queryset'
+        ) as mock_get_queryset, patch(
+            'api.services.facilities_download_service.FacilitiesDownloadService.fetch_page_and_cache',
+            return_value=([], True),
+        ):
+            mock_qs = MagicMock()
+            mock_fac = MagicMock()
+            mock_fac.contributors = [{'id': contributor.id}]
+            mock_qs.__iter__.return_value = [mock_fac]
+            mock_qs.count.return_value = 1
+            mock_get_queryset.return_value = mock_qs
+            resp = self.get_facility_downloads({'contributors': [str(contributor.id)]})
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+            limit = FacilityDownloadLimit.objects.get(user=user)
+            self.assertEqual(limit.free_download_records, 0)
+            self.assertEqual(limit.paid_download_records, 0)

---

555-588: Mixed contributors case is covered; add fetch_page stub to reduce coupling

Same serializer fragility note applies; consider stubbing fetch_page_and_cache here too.

-        ) as mock_get_queryset:
+        ) as mock_get_queryset, patch(
+            'api.services.facilities_download_service.FacilitiesDownloadService.fetch_page_and_cache',
+            return_value=([], True)
+        ):

---

589-599: No-contributor user path verified

Solid. Optionally assert that the response still includes expected schema (rows/headers).

---

600-633: AND logic scenario is good; ensure deterministic contributor linkage

If Contributor->User linkage relies on a specific related_name, add a quick guard:

         contributor = Contributor.objects.create(
             admin=user,
             name="Test Contributor",
             contrib_type="Brand / Retailer"
         )
+        # Sanity: current user has an associated contributor
+        self.assertTrue(user.has_contributor)

---

634-663: Multi-page: free decremented by total_count — test is precise

Nice coverage. Consider asserting that no emails were sent if you mock notifications (optional).

---

717-749: OR logic with multiple contributors covered

Looks good. Same optional fetch_page stub suggestion applies to keep tests resilient.

src/django/api/facilities_download_view_set.py (1)

106-108: Avoid duplicate COUNT queries within the same request

You call base_qs.count() for count and again for returned_count. Cache once post-pagination and reuse.

-        if is_first_page:
-            payload['count'] = base_qs.count()
+        # Compute once and reuse as needed
+        total_count = base_qs.count() if (is_first_page or (limit and is_last_page)) else None
+        if is_first_page:
+            payload['count'] = total_count
...
-        if is_last_page and limit:
-            # Charge for the full result set, not just the last page size
-            returned_count = base_qs.count()
+        if is_last_page and limit:
+            # Charge for the full result set, not just the last page size
+            returned_count = total_count if total_count is not None else base_qs.count()

Also applies to: 110-127

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fbe14e3 and 7d8a6aa.

📒 Files selected for processing (2)

• src/django/api/facilities_download_view_set.py (4 hunks)
• src/django/api/tests/test_facilities_download_viewset.py (3 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py
• src/django/api/facilities_download_view_set.py

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py
• src/django/api/facilities_download_view_set.py

🧬 Code graph analysis (2)

src/django/api/tests/test_facilities_download_viewset.py (2)

src/django/api/models/user.py (1)

• User (71-213)

src/django/api/models/facility_download_limit.py (1)

• FacilityDownloadLimit (14-102)

src/django/api/facilities_download_view_set.py (2)

src/django/api/serializers/facility/utils.py (1)

• is_same_contributor_for_queryset (351-367)

src/django/api/services/facilities_download_service.py (3)

• FacilitiesDownloadService (40-262)
• register_download_if_needed (104-119)
• send_email_if_needed (122-174)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-dd-code-quality
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-flake8-linter
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-integration-test-code-quality
• GitHub Check: run-countries-code-quality
• GitHub Check: get-base-branch-django-cov
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: run-django-code-quality
• GitHub Check: run-fe-code-quality

🔇 Additional comments (10)

src/django/api/tests/test_facilities_download_viewset.py (5)

5-5: Good addition: MagicMock import enables lightweight queryset stubbing

No issues. This keeps tests fast and isolated.

---

8-8: Direct Contributor import is appropriate for setting up ownership scenarios

Looks correct; aligns with is_same_contributor test cases.

---

422-422: Passing countries as a list matches serializer expectations

LGTM.

---

664-687: Multi-page: paid consumption formula correct, matches model capping

LGTM.

---

688-716: Empty queryset returns False — good edge case

No changes needed.

src/django/api/facilities_download_view_set.py (5)

12-12: Import is correct and localized

No issues.

---

55-59: Correct: bypass limits when all rows belong to the requester

This aligns with the PR objective and still allows global gating elsewhere.

---

89-91: Serializer access pattern is fine; keep it robust to empty pages

No change needed.

---

92-96: Expose is_same_contributor in payload — consistent with frontend gating

LGTM. Confirm CSV export filters it out (already addressed in prior commits).

---

110-127: Charging by total result set on last page — correct and fixes under-charging

This resolves the prior under-deduction bug for multi-page downloads.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/django/api/tests/test_facilities_download_viewset.py (1)

516-523: Add token‐based auth coverage for API users
Extend test_api_user_not_limited_by_download_count to authenticate via DRF Token (no group) and assert unlimited behavior. For example, after creating user = self.create_user(is_api_user=True), add:

from rest_framework.authtoken.models import Token
token = Token.objects.create(user=user)
response = self.get_facility_downloads(headers={'HTTP_AUTHORIZATION': f'Token {token.key}'})
assert response.status_code == status.HTTP_200_OK
assert not Quota.objects.filter(user=user).exists()

[test_facilities_download_viewset.py:516-523]

🧹 Nitpick comments (9)

src/django/api/tests/test_facilities_download_viewset.py (9)

422-422: Param shape change to list looks correct—verify consistency across API.

If the endpoint now expects countries as a list, ensure all tests and docs use the list form to avoid regressions.

---

554-588: Mixed contributors correctly returns False; consider asserting that limits still decrement.

Add an assertion that a non-exempt result set triggers quota deductions to guard against accidental exemptions in mixed sets.

---

600-633: AND logic scenario looks right—confirm desired semantics for co-contributed facilities.

You treat a facility as “mine” if my contributor ID is present among contributors (even if others contributed). That matches the reviewer’s question (“…or they are a contributor to them”). Ensure this is the documented contract.

---

634-669: Solid multi-page free-limit test—assert invariants explicitly.

Optional: also assert that (initial_free - final_free) == min(total_count, initial_free) for clarity.

---

670-698: Paid rollover math OK—simplify expectation for readability.

You can compute expected deltas via a single flow to reduce mental math:

• total_to_subtract = min(total_count, free + paid)
• expected_free = max(free - total_to_subtract, 0)
• expected_paid = max(paid - max(total_to_subtract - free, 0), 0)

---

699-727: Empty queryset returns False—confirm contract.

Mathematically, “all items are mine” is vacuously true for empty sets, but returning False is fine if the flag is meant for gating, not math. Consider documenting this to avoid confusion.

---

728-761: OR logic scenario OK; strengthen robustness of queryset mock.

To future-proof against service refactors that chain queryset methods, stub common chainers on mock_queryset:
order_by, all, filter, values_list returning mock_queryset.

             mock_queryset = MagicMock()
+            # Future-proof method chaining used by the service/view
+            for m in ("order_by", "all", "filter", "distinct"):
+                setattr(mock_queryset, m, MagicMock(return_value=mock_queryset))

---

458-473: Edge case from learnings: prevent negative deductions—add explicit regression test.

Given past overdraft issues, add a case where total_count > free + paid (and exemption is False) to assert post-download free >= 0 and paid >= 0 with correct capping, or the endpoint rejects upfront per current behavior (documented).

---

524-761: Optionally: target the new helper directly for fast unit coverage.

If is_same_contributor_for_queryset is a pure helper, add a small unit test class for it (no HTTP, no pagination), asserting True/False across empty, mixed, and co-contributed cases. This keeps behavior well-specified independent of the view.

I can draft these focused unit tests if you confirm the helper’s module path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8a6aa and e9522a9.

📒 Files selected for processing (1)

• src/django/api/tests/test_facilities_download_viewset.py (3 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py

🧬 Code graph analysis (1)

src/django/api/tests/test_facilities_download_viewset.py (2)

src/django/api/models/user.py (1)

• User (71-213)

src/django/api/models/facility_download_limit.py (1)

• FacilityDownloadLimit (14-102)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: run-flake8-linter
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-dd-code-quality
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-django-code-quality
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: run-countries-code-quality
• GitHub Check: run-integration-test-code-quality
• GitHub Check: get-base-branch-django-cov
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: run-fe-code-quality

🔇 Additional comments (4)

src/django/api/tests/test_facilities_download_viewset.py (4)

5-5: LGTM: added mocks for service patching.

MagicMock is appropriate for simulating queryset/facility objects in these tests.

---

8-8: LGTM: importing Contributor.

Matches how contributor ownership is asserted in the new scenarios.

---

589-599: No-contributor user path covered.

Looks good; this exercises the fallback when request.user has no contributor.

---

1-761: All checks passed: helper, service, and token auth are correctly implemented
The is_same_contributor_for_queryset helper is defined in src/django/api/serializers/facility/utils.py and imported in both download and facility viewsets; FacilitiesDownloadService and its get_filtered_queryset method reside in src/django/api/services/facilities_download_service.py; token-based authentication is enabled via rest_framework.authtoken and referenced appropriately in settings and tests.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/django/api/tests/test_facilities_download_viewset.py (1)

769-804: Critical: test function defined at module scope with a self parameter.
This won’t run under unittest (or will error under pytest). It must be a method of FacilitiesDownloadViewSetTest.

-def test_exhausted_quota_all_mine_still_allowed(self):
-    user = self.create_user()
-    self.login_user(user)
+    def test_exhausted_quota_all_mine_still_allowed(self):
+        user = self.create_user()
+        self.login_user(user)
@@
-    contributor = Contributor.objects.create(
-        admin=user,
-        name="Test Contributor",
-        contrib_type="Brand / Retailer"
-    )
+        contributor = Contributor.objects.create(
+            admin=user,
+            name="Test Contributor",
+            contrib_type="Brand / Retailer"
+        )
@@
-    limit = FacilityDownloadLimit.objects.create(
-        user=user,
-        free_download_records=0,
-        paid_download_records=0,
-    )
+        limit = FacilityDownloadLimit.objects.create(
+            user=user,
+            free_download_records=0,
+            paid_download_records=0,
+        )
@@
-    with patch(
+        with patch(
             'api.services.facilities_download_service.'
             'FacilitiesDownloadService.get_filtered_queryset'
-    ) as mock_get_queryset:
-        mock_queryset = MagicMock()
-        mock_facility = MagicMock()
-        mock_facility.contributors = [{'id': contributor.id}]
-        mock_queryset.__iter__.return_value = [mock_facility]
-        mock_queryset.count.return_value = 1
-        mock_get_queryset.return_value = mock_queryset
+        ) as mock_get_queryset:
+            mock_queryset = MagicMock()
+            mock_facility = MagicMock()
+            mock_facility.contributors = [{'id': contributor.id}]
+            mock_queryset.__iter__.return_value = [mock_facility]
+            mock_queryset.count.return_value = 1
+            mock_get_queryset.return_value = mock_queryset
@@
-        resp = self.get_facility_downloads({
-            'contributors': [str(contributor.id)]
-        })
-        self.assertEqual(resp.status_code, status.HTTP_200_OK)
-        # Quotas must remain unchanged since it's an own-data download
-        limit.refresh_from_db()
-        self.assertEqual(limit.free_download_records, 0)
-        self.assertEqual(limit.paid_download_records, 0)
+            resp = self.get_facility_downloads({
+                'contributors': [str(contributor.id)]
+            })
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+            # Quotas must remain unchanged since it's an own-data download
+            limit.refresh_from_db()
+            self.assertEqual(limit.free_download_records, 0)
+            self.assertEqual(limit.paid_download_records, 0)

♻️ Duplicate comments (1)

src/django/api/tests/test_facilities_download_viewset.py (1)

524-553: Also assert quotas remain unchanged when all results are own data.
Good “is_same_contributor=True” assertion. Add explicit quota invariants here too for tighter coupling to PR objective (you already do this in a separate test).

     def test_is_same_contributor_true_when_all_facilities_belong_to_user(self):
         user = self.create_user()
         self.login_user(user)
+        limit = FacilityDownloadLimit.objects.create(
+            user=user, free_download_records=10, paid_download_records=5
+        )
@@
             self.assertEqual(response.status_code, status.HTTP_200_OK)
             self.assertTrue(
                 response.data['results']['is_same_contributor']
             )
+            limit.refresh_from_db()
+            self.assertEqual(limit.free_download_records, 10)
+            self.assertEqual(limit.paid_download_records, 5)

🧹 Nitpick comments (4)

src/django/api/tests/test_facilities_download_viewset.py (4)

422-422: Verify countries param shape change (list vs string).
If the API used to accept a single string, either ensure backward compatibility or add a negative test proving string input is rejected with a clear message.

Example follow-up test (optional):

+    def test_query_parameters_countries_accepts_string_for_backward_compat(self):
+        user = self.create_user()
+        self.login_user(user)
+        resp = self.get_facility_downloads({"countries": "IN"})
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)

---

600-633: LGTM: AND logic covered for multi-contributor facilities including the user.
Consider adding a counterpart where results include facilities lacking the user (still matching AND across other IDs) to ensure False—if that path is reachable in practice.

---

634-672: Add idempotency test: revisiting the last page should not double-charge.
You decrement on the last page using total_count. Guard with a test to ensure a repeated last-page request doesn’t subtract twice.

+    def test_multi_page_last_page_idempotent(self):
+        user = self.create_user()
+        self.login_user(user)
+        limit = FacilityDownloadLimit.objects.create(
+            user=user, free_download_records=50, paid_download_records=0
+        )
+        # Prime pagination and get count
+        resp_page1 = self.get_facility_downloads({"pageSize": 10, "page": 1})
+        self.assertEqual(resp_page1.status_code, status.HTTP_200_OK)
+        with patch(
+            'api.services.facilities_download_service.FacilitiesDownloadService.send_email_if_needed',
+            return_value=None
+        ):
+            # First last-page visit
+            self.assertEqual(
+                self.get_facility_downloads({"pageSize": 10, "page": 2}).status_code,
+                status.HTTP_200_OK
+            )
+            # Repeat last-page visit
+            self.assertEqual(
+                self.get_facility_downloads({"pageSize": 10, "page": 2}).status_code,
+                status.HTTP_200_OK
+            )
+        # Only one subtraction should occur
+        limit.refresh_from_db()
+        self.assertGreater(limit.free_download_records, 0)
+        self.assertLess(limit.free_download_records, 50)

---

534-544: DRY the repeated mocking of get_filtered_queryset.
Create a small helper to build the mock queryset and reduce duplication/noise across tests.

 class FacilitiesDownloadViewSetTest(APITestCase):
@@
+    def _mock_queryset_with_facilities(self, facilities):
+        mq = MagicMock()
+        mq.__iter__.return_value = facilities
+        mq.count.return_value = len(facilities)
+        return mq
@@
-        ) as mock_get_queryset:
-            mock_queryset = MagicMock()
-            mock_facility = MagicMock()
-            mock_facility.contributors = [{'id': contributor.id}]
-            mock_queryset.__iter__.return_value = [mock_facility]
-            mock_queryset.count.return_value = 1
-            mock_get_queryset.return_value = mock_queryset
+        ) as mock_get_queryset:
+            f = MagicMock(); f.contributors = [{'id': contributor.id}]
+            mock_get_queryset.return_value = self._mock_queryset_with_facilities([f])

(Apply similarly in other tests.)

Also applies to: 565-579, 611-623, 657-665, 690-697, 717-724, 745-758

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2ac4fab and f97dbdd.

📒 Files selected for processing (1)

• src/django/api/tests/test_facilities_download_viewset.py (3 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-06-17T10:55:08.363Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py

📚 Learning: 2025-06-18T12:46:27.549Z

Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.

Applied to files:

• src/django/api/tests/test_facilities_download_viewset.py

🧬 Code graph analysis (1)

src/django/api/tests/test_facilities_download_viewset.py (3)

src/django/api/models/user.py (1)

• User (71-213)

src/django/api/views/user/submit_new_user_form.py (1)

• create (19-69)

src/django/api/models/facility_download_limit.py (1)

• FacilityDownloadLimit (14-102)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-integration-test-code-quality
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: run-flake8-linter
• GitHub Check: run-dd-code-quality
• GitHub Check: get-base-branch-dd-cov
• GitHub Check: get-base-branch-django-cov
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: get-base-branch-fe-cov
• GitHub Check: run-django-code-quality
• GitHub Check: run-countries-code-quality
• GitHub Check: run-fe-code-quality

🔇 Additional comments (7)

src/django/api/tests/test_facilities_download_viewset.py (7)

5-5: LGTM: patch/MagicMock import is correct.
Used appropriately in tests below.

---

8-8: LGTM: importing Contributor is necessary for ownership tests.
Matches model usage in new tests.

---

554-588: LGTM: mixed contributors correctly yields is_same_contributor=False.
Mocks isolate the utility logic from filtering; assertion is precise.

---

589-599: LGTM: user without a Contributor gets is_same_contributor=False.
Covers the no-contributor branch.

---

673-704: LGTM: paid spillover calculation matches register_download semantics.
Free drained first, remainder deducted from paid; assertions align with model logic.

---

705-733: LGTM: empty queryset returns is_same_contributor=False.
Prevents false positives on empty results.

---

734-767: LGTM: OR logic case where user’s contributor is present.
Complements the mixed-contributors negative test.

[roman-stolar]

LGTM

[vlad-shapik]

LGTM