[OSDEV-2305] Add LiwingWage indicator support to the platform by vlad-shapik · Pull Request #842 · opensupplyhub/open-supply-hub

vlad-shapik · 2025-12-15T14:39:24Z

Introduced automatic wage indicator reference links on production location profiles for 171 countries.

Each production location now displays country-specific links to authoritative living wage and minimum wage information and its regional partner sites.
The wage indicator data is presented in both the national language and English, providing users with easy access to benchmarking information for fair wage assessments.
The links appear automatically based on the production location's country and are managed as a system-generated partner field that cannot be manually edited or deleted, ensuring data consistency and reliability across the platform.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (5)

src/django/api/models/wage_indicator_country_data.py (1)
67-98: Consider cache invalidation on WageIndicatorLinkTextConfig changes.

The get_links_with_text() method caches link texts for 1 hour (line 82). If an administrator updates the display text via the admin interface, users won't see the change for up to an hour. Consider adding a cache invalidation hook in the WageIndicatorLinkTextConfig model's save/delete methods.
Example cache invalidation in WageIndicatorLinkTextConfig model:
# In src/django/api/models/wage_indicator_link_text_config.py
from django.core.cache import cache

class WageIndicatorLinkTextConfig(Model):
    # ... existing fields ...
    
    def save(self, *args, **kwargs):
        super().save(*args, **kwargs)
        cache.delete('wage_indicator_link_texts')
    
    def delete(self, *args, **kwargs):
        result = super().delete(*args, **kwargs)
        cache.delete('wage_indicator_link_texts')
        return result
src/django/api/serializers/facility/facility_index_details_serializer.py (4)
403-404: Move logger initialization to module level.

Importing and initializing the logger inside the method is unconventional. Consider moving it to the module level for consistency with Django best practices.
🔎 Suggested change:

Add at module level (after the imports, before the class):
import logging

logger = logging.getLogger(__name__)
Then remove lines 403-404 from the method and use logger directly.
441-446: Use logging.exception to preserve stack trace.

The static analysis correctly identifies that logging.exception should be used instead of logging.error when logging from an exception handler. This preserves the full stack trace for debugging.

While catching a broad Exception here is intentional for graceful degradation (preventing one field failure from breaking the entire response), the stack trace should be preserved for debugging purposes.
🔎 Apply this diff:
             except Exception as exc:
-                logger.error(
+                logger.exception(
                     f"Failed to serialize partner field '{field_name}': "
                     f"{exc}"
                 )
                 grouped_data[field_name] = []
450-467: Consider adding error handling for individual provider failures.

If one provider's fetch_data raises an exception, it will prevent subsequent providers from being processed and may break the entire partner fields response. Consider wrapping individual provider calls in try/except for resilience.
🔎 Suggested change:
+        import logging
+        logger = logging.getLogger(__name__)
+        
         for provider in providers:
-            field_data = provider.fetch_data(production_location)
-            if field_data is not None:
-                system_fields.append(field_data)
+            try:
+                field_data = provider.fetch_data(production_location)
+                if field_data is not None:
+                    system_fields.append(field_data)
+            except Exception:
+                logger.exception(
+                    f"Provider {provider.__class__.__name__} failed for "
+                    f"location {production_location.id}"
+                )
480-493: Extract cache TTL as a constant for consistency and maintainability.

The 60-second cache TTL is hardcoded in the cache.set() call. Consider extracting this as a module-level constant (e.g., PARTNER_FIELD_CACHE_TTL = 60) to centralize cache configuration and simplify adjustments if needed across the codebase.

Separately, confirm that PartnerField.objects.all() applies any intended filtering through a custom manager. If the PartnerField model uses a default manager with active field filtering, ensure this aligns with how cache invalidation is handled elsewhere in the codebase when partner fields are created, updated, or deleted.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b91f2a0 and 72d7691.

📒 Files selected for processing (21)

doc/release/RELEASE-NOTES.md (1 hunks)
src/django/api/admin.py (4 hunks)
src/django/api/migrations/0190_add_active_system_field_to_partnerfield.py (1 hunks)
src/django/api/migrations/0191_create_wage_indicator_partner_field.py (1 hunks)
src/django/api/migrations/0192_create_wage_indicator_models.py (1 hunks)
src/django/api/migrations/0193_populate_wage_indicator_data.py (1 hunks)
src/django/api/models/__init__.py (1 hunks)
src/django/api/models/partner_field.py (3 hunks)
src/django/api/models/partner_field_manager.py (1 hunks)
src/django/api/models/wage_indicator_country_data.py (1 hunks)
src/django/api/models/wage_indicator_link_text_config.py (1 hunks)
src/django/api/partner_fields/base_provider.py (1 hunks)
src/django/api/partner_fields/registry.py (1 hunks)
src/django/api/partner_fields/wage_indicator_provider.py (1 hunks)
src/django/api/serializers/facility/facility_index_details_serializer.py (2 hunks)
src/django/api/serializers/facility/facility_index_serializer.py (5 hunks)
src/django/api/tests/test_facility_history_endpoint.py (1 hunks)
src/django/api/tests/test_facility_index_details_serializer.py (4 hunks)
src/django/api/tests/test_facility_index_serializer.py (0 hunks)
src/django/api/tests/test_wage_indicator_provider.py (1 hunks)
src/react/src/components/PartnerFields/PartnerFieldSchemaValue.jsx (1 hunks)

💤 Files with no reviewable changes (1)

src/django/api/tests/test_facility_index_serializer.py

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

src/django/api/models/init.py
src/django/api/partner_fields/registry.py

🧰 Additional context used

🧠 Learnings (10)

📚 Learning: 2025-12-18T12:50:17.910Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 839
File: src/react/src/components/ClaimedFacilitiesDetails/ClaimedFacilitiesDetails.jsx:633-638
Timestamp: 2025-12-18T12:50:17.910Z
Learning: In projects using Material-UI v3 (as in opensupplyhub/open-supply-hub), Typography variant names such as 'headline' and 'title' are valid and not deprecated. When reviewing components, ensure Typography components use v3-compatible variants (e.g., variant='headline' or 'title') and avoid assuming v4+ renamed variants. This guidance applies to all JSX files in the codebase that render Typography from material-ui/core.

Applied to files:

src/react/src/components/PartnerFields/PartnerFieldSchemaValue.jsx

📚 Learning: 2024-11-28T11:29:28.139Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py:23-24
Timestamp: 2024-11-28T11:29:28.139Z
Learning: The files `src/django/api/tests/test_facility_claim_view_set.py`, `src/django/api/views/facility/facilities_view_set.py`, and `src/django/api/facility_actions/processing_facility_api.py` are not part of the V1 codebase in the Open Supply Hub project.

Applied to files:

src/django/api/tests/test_facility_history_endpoint.py
src/django/api/tests/test_facility_index_details_serializer.py
src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2024-11-28T11:40:54.281Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/tests/test_production_locations_view_set.py:45-46
Timestamp: 2024-11-28T11:40:54.281Z
Learning: Files `src/django/api/views/facility/facilities_view_set.py`, `src/django/api/facility_actions/processing_facility_api.py`, and `src/django/api/tests/test_facility_claim_view_set.py` are not part of v1 and should not be considered in v1-related code reviews.

Applied to files:

src/django/api/tests/test_facility_history_endpoint.py
src/django/api/tests/test_facility_index_details_serializer.py
src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2024-11-28T12:54:15.135Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/tests/test_facility_claim_view_set.py:0-0
Timestamp: 2024-11-28T12:54:15.135Z
Learning: The `test_facility_claim_view_set.py` file is not related to V1, so changes specific to V1 error responses, like updating 'message' to 'detail', should not be applied to it.

Applied to files:

src/django/api/tests/test_facility_history_endpoint.py

📚 Learning: 2024-12-12T14:59:19.694Z

Learnt from: mazursasha1990
Repo: opensupplyhub/open-supply-hub PR: 438
File: src/django/api/tests/test_moderation_events_add_production_location.py:21-65
Timestamp: 2024-12-12T14:59:19.694Z
Learning: In `src/django/api/tests/test_moderation_events_add_production_location.py`, the tests have been refactored, and the use of `POST` methods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.

Applied to files:

src/django/api/tests/test_facility_history_endpoint.py

📚 Learning: 2024-11-28T12:54:16.114Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/tests/test_facility_claim_view_set.py:0-0
Timestamp: 2024-11-28T12:54:16.114Z
Learning: In files not related to v1, such as `src/django/api/tests/test_facility_claim_view_set.py`, error responses use `'message'` instead of `'detail'`.

Applied to files:

src/django/api/tests/test_facility_history_endpoint.py

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: The `contributors` parameter in `src/django/api/serializers/facility/facility_query_params_serializer.py` is defined as `ListField(child=IntegerField(required=False))`, which automatically validates and converts contributor IDs to integers before they reach utility functions, eliminating the need for defensive parsing in downstream code.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: In the Open Supply Hub codebase, the CONTRIBUTORS query parameter validation is handled in `src/django/api/serializers/facility/facility_query_params_serializer.py`, so additional validation for contributor ID parsing is not needed in utility functions like `is_same_contributor_from_url_param` in `src/django/api/serializers/facility/utils.py`.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-04-11T05:31:54.895Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 578
File: src/django/api/serializers/facility/facility_create_claim_serializer.py:20-26
Timestamp: 2025-04-11T05:31:54.895Z
Learning: In the Open Supply Hub project, the `validate_workers` function in facility claim serializers intentionally returns `None` for invalid worker counts rather than raising validation errors, as this behavior aligns with the project's business logic.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-04-22T20:16:21.889Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 598
File: src/django/api/serializers/v1/additional_identifiers_serializer.py:0-0
Timestamp: 2025-04-22T20:16:21.889Z
Learning: The validation logic for AdditionalIdentifiersSerializer will be implemented in a future PR. The current implementation is intentionally a placeholder.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

🧬 Code graph analysis (14)

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

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

WageIndicatorCountryData (11-109)

src/django/api/partner_fields/wage_indicator_provider.py (4)

WageIndicatorProvider (8-64)

_get_field_name (16-18)

_fetch_raw_data (20-30)

_format_data (32-64)

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

get_all_including_inactive (11-13)

src/django/api/migrations/0191_create_wage_indicator_partner_field.py (2)

src/django/api/migrations/0192_create_wage_indicator_models.py (1)

Migration (7-118)

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

PartnerField (11-138)

delete (133-138)

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

src/django/api/models/facility/facility_index.py (1)

FacilityIndex (13-225)

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

PartnerField (11-138)

save (104-131)

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

FacilityIndexDetailsSerializer (37-493)

src/django/api/migrations/0192_create_wage_indicator_models.py (2)

src/django/api/migrations/0193_populate_wage_indicator_data.py (1)

Migration (2226-2241)

src/django/api/migrations/0191_create_wage_indicator_partner_field.py (1)

Migration (66-77)

src/django/api/migrations/0190_add_active_system_field_to_partnerfield.py (1)

src/django/api/migrations/0191_create_wage_indicator_partner_field.py (1)

Migration (66-77)

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

src/django/api/models/wage_indicator_country_data.py (3)

WageIndicatorCountryData (11-109)

LinkType (19-37)

Meta (60-62)

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

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

Meta (36-38)

WageIndicatorLinkTextConfig (6-46)

src/django/api/admin.py (4)

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

WageIndicatorCountryData (11-109)

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

WageIndicatorLinkTextConfig (6-46)

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

PartnerField (11-138)

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

get_all_including_inactive (11-13)

get_queryset (7-9)

src/django/api/migrations/0193_populate_wage_indicator_data.py (3)

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

WageIndicatorCountryData (11-109)

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

WageIndicatorLinkTextConfig (6-46)

src/django/api/migrations/0192_create_wage_indicator_models.py (1)

Migration (7-118)

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

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

get_queryset (340-348)

src/django/api/partner_fields/base_provider.py (3)

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

PartnerField (11-138)

src/django/api/partner_fields/wage_indicator_provider.py (3)

_fetch_raw_data (20-30)

_format_data (32-64)

_get_field_name (16-18)

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

get_all_including_inactive (11-13)

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

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

PartnerFieldManager (4-13)

get_all_including_inactive (11-13)

src/django/api/serializers/facility/facility_index_serializer.py (2)

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

is_created_at_main_date (44-52)

get_efs_associated_with_contributor (148-155)

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

extended_fields (148-192)

src/django/api/serializers/facility/facility_index_details_serializer.py (4)

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

parse_raw_data (29-38)

get_csv_values (41-46)

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

FacilityIndexExtendedFieldListSerializer (10-110)

src/django/api/partner_fields/registry.py (1)

providers (15-17)

src/django/api/partner_fields/base_provider.py (1)

fetch_data (20-38)

🪛 Ruff (0.14.8)

src/django/api/migrations/0191_create_wage_indicator_partner_field.py

6-6: Unused function argument: schema_editor

(ARG001)

60-60: Unused function argument: schema_editor

(ARG001)

68-70: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

72-77: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

src/django/api/migrations/0192_create_wage_indicator_models.py

9-11: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

13-118: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

src/django/api/migrations/0190_add_active_system_field_to_partnerfield.py

9-11: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

13-38: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

src/django/api/admin.py

384-384: Unused method argument: request

(ARG002)

src/django/api/migrations/0193_populate_wage_indicator_data.py

2179-2179: Unused function argument: schema_editor

(ARG001)

2211-2211: Unused function argument: schema_editor

(ARG001)

2232-2234: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

2236-2241: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

src/django/api/partner_fields/base_provider.py

74-77: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

111-117: Consider moving this statement to an else block

(TRY300)

src/django/api/serializers/facility/facility_index_details_serializer.py

441-441: Do not catch blind exception: Exception

(BLE001)

442-445: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

⏰ 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-contricleaner-code-quality
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: run-django-code-quality
GitHub Check: get-base-branch-fe-cov
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: run-countries-code-quality
GitHub Check: get-base-branch-dd-cov
GitHub Check: run-fe-code-quality
GitHub Check: get-base-branch-countries-cov
GitHub Check: run-dd-code-quality
GitHub Check: run-integration-test-code-quality
GitHub Check: get-base-branch-django-cov

🔇 Additional comments (28)

src/react/src/components/PartnerFields/PartnerFieldSchemaValue.jsx (1)

82-86: LGTM! Clean visual separation for multiple items.

The logic correctly inserts a line break before each subsequent item when rendering multiple partner field entries, improving readability for system-generated fields like wage indicators.

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

724-745: LGTM! More robust test validation.

The test now explicitly searches for the expected dissociation entry by matching both action and detail, rather than assuming it's the first element. This is more resilient to changes in the history entry ordering.

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

1-13: LGTM! Clean manager implementation.

The custom manager correctly filters active partner fields by default while providing get_all_including_inactive() for admin and special cases. This aligns with the system-field visibility pattern introduced in this PR.

src/django/api/tests/test_facility_index_details_serializer.py (6)

188-196: LGTM! Clean test setup.

The test fixture correctly creates a PartnerField and associates it with the contributor for subsequent test scenarios.

484-535: LGTM! Comprehensive partner field tests.

The tests correctly verify that partner fields are serialized as a dictionary with the expected structure, validating both the container type and the presence of field values.

537-617: LGTM! Source attribution tests.

The tests properly validate both the presence of source_by when set and its absence (None) when not configured, ensuring correct attribution display for system-generated fields.

619-695: LGTM! Unit field tests.

The tests correctly verify unit field handling, including both populated and empty string scenarios, ensuring proper metadata serialization.

697-773: LGTM! Label field tests.

The tests properly validate label field serialization, covering both populated and empty string cases for display text configuration.

775-870: LGTM! JSON schema tests.

The tests correctly verify JSON schema handling for object-type partner fields, including both populated schema objects and null values, which is critical for the wage indicator field's structured data.

src/django/api/migrations/0191_create_wage_indicator_partner_field.py (1)

6-77: LGTM! Migration correctly creates the wage indicator field.

The migration properly creates a system-generated wage_indicator partner field with:

A JSON schema defining the structure for wage indicator links and display text

Attribution to WageIndicator via the source_by field

system_field=True to protect against deletion

Note: Static analysis warnings about unused schema_editor parameters and ClassVar annotations are false positives—these follow standard Django migration patterns.

src/django/api/migrations/0190_add_active_system_field_to_partnerfield.py (1)

7-38: LGTM! Migration correctly adds active and system_field flags.

The migration properly introduces two boolean fields to the PartnerField model:

active: Controls visibility and availability (default True)

system_field: Marks system-generated fields for protection (default False)

Both fields have clear help text explaining their purpose. The static analysis warnings about ClassVar are false positives for Django migration patterns.

src/django/api/models/partner_field.py (3)

80-99: LGTM! System field infrastructure added.

The new active and system_field boolean fields provide the necessary infrastructure for system-generated fields and visibility control. The custom PartnerFieldManager ensures active filtering by default.

104-131: LGTM! Smart cache invalidation logic.

The enhanced save() method correctly:

Uses _state.adding to detect new records (works with UUID primary keys)

Fetches the original via get_all_including_inactive() to avoid manager filtering

Invalidates caches only when active status changes or on new record creation

Handles the DoesNotExist exception appropriately

This ensures cache consistency while minimizing unnecessary invalidations.

133-138: LGTM! Consistent cache cleanup on deletion.

The delete() method properly invalidates both cache keys after deletion, maintaining consistency with the save() behavior.

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

1-46: LGTM! Clean configuration model for link display text.

The WageIndicatorLinkTextConfig model provides a clean way to configure display text for wage indicator links:

Uses link_type as the primary key, referencing WageIndicatorCountryData.LinkType.choices

Allows admin-configurable display text without per-country duplication

Clear help text and verbose names

Descriptive __str__ method for admin display

This design enables centralized management of link text while maintaining consistency with the country data model.

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

1-238: LGTM! Comprehensive test coverage.

The test suite is well-structured and provides thorough coverage of the WageIndicatorProvider:

Tests basic provider methods (_get_field_name, _fetch_raw_data, _format_data)

Validates end-to-end data flow

Covers error scenarios (missing wage data, missing contributors, inactive fields)

Tests link text configuration and cache invalidation

The test isolation approach using get_all_including_inactive() and explicit contributor cleanup ensures tests don't interfere with each other.

src/django/api/partner_fields/wage_indicator_provider.py (1)

1-64: Past review comments already address the key issues in this file.

The previous reviews correctly identified:

The placeholder contributor ID that needs to be replaced (line 14)

The magic number for facility_list_item_id (line 62)

No additional issues found in the current implementation.

src/django/api/migrations/0193_populate_wage_indicator_data.py (1)

1-2241: Data migration implementation looks solid.

The migration correctly:

Seeds 171 countries with wage indicator URLs

Configures 3 link text display strings

Uses efficient bulk_create with appropriate batch sizing

Provides proper reverse migration

The Nepal HTTPS issue flagged in previous reviews has been resolved.

src/django/api/migrations/0192_create_wage_indicator_models.py (1)

1-118: Migration correctly defines the wage indicator models.

The migration properly creates:

WageIndicatorCountryData with country_code as primary key and three URL fields

WageIndicatorLinkTextConfig with link_type as primary key linked to the LinkType choices

Field constraints (max_length, help_text) are appropriate and match the model definitions.

src/django/api/partner_fields/base_provider.py (1)

1-119: Well-designed provider abstraction.

The SystemPartnerFieldProvider base class provides a clean template method pattern:

fetch_data() orchestrates the workflow

Abstract methods define extension points for concrete providers

Graceful error handling with None returns and appropriate logging

Properly uses get_all_including_inactive() to access system fields

The implementation correctly separates concerns between raw data fetching and formatting.

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

413-462: Refactoring to private methods improves encapsulation.

The conversion of double-underscore methods to single-underscore static methods is a good refactoring:

_get_request(), _date_field_to_sort(), _sort_order(), _sort_order_excluding_date(), _filter_contributor_extended_fields() are now properly scoped as internal helpers

Making them static where possible reduces instance coupling

The removal of partner_fields integration appears to be part of a broader architectural shift toward system-generated fields handled elsewhere.

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

285-326: Robust protection for system fields.

The clean() validation correctly prevents modifications to critical fields (name, type, json_schema, system_field) on system-defined partner fields. The approach:

Fetches the original instance using get_all_including_inactive()

Compares protected field values

Adds field-specific errors with helpful messages

This prevents accidental breakage while still allowing updates to presentation fields (label, unit, source_by, etc.).

340-361: Proper queryset and permission overrides for system fields.

The admin correctly:

Overrides get_queryset() to show inactive fields (line 344), ensuring system fields are always visible even when inactive

Prevents deletion of system fields via has_delete_permission() (lines 350-361) with a user-friendly warning message

This ensures system fields remain manageable but protected from destructive operations.

376-392: Country code primary key handling is correct.

Making country_code read-only when editing (line 389) prevents accidental changes to the primary key, which would create a new record rather than updating the existing one. This is the correct approach for primary key fields in Django admin.

doc/release/RELEASE-NOTES.md (1)

12-35: LGTM! Release notes are comprehensive and well-structured.

The documentation accurately describes the wage indicator feature including:

All four migrations with clear descriptions

Schema changes for PartnerField model extensions and new WageIndicator models

API endpoint changes with proper coverage

User-facing feature description in "What's new"

The release instructions correctly specify migrate and reindex_database commands.

src/django/api/serializers/facility/facility_index_details_serializer.py (3)

1-34: Imports are well-organized and appropriate for the new functionality.

All new imports are necessary for the partner fields feature implementation.

358-392: Well-structured method with clear separation of concerns.

The get_partner_fields method follows a clean pattern: fetch system fields → merge → filter → group → serialize. The use of helper methods improves readability and maintainability.

469-478: LGTM!

Clean and efficient implementation using defaultdict. Properly handles edge case of missing field_name.

src/django/api/serializers/facility/facility_index_serializer.py

protsack-stephan

Good work! Couple of comments from me.

Note that I might don't understand the full context of the work and therefore feel free to ignore my comments if you think they are irrelevant.

Also, I'm most likely won't be able to resolve the comments that I've left so I would ask you to resolve them on your own if you consider them done. Thanks!

protsack-stephan · 2025-12-19T09:51:26Z

src/django/api/models/wage_indicator_country_data.py

+    updated_at = DateTimeField(auto_now=True)
+
+    class Meta:
+        verbose_name = 'Wage Indicator - Country Data'


Can you please change this to Wageindicator country. I’m asking to ensure consistency with other Django admin menu items, especially since they use the name Wagenidicator instead of Wage Indicator.

protsack-stephan · 2025-12-19T09:51:39Z

src/django/api/models/wage_indicator_country_data.py

+
+    class Meta:
+        verbose_name = 'Wage Indicator - Country Data'
+        verbose_name_plural = 'Wage Indicator - Country Data'


Can you please change this to Wageindicator countries. I’m asking to ensure consistency with other Django admin menu items, especially since they use the name Wagenidicator instead of Wage Indicator.

protsack-stephan · 2025-12-19T09:51:55Z

src/django/api/migrations/0192_create_wage_indicator_models.py

+                ('updated_at', DateTimeField(auto_now=True)),
+            ],
+            options={
+                'verbose_name': 'Wage Indicator - Country Data',


Can you please change this to Wageindicator country. I’m asking to ensure consistency with other Django admin menu items, especially since they use the name Wagenidicator instead of Wage Indicator.

protsack-stephan · 2025-12-19T09:52:08Z

src/django/api/migrations/0192_create_wage_indicator_models.py

+            ],
+            options={
+                'verbose_name': 'Wage Indicator - Country Data',
+                'verbose_name_plural': 'Wage Indicator - Country Data',


Can you please change this to Wageindicator countries. I’m asking to ensure consistency with other Django admin menu items, especially since they use the name Wagenidicator instead of Wage Indicator.

protsack-stephan · 2025-12-19T09:57:09Z

src/django/api/models/wage_indicator_link_text_config.py

+    updated_at = DateTimeField(auto_now=True)
+
+    class Meta:
+        verbose_name = 'Wage Indicator - Link Text'


Can you please change this to Wageindicator link text. I’m asking to ensure consistency with other Django admin menu items, especially since they use the name Wagenidicator instead of Wage Indicator.

src/django/api/admin.py

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (4)

src/django/api/models/partner_field.py (2)
104-131: Previous feedback on cache invalidation remains unaddressed.

The suggestions from prior reviews to extract a helper function and use a constant for 'partner_field_names' would improve maintainability:
🔎 Suggested refactor
+PARTNER_FIELD_NAMES_KEY = 'partner_field_names'
+
+def invalidate_partner_fields_cache():
+    cache.delete(PARTNER_FIELD_LIST_KEY)
+    cache.delete(PARTNER_FIELD_NAMES_KEY)
+
 def save(self, *args, **kwargs):
     ...
-        if should_invalidate_cache:
-            cache.delete(PARTNER_FIELD_LIST_KEY)
-            cache.delete('partner_field_names')
+        if should_invalidate_cache:
+            invalidate_partner_fields_cache()
119-121: Add logging when the original record is unexpectedly missing.

The except PartnerField.DoesNotExist block silently handles the case where a record doesn't exist during an update operation. This could indicate a race condition or data integrity issue that warrants visibility.
🔎 Suggested improvement
+import logging
+
+logger = logging.getLogger(__name__)
+
             except PartnerField.DoesNotExist:
-                # If original doesn't exist, treat as new.
+                # If original doesn't exist, treat as new and log warning.
+                logger.warning(
+                    'PartnerField with pk=%s not found during save; '
+                    'treating as new record.',
+                    self.pk
+                )
                 should_invalidate_cache = True
src/django/api/models/wage_indicator_link_text_config.py (1)

36-38: Verify naming consistency with past feedback.

Past review comments suggested using "Wageindicator" (one word, lowercase 'i') instead of "WageIndicator" for consistency with other Django admin menu items. The current verbose names still use "WageIndicator". Please confirm whether this naming was intentionally retained or if it needs to be updated to match the requested "Wageindicator" format.

src/django/api/migrations/0192_create_wage_indicator_models.py (1)

12-57: Verify verbose name consistency with past feedback.

The model structure and field definitions are correct. However, similar to the other model, past review comments suggested using "Wageindicator" instead of "WageIndicator" for consistency. Please confirm the naming convention decision.

🧹 Nitpick comments (1)

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

40-46: Consider caching the formatted link_type.

The __str__ method calls self.link_type.replace('_', ' ').title() twice. For better efficiency, consider storing the result in a variable.

🔎 Proposed refactor

 def __str__(self):
+    formatted_link_type = self.link_type.replace('_', ' ').title()
     return ("The wage indicator link "
-            f"'{self.link_type.replace('_', ' ').title()}' "
+            f"'{formatted_link_type}' "
             f"has the link display text: '{self.display_text}'. This data "
             "then maps to the WageIndicatorCountryData model's "
-            f"'{self.link_type.replace('_', ' ').title()}' field which is "
+            f"'{formatted_link_type}' field which is "
             "a URL field.")

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6ea4aa and 133a4b5.

📒 Files selected for processing (4)

src/django/api/migrations/0192_create_wage_indicator_models.py
src/django/api/models/partner_field.py
src/django/api/models/wage_indicator_country_data.py
src/django/api/models/wage_indicator_link_text_config.py

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

src/django/api/models/wage_indicator_country_data.py

🧰 Additional context used

🧬 Code graph analysis (3)

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

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

PartnerFieldManager (4-13)

get_all_including_inactive (11-13)

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

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

WageIndicatorCountryData (11-109)

LinkType (19-37)

src/django/api/migrations/0192_create_wage_indicator_models.py (4)

src/django/api/migrations/0193_populate_wage_indicator_data.py (1)

Migration (2226-2241)

src/django/api/migrations/0190_add_active_system_field_to_partnerfield.py (1)

Migration (7-38)

src/django/api/migrations/0191_create_wage_indicator_partner_field.py (1)

Migration (66-77)

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

fields (1618-1618)

🪛 Ruff (0.14.10)

src/django/api/migrations/0192_create_wage_indicator_models.py

8-10: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

12-104: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

⏰ 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-flake8-linter
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: run-integration-test-code-quality
GitHub Check: run-contricleaner-code-quality
GitHub Check: run-countries-code-quality
GitHub Check: get-base-branch-countries-cov
GitHub Check: get-base-branch-fe-cov
GitHub Check: run-django-code-quality
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: run-fe-code-quality
GitHub Check: run-dd-code-quality
GitHub Check: get-base-branch-django-cov

🔇 Additional comments (8)

src/django/api/models/partner_field.py (4)

80-94: LGTM!

The new active and system_field boolean fields are well-defined with appropriate defaults and descriptive help text.

99-100: LGTM!

The custom PartnerFieldManager is correctly assigned, ensuring that default queries only return active partner fields while still providing access to all records via get_all_including_inactive().

27-28: LGTM!

Correct plural form for admin display.

133-138: Protection against system field deletion is already properly enforced via the admin layer.

The PartnerFieldAdmin.has_delete_permission() method checks the system_field flag and prevents deletion with a warning message. Additionally, PartnerField is not exposed via any public API endpoint—it's only used for reading data in serializers and views. This design adequately prevents accidental deletion of system fields.

src/django/api/models/wage_indicator_link_text_config.py (3)

1-4: LGTM!

The imports are appropriate and correctly structured for this Django model.

6-24: LGTM!

The model structure is well-designed for a configuration table. Using the LinkType choices from WageIndicatorCountryData ensures consistency between the two models.

25-34: LGTM!

The display_text field and timestamp configuration are appropriate for this model's purpose.

src/django/api/migrations/0192_create_wage_indicator_models.py (1)

1-10: LGTM!

The migration structure and dependencies are correctly configured.

src/django/api/migrations/0192_create_wage_indicator_models.py

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

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

340-348: LGTM! Queryset override correctly shows all fields.

The override properly uses get_all_including_inactive() to ensure administrators can view and manage inactive partner fields. This is essential for admin functionality.

Optional: The comment could explain why rather than what per past feedback: "Administrators need visibility into inactive fields for auditing and reactivation purposes, unlike the public API which filters them out."

350-362: LGTM! Deletion protection correctly implemented.

The override properly prevents deletion of system-defined fields with clear user feedback. The implementation follows Django admin patterns correctly.

Optional: The comment could explain why rather than what per past feedback: "System fields are required for core application functionality and their deletion would break data display on production location profiles."

Note: The parameter name obj is standard Django admin convention, despite a past comment suggesting renaming to field.

377-393: LGTM! Wage indicator admin correctly configured.

The admin class properly implements country_code protection (readonly on edit, editable on create) to prevent primary key modification. The configuration provides appropriate visibility for wage indicator data management.

Note on static analysis: The unused request parameter flagged at line 385 is required by Django's get_readonly_fields method signature—this is a false positive.

Optional comment improvement: Per past feedback, explain why: "Prevent modification of the primary key (country_code) after creation to maintain referential integrity."

As per static analysis hints (false positive).

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 133a4b5 and c9d716b.

📒 Files selected for processing (1)

src/django/api/admin.py

🧰 Additional context used

🧬 Code graph analysis (1)

src/django/api/admin.py (4)

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

WageIndicatorCountryData (11-109)

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

WageIndicatorLinkTextConfig (6-46)

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

PartnerField (11-138)

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

get_all_including_inactive (11-13)

get_queryset (7-9)

🪛 Ruff (0.14.10)

src/django/api/admin.py

385-385: Unused method argument: request

(ARG002)

⏰ 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-contricleaner-code-quality
GitHub Check: run-flake8-linter
GitHub Check: run-dd-code-quality
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: get-base-branch-dd-cov
GitHub Check: run-integration-test-code-quality
GitHub Check: run-countries-code-quality
GitHub Check: get-base-branch-fe-cov
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: run-django-code-quality
GitHub Check: run-fe-code-quality
GitHub Check: get-base-branch-countries-cov
GitHub Check: get-base-branch-django-cov

🔇 Additional comments (6)

src/django/api/admin.py (6)

2-2: LGTM! Logging addition as requested.

The logging import and logger initialization follow best practices and address the previous review feedback requesting observability for system field operations.

Also applies to: 31-31

16-19: LGTM! Model imports are correct.

The imports for the new wage indicator models are properly structured and support the admin registrations below.

279-282: LGTM! Form fields correctly updated.

The addition of active and system_field to the form properly exposes the new attributes for admin management.

331-338: LGTM! Admin display fields properly configured.

The additions to list_display, list_filter, and fields correctly expose the new active and system_field attributes, providing good visibility and filtering options for administrators.

395-398: LGTM! Link text config admin is correctly configured.

The admin class provides appropriate visibility and search functionality for managing wage indicator link text configurations.

424-427: LGTM! Admin registrations are correct.

The new wage indicator models are properly registered with their respective admin classes, following the established pattern in this file.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/django/api/models/partner_field.py (2)
18-27: Consider consistent string quoting.

The TYPE_CHOICES now uses single quotes, but help_text strings throughout the file use double quotes. While Python accepts both, consistent quoting improves readability.

130-133: Clarify the error message.

The warning message states "System field must exist in database," but this exception handler catches DoesNotExist for any partner field, not just system fields. The message could be misleading when updating non-system fields.
🔎 Suggested improvement
-            logger.warning(
-                f"Partner field `{self.pk}` not found. "
-                "System field must exist in database."
-            )
+            logger.warning(
+                f"Partner field `{self.pk}` not found during save. "
+                "The record may have been deleted or the PK is invalid."
+            )

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d716b and a7d3419.

📒 Files selected for processing (2)

src/django/api/constants.py
src/django/api/models/partner_field.py

🧰 Additional context used

🧬 Code graph analysis (1)

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

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

PartnerFieldManager (4-13)

get_all_including_inactive (11-13)

⏰ 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-contricleaner-code-quality
GitHub Check: get-base-branch-dd-cov
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: run-django-code-quality
GitHub Check: run-fe-code-quality
GitHub Check: get-base-branch-fe-cov
GitHub Check: run-countries-code-quality
GitHub Check: run-dd-code-quality
GitHub Check: get-base-branch-django-cov
GitHub Check: run-integration-test-code-quality
GitHub Check: get-base-branch-countries-cov

🔇 Additional comments (5)

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

303-303: LGTM!

The new cache key constant follows the existing pattern and addresses the previous review feedback to avoid hardcoded cache key strings.

src/django/api/models/partner_field.py (4)

1-11: LGTM!

The logging setup and new imports are appropriate and address the previous review feedback to add logging for better visibility.

111-114: LGTM!

The cache invalidation helper follows the previous review feedback and implements the DRY principle effectively.

138-142: LGTM!

The delete method correctly invalidates the cache after removing the record from the database.

87-106: The custom manager's active filtering is correctly implemented and properly guarded throughout the codebase.

The PartnerFieldManager filters to active=True by default, but code requiring access to inactive fields explicitly uses get_all_including_inactive():

Admin operations (form validation, admin interface override)

Internal field modifications (partner_field.py save method)

System field providers (wage_indicator_provider)

Public APIs intentionally filter to active fields only, and tests validate this behavior. No broken code was found that should retrieve all fields but doesn't.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/django/api/partner_fields/base_provider.py (1)
79-88: Consider using logging.exception in exception handlers.

When logging within an except block, logging.exception automatically includes the stack trace, which aids debugging.
🔎 Suggested change
         except PartnerField.DoesNotExist:
-            logger.error(
+            logger.exception(
                 f"Partner field '{field_name}' not found. "
                 "System field must exist in database."
             )
src/django/api/serializers/facility/facility_index_details_serializer.py (1)
414-443: Consider using logger.exception for better debugging.

While catching the broad Exception is acceptable here for graceful degradation, using logger.exception instead of logger.error in the except handler would automatically include the stack trace, making debugging easier.
🔎 Suggested change
             except Exception as exc:
-                logger.error(
+                logger.exception(
                     f"Failed to serialize partner field '{field_name}': "
                     f"{exc}"
                 )

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7d3419 and 7947b5f.

📒 Files selected for processing (12)

src/django/api/admin.py
src/django/api/models/partner_field.py
src/django/api/models/wage_indicator_country_data.py
src/django/api/partner_fields/base_provider.py
src/django/api/partner_fields/registry.py
src/django/api/partner_fields/wage_indicator_provider.py
src/django/api/serializers/facility/facility_index_details_serializer.py
src/django/api/serializers/facility/facility_index_serializer.py
src/django/api/tests/test_facility_history_endpoint.py
src/django/api/tests/test_facility_index_details_serializer.py
src/django/api/tests/test_wage_indicator_provider.py
src/react/src/components/PartnerFields/PartnerFieldSchemaValue.jsx

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

src/django/api/tests/test_facility_history_endpoint.py
src/react/src/components/PartnerFields/PartnerFieldSchemaValue.jsx
src/django/api/models/wage_indicator_country_data.py
src/django/api/tests/test_facility_index_details_serializer.py

🧰 Additional context used

🧠 Learnings (8)

📚 Learning: 2024-11-28T11:29:28.139Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py:23-24
Timestamp: 2024-11-28T11:29:28.139Z
Learning: The files `src/django/api/tests/test_facility_claim_view_set.py`, `src/django/api/views/facility/facilities_view_set.py`, and `src/django/api/facility_actions/processing_facility_api.py` are not part of the V1 codebase in the Open Supply Hub project.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2024-11-28T11:40:54.281Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/tests/test_production_locations_view_set.py:45-46
Timestamp: 2024-11-28T11:40:54.281Z
Learning: Files `src/django/api/views/facility/facilities_view_set.py`, `src/django/api/facility_actions/processing_facility_api.py`, and `src/django/api/tests/test_facility_claim_view_set.py` are not part of v1 and should not be considered in v1-related code reviews.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: The `contributors` parameter in `src/django/api/serializers/facility/facility_query_params_serializer.py` is defined as `ListField(child=IntegerField(required=False))`, which automatically validates and converts contributor IDs to integers before they reach utility functions, eliminating the need for defensive parsing in downstream code.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py
src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-09-03T06:24:20.622Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 709
File: src/django/api/facilities_download_view_set.py:48-51
Timestamp: 2025-09-03T06:24:20.622Z
Learning: In src/django/api/facilities_download_view_set.py, the base_qs returned by FacilitiesDownloadService.get_filtered_queryset(request) is a list, not a Django QuerySet, so DB-level optimization methods like exists() and exclude() cannot be used on it.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2024-11-22T06:53:05.768Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 415
File: src/django/api/tests/test_facility_list_create.py:269-277
Timestamp: 2024-11-22T06:53:05.768Z
Learning: Prefer solutions that avoid using regular expressions when simpler alternatives are available to enhance code readability and maintainability, especially in test cases like `src/django/api/tests/test_facility_list_create.py`.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: In the Open Supply Hub codebase, the CONTRIBUTORS query parameter validation is handled in `src/django/api/serializers/facility/facility_query_params_serializer.py`, so additional validation for contributor ID parsing is not needed in utility functions like `is_same_contributor_from_url_param` in `src/django/api/serializers/facility/utils.py`.

Applied to files:

src/django/api/serializers/facility/facility_index_serializer.py
src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-04-11T05:31:54.895Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 578
File: src/django/api/serializers/facility/facility_create_claim_serializer.py:20-26
Timestamp: 2025-04-11T05:31:54.895Z
Learning: In the Open Supply Hub project, the `validate_workers` function in facility claim serializers intentionally returns `None` for invalid worker counts rather than raising validation errors, as this behavior aligns with the project's business logic.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

📚 Learning: 2025-04-22T20:16:21.889Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 598
File: src/django/api/serializers/v1/additional_identifiers_serializer.py:0-0
Timestamp: 2025-04-22T20:16:21.889Z
Learning: The validation logic for AdditionalIdentifiersSerializer will be implemented in a future PR. The current implementation is intentionally a placeholder.

Applied to files:

src/django/api/partner_fields/wage_indicator_provider.py

🧬 Code graph analysis (7)

src/django/api/admin.py (4)

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

WageIndicatorCountryData (10-123)

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

WageIndicatorLinkTextConfig (6-46)

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

PartnerField (14-143)

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

get_all_including_inactive (11-13)

get_queryset (7-9)

src/django/api/partner_fields/base_provider.py (3)

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

PartnerField (14-143)

src/django/api/partner_fields/wage_indicator_provider.py (3)

_fetch_raw_data (22-37)

_get_field_name (18-20)

_format_data (39-67)

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

get_all_including_inactive (11-13)

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

src/django/api/models/partner_field.py (3)

PartnerField (14-143)

save (117-137)

delete (139-143)

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

WageIndicatorCountryData (10-123)

src/django/api/partner_fields/wage_indicator_provider.py (4)

WageIndicatorProvider (12-67)

_get_field_name (18-20)

_fetch_raw_data (22-37)

_format_data (39-67)

src/django/api/serializers/facility/facility_index_serializer.py (2)

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

is_created_at_main_date (44-52)

get_efs_associated_with_contributor (148-155)

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

extended_fields (148-192)

src/django/api/partner_fields/wage_indicator_provider.py (2)

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

WageIndicatorCountryData (10-123)

get_links_with_text (67-109)

src/django/api/partner_fields/base_provider.py (4)

SystemPartnerFieldProvider (12-108)

_get_field_name (53-58)

_fetch_raw_data (61-63)

_format_data (66-70)

src/django/api/partner_fields/registry.py (2)

src/django/api/partner_fields/base_provider.py (1)

SystemPartnerFieldProvider (12-108)

src/django/api/partner_fields/wage_indicator_provider.py (1)

WageIndicatorProvider (12-67)

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

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

PartnerFieldManager (4-13)

get_all_including_inactive (11-13)

🪛 Ruff (0.14.10)

src/django/api/admin.py

386-386: Unused method argument: request

(ARG002)

src/django/api/partner_fields/base_provider.py

84-87: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

97-99: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

src/django/api/serializers/facility/facility_index_details_serializer.py

436-436: Do not catch blind exception: Exception

(BLE001)

437-440: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

⏰ 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-countries-code-quality
GitHub Check: run-flake8-linter
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: get-base-branch-dd-cov
GitHub Check: run-integration-test-code-quality
GitHub Check: get-base-branch-django-cov
GitHub Check: get-base-branch-fe-cov
GitHub Check: run-django-code-quality
GitHub Check: get-base-branch-countries-cov
GitHub Check: run-fe-code-quality
GitHub Check: run-contricleaner-code-quality
GitHub Check: run-dd-code-quality
GitHub Check: get-base-branch-contricleaner-cov

🔇 Additional comments (20)

src/django/api/partner_fields/registry.py (1)

1-28: LGTM! Clean registry implementation.

The registry pattern is well-implemented with clear separation of concerns. The design allows easy extension by adding new providers to the __register_providers method.

src/django/api/partner_fields/base_provider.py (2)

20-50: LGTM! Well-structured orchestration method.

The fetch_data method clearly orchestrates the workflow with appropriate error handling and logging.

52-70: LGTM! Clear abstract interface.

The abstract methods define a clean interface for concrete providers to implement.

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

1-219: LGTM! Comprehensive test coverage.

The test suite thoroughly covers the wage indicator provider functionality including happy paths, error scenarios, and edge cases like inactive fields and custom link text configuration.

src/django/api/partner_fields/wage_indicator_provider.py (1)

1-67: LGTM! Clean provider implementation.

The wage indicator provider correctly implements the abstract base class interface. The formatting logic properly transforms wage indicator data into the expected partner field structure.

src/django/api/models/partner_field.py (3)

88-107: LGTM! Well-defined fields with clear semantics.

The active and system_field boolean fields are properly defined with descriptive help text. The custom manager integration is correct.

112-115: LGTM! Clean cache invalidation helper.

The __invalidate_cache method is focused and reusable.

117-143: LGTM! Proper cache invalidation logic.

The save and delete methods correctly invalidate the cache when needed. The warning log in the DoesNotExist exception handler provides visibility as suggested in past reviews.
src/django/api/serializers/facility/facility_index_serializer.py (4)
413-439: LGTM! Clean helper method extraction.

The private helper methods improve code organization and reusability.

441-464: LGTM! Contributor filtering logic is correct.

The method correctly filters extended fields based on contributor ID in embed mode.

244-289: LGTM! Field processing and sorting logic is correct.

The field-specific processing for NAME and ADDRESS fields, along with the default sorting behavior, is properly implemented using the new helper methods.

213-224: Call facility.extended_fields() with parentheses to invoke the method.

Line 222 passes facility.extended_fields without parentheses, but extended_fields is a method on the Facility model, not a property. Without (), this passes a bound method object instead of the QuerySet it should return. The downstream code at line 233 iterates over the result and calls .get('field_name') on each element, which will fail because a method object cannot be iterated. Change line 222 to:
fields = self._filter_contributor_extended_fields(
    facility.extended_fields(),
    request
)
⛔ Skipped due to learnings
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: The `contributors` parameter in `src/django/api/serializers/facility/facility_query_params_serializer.py` is defined as `ListField(child=IntegerField(required=False))`, which automatically validates and converts contributor IDs to integers before they reach utility functions, eliminating the need for defensive parsing in downstream code.
src/django/api/admin.py (4)

330-363: LGTM! Proper admin configuration for partner fields.

The admin enhancements correctly display all fields (including inactive ones), prevent deletion of system fields, and provide clear user feedback.

378-394: LGTM! Well-configured admin for wage indicator country data.

The admin class properly configures the fields and makes country_code readonly when editing (but editable when creating new entries). The unused request parameter flagged by static analysis is required by Django's admin API.

396-399: LGTM! Simple and appropriate admin configuration.

The admin class for link text configuration is properly set up.

425-428: LGTM! Admin classes properly registered.

The wage indicator admin classes are correctly registered with the admin site.

src/django/api/serializers/facility/facility_index_details_serializer.py (4)

358-387: LGTM! Partner fields integration is well-structured.

The method properly merges system-generated partner fields with facility extended fields, filters by contributor if needed, and groups by field name before serialization.

445-462: LGTM! Clean integration with system partner field registry.

The method correctly iterates through registered providers and aggregates their field data.

464-473: LGTM! Clean field grouping logic.

The method efficiently groups fields by name using defaultdict.

475-488: LGTM! Proper caching implementation.

The method correctly implements caching with appropriate TTL for partner field definitions.

src/django/api/admin.py

src/django/api/partner_fields/base_provider.py

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/django/api/partner_fields/base_provider.py (1)
79-88: Consider using logging.exception in the except block.

When logging errors within an exception handler, logging.exception automatically includes the traceback, which aids debugging.
🔎 Proposed refinement
         except PartnerField.DoesNotExist:
-            logger.error(
+            logger.exception(
                 f"Partner field '{field_name}' not found. "
                 "System field must exist in database."
             )
             return None

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57f221a and 8e87a43.

📒 Files selected for processing (1)

src/django/api/partner_fields/base_provider.py

🧰 Additional context used

🧬 Code graph analysis (1)

src/django/api/partner_fields/base_provider.py (3)

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

PartnerField (14-143)

src/django/api/partner_fields/wage_indicator_provider.py (3)

_fetch_raw_data (22-39)

_get_field_name (18-20)

_format_data (41-71)

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

get_all_including_inactive (11-13)

🪛 Ruff (0.14.10)

src/django/api/partner_fields/base_provider.py

84-87: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

⏰ 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-dd-code-quality
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: get-base-branch-dd-cov
GitHub Check: run-integration-test-code-quality
GitHub Check: run-contricleaner-code-quality
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: run-fe-code-quality
GitHub Check: get-base-branch-countries-cov
GitHub Check: run-django-code-quality
GitHub Check: get-base-branch-django-cov
GitHub Check: get-base-branch-fe-cov
GitHub Check: run-countries-code-quality

src/django/api/partner_fields/base_provider.py

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/django/api/partner_fields/base_provider.py (1)

89-99: Consider warning when multiple contributors exist for system fields.

The code correctly handles the no-contributor case, but silently selects the first contributor (by ID) when multiple exist. For system partner fields, there should typically be exactly one contributor. While the deterministic ordering prevents inconsistency, a warning would help surface potential data integrity issues during development or operations.

🔎 Optional enhancement to detect multiple contributors

-        contributor = (
-            Contributor.objects.filter(partner_fields=partner_field)
-            .order_by("id")
-            .first()
-        )
-
-        if not contributor:
+        contributors = (
+            Contributor.objects.filter(partner_fields=partner_field)
+            .order_by("id")[:2]  # Fetch at most 2 to detect multiples
+        )
+        
+        if not contributors:
             logger.warning(
                 f"No contributor found for '{field_name}' partner field.",
             )
             return None
+        
+        if len(contributors) > 1:
+            logger.warning(
+                f"Multiple contributors found for system field '{field_name}'. "
+                f"Using first contributor (ID: {contributors[0].id})."
+            )
+        
+        contributor = contributors[0]

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e87a43 and 024651d.

📒 Files selected for processing (2)

src/django/api/partner_fields/base_provider.py
src/django/api/serializers/facility/facility_index_details_serializer.py

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: The `contributors` parameter in `src/django/api/serializers/facility/facility_query_params_serializer.py` is defined as `ListField(child=IntegerField(required=False))`, which automatically validates and converts contributor IDs to integers before they reach utility functions, eliminating the need for defensive parsing in downstream code.

Applied to files:

src/django/api/partner_fields/base_provider.py

📚 Learning: 2025-09-15T08:21:08.345Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 746
File: src/django/api/serializers/facility/utils.py:369-374
Timestamp: 2025-09-15T08:21:08.345Z
Learning: In the Open Supply Hub codebase, the CONTRIBUTORS query parameter validation is handled in `src/django/api/serializers/facility/facility_query_params_serializer.py`, so additional validation for contributor ID parsing is not needed in utility functions like `is_same_contributor_from_url_param` in `src/django/api/serializers/facility/utils.py`.

Applied to files:

src/django/api/partner_fields/base_provider.py

🧬 Code graph analysis (1)

src/django/api/partner_fields/base_provider.py (2)

src/django/api/partner_fields/wage_indicator_provider.py (3)

_fetch_raw_data (22-39)

_get_field_name (18-20)

_format_data (41-71)

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

get_all_including_inactive (11-13)

🪛 Ruff (0.14.10)

src/django/api/serializers/facility/facility_index_details_serializer.py

436-436: Do not catch blind exception: Exception

(BLE001)

⏰ 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-integration-test-code-quality
GitHub Check: run-eslint-linter-and-prettier-formatter
GitHub Check: get-base-branch-countries-cov
GitHub Check: get-base-branch-dd-cov
GitHub Check: run-dd-code-quality
GitHub Check: get-base-branch-django-cov
GitHub Check: run-django-code-quality
GitHub Check: run-countries-code-quality
GitHub Check: run-contricleaner-code-quality
GitHub Check: get-base-branch-fe-cov
GitHub Check: get-base-branch-contricleaner-cov
GitHub Check: run-fe-code-quality

🔇 Additional comments (9)

src/django/api/partner_fields/base_provider.py (3)

1-18: LGTM! Clean imports and clear abstraction.

The abstract base class is well-documented and imports are appropriate for the provider pattern.

20-50: LGTM! Well-structured orchestration.

The fetch_data method cleanly orchestrates the data retrieval workflow with appropriate logging for missing data scenarios.

52-70: LGTM! Clear abstract interface.

The abstract methods define a clean contract for subclasses with appropriate type hints and documentation.

src/django/api/serializers/facility/facility_index_details_serializer.py (6)

1-34: LGTM! Appropriate imports for partner field support.

The new imports are all relevant to the partner field functionality and are organized appropriately.

358-387: LGTM! Clean orchestration of partner field retrieval and merging.

The method effectively combines system-generated and user-contributed partner fields with clear delegation to helper methods.

414-441: Broad exception handling is appropriate here for resilience.

While static analysis flags the broad Exception catch at line 436, this pattern is appropriate in this context because:

Each partner field is serialized independently

A failure in one field shouldn't break the entire facility response

The error is logged with the specific field name for debugging

Graceful degradation returns an empty list for the failed field

This is a deliberate design choice for non-critical display data.

445-462: LGTM! Clean provider registry integration.

The method cleanly integrates with the provider registry pattern to fetch all system-generated partner fields.

464-473: LGTM! Idiomatic field grouping.

Clean use of defaultdict to group fields by name with appropriate null checks.

475-488: LGTM! Effective caching for partner field definitions.

The caching implementation is sound with an appropriate 60-second TTL for reference data. The queryset is properly evaluated to a list before caching.

src/django/api/serializers/facility/facility_index_details_serializer.py

…OSDEV-2305-upload-the-wageindicator-data-into-the-platform

sonarqubecloud · 2025-12-26T16:02:17Z

Quality Gate passed

Issues
6 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

Implemented basic wage indicator mapping logic

b91f2a0

vlad-shapik self-assigned this Dec 15, 2025

vlad-shapik temporarily deployed to Quality Environment December 15, 2025 14:39 — with GitHub Actions Inactive

vlad-shapik had a problem deploying to Quality Environment December 15, 2025 14:39 — with GitHub Actions Failure

vlad-shapik temporarily deployed to Quality Environment December 15, 2025 14:39 — with GitHub Actions Inactive

vlad-shapik marked this pull request as draft December 15, 2025 14:42

vlad-shapik temporarily deployed to Quality Environment December 15, 2025 14:42 — with GitHub Actions Inactive

vlad-shapik had a problem deploying to Quality Environment December 15, 2025 14:42 — with GitHub Actions Failure

vlad-shapik temporarily deployed to Quality Environment December 15, 2025 14:42 — with GitHub Actions Inactive

vladsha-dev added 10 commits December 18, 2025 12:08

Handled coderabbit comments

c479427

Updated release notes and naming

ebe486c

Fixed tests

99b2838

Fixed tests

d989054

Fixed tests

7083459

Added prints to debug

5d5110d

Added more prints

6bddcd9

Fixed tests

fce929c

Fixed tests

1430956

Updated release notes

72d7691

coderabbitai bot reviewed Dec 18, 2025

View reviewed changes

src/django/api/serializers/facility/facility_index_serializer.py Show resolved Hide resolved

protsack-stephan reviewed Dec 19, 2025

View reviewed changes

protsack-stephan added 2 commits December 26, 2025 11:49

Merge main

f6ea4aa

Change labels in the Django admin

133a4b5

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

src/django/api/migrations/0192_create_wage_indicator_models.py Show resolved Hide resolved

Cleanup admin.py file from unneeded comments

c9d716b

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

Refacot the partner_field.py to be more readable

a7d3419

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

Fix review comments

7947b5f

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

src/django/api/admin.py Show resolved Hide resolved

src/django/api/partner_fields/base_provider.py Outdated Show resolved Hide resolved

protsack-stephan added 2 commits December 26, 2025 16:21

Fix linting issues

57f221a

Fix the issue with base provider

8e87a43

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

src/django/api/partner_fields/base_provider.py Show resolved Hide resolved

Fix linting issues and change errors to warnings in logs

024651d

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

src/django/api/serializers/facility/facility_index_details_serializer.py Show resolved Hide resolved

Merge branch 'main' of github.com:opensupplyhub/open-supply-hub into …

0134c9d

…OSDEV-2305-upload-the-wageindicator-data-into-the-platform

This was referenced Dec 26, 2025

[OSDEV-2304] Upload MIT Living Wage data to the platform #844

Merged

[OSEDV-814] Upgrade Django and Python #846

Merged

Conversation

vlad-shapik commented Dec 15, 2025 • edited by protsack-stephan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

protsack-stephan left a comment

Choose a reason for hiding this comment

Uh oh!

protsack-stephan Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

protsack-stephan Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

protsack-stephan Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

protsack-stephan Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

protsack-stephan Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 26, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vlad-shapik commented Dec 15, 2025 •

edited by protsack-stephan

Loading