[OSDEV-2122][Climate Trace] Allow users to PATCH v1/production-locations without 'name', 'address', 'country'

[VadimKovalenkoSNF]

Fix OSDEV-2122
Enhanced PATCH /api/v1/production-locations/{os_id}/ endpoint validation to enforce required field constraints when coordinates are provided. Implemented automatic backfill of missing required fields (name, address, country) from existing facility data when no required fields are provided in PATCH requests.

We backfill name, address and country fields given these steps:

1. First: check for the last promoted match in FacilitylistItem table by os id.
2. If promoted match doesn't exist, fallback to the latest facility list item by os id.
3. If the previous record doesn't exist, take data from the Facility table by OS ID.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

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

970-999: Tighten assertions: verify missing-field set instead of checking attribute presence.

Assert which required fields are reported missing; avoid result.__dict__ pokes.

-        self.assertIn('errors', result.__dict__)
+        self.assertIsInstance(result.errors, dict)
+        missing = {e.get('field') for e in result.errors.get('errors', [])}
+        self.assertTrue({'name', 'address', 'country'}.issubset(missing))

Optional: patch geocoding and assert it isn’t called to future-proof against pipeline changes.

---

1000-1027: Be explicit about which field is missing.

Since partial required fields should fail, assert that 'country' is flagged.

-        self.assertIn('errors', result.__dict__)
+        self.assertIsInstance(result.errors, dict)
+        fields = {e.get('field') for e in result.errors.get('errors', [])}
+        self.assertIn('country', fields)

---

1058-1088: Assert the exact missing-field set for a clearer contract.

Strengthen to ensure both 'address' and 'country' are reported missing.

-        self.assertIn('errors', result.__dict__)
+        self.assertIsInstance(result.errors, dict)
+        missing = {e.get('field') for e in result.errors.get('errors', [])}
+        self.assertTrue({'address', 'country'}.issubset(missing))

---

1089-1116: Guard against external calls and validate error specifics.

Add a concrete check for the missing 'country' and optionally mock geocoding to prevent accidental network calls if validation order changes.

-        self.assertIn('errors', result.__dict__)
+        self.assertIsInstance(result.errors, dict)
+        fields = {e.get('field') for e in result.errors.get('errors', [])}
+        self.assertIn('country', fields)

---

1149-1190: Make the test factory reusable via parameters to reduce duplication.

Allow overriding name/address/country/sector/location to reuse across tests and replace ad-hoc setups elsewhere.

-    def _create_existing_facility(self):
+    def _create_existing_facility(
+            self,
+            *,
+            name='Existing Facility Name',
+            address='Existing Address',
+            country_code='US',
+            location=None,
+            sector=None):
@@
-        list_item = FacilityListItem.objects.create(
-            name='Existing Facility Name',
-            address='Existing Address',
-            country_code='US',
-            sector=['Existing Sector'],
+        if location is None:
+            location = Point(0, 0)
+        if sector is None:
+            sector = ['Existing Sector']
+        list_item = FacilityListItem.objects.create(
+            name=name,
+            address=address,
+            country_code=country_code,
+            sector=sector,
             row_index=1,
             status=FacilityListItem.CONFIRMED_MATCH,
             source=source
         )
@@
-        facility = Facility.objects.create(
+        facility = Facility.objects.create(
             name=list_item.name,
             address=list_item.address,
             country_code=list_item.country_code,
-            location=Point(0, 0),
+            location=location,
             created_from=list_item
         )

Also consider adding a test for “no required fields and no coords” to exercise the automatic backfill path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf5e11f and 6ce8f57.

📒 Files selected for processing (1)

• src/django/api/tests/test_location_contribution_strategy.py (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#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_location_contribution_strategy.py

📚 Learning: 2024-12-17T15:59:02.112Z

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

Applied to files:

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

🧬 Code graph analysis (1)

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

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

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

• ModerationEvent (10-166)
• RequestType (14-17)
• Source (24-26)

src/django/api/moderation_event_actions/creation/moderation_event_creator.py (1)

• perform_event_creation (13-32)

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

🔇 Additional comments (2)

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

1028-1057: PATCH with coords + all required fields: looks good.

Happy path is covered and asserts are sufficient.

---

1117-1146: PATCH with coords + all required fields + extra fields: LGTM.

Covers the success path well and asserts linkage to the existing facility.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (1)

141-171: Guard backfill against missing OS / facility

• fetch_required_fields(os_id) falls back to Facility.objects.get(id=os_id) and will raise Facility.DoesNotExist for unknown IDs (no exception handling). (src/django/api/services/production_locations_lookup.py:6+)
• __validate_and_backfill_patch_data calls fetch_required_fields(event_dto.os.id) without guarding event_dto.os — AttributeError possible if os is None and/or Facility.DoesNotExist will propagate (src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:157).

Fix: either validate/guard event_dto.os before calling fetch_required_fields, or make fetch_required_fields catch Facility.DoesNotExist and return safe defaults ({'name':'','address':'','country':''}); add tests.

🧹 Nitpick comments (5)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

13-21: Consider consistent error structure with base validator.

The error structure here differs from the parent class's validation errors. The base class directly raises serializers.ValidationError(errors) with a list, while this uses a wrapped structure with field and detail keys.

Consider aligning the error structure:

     def validate(self, data):
         if not data:
-            raise serializers.ValidationError([
-                {
-                    'field': 'non_field_errors',
-                    'detail': 'No fields provided.'
-                }
-            ])
+            raise serializers.ValidationError({'non_field_errors': ['No fields provided.']})
         return super().validate(data)

src/django/api/serializers/v1/production_location_schema_serializer.py (1)

58-65: Consider moving the long error message to a constant.

The error message spans multiple lines and could be extracted to improve readability.

Consider extracting the error message:

+    ABSTRACT_CLASS_ERROR = (
+        'ProductionLocationSchemaSerializer is abstract; '
+        'use a concrete subclass.'
+    )
+
     # Use only subclasses.
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         if self.__class__ is ProductionLocationSchemaSerializer:
-            raise TypeError(
-                'ProductionLocationSchemaSerializer is abstract;'
-                ' use a concrete subclass.'
-            )
+            raise TypeError(self.ABSTRACT_CLASS_ERROR)

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

977-1006: Add assertion for specific error messages in PATCH validation tests.

The test verifies the error structure but doesn't check the specific error messages that should be present when coordinates are provided without required fields.

Add specific assertions for the expected error messages:

         self.assertIn('errors', result.__dict__)
         self.assertIn('detail', result.errors)
         self.assertIn('errors', result.errors)
+        # Verify specific error messages for missing required fields with coordinates
+        error_fields = [e['field'] for e in result.errors['errors']]
+        self.assertIn('name', error_fields)
+        self.assertIn('address', error_fields)
+        self.assertIn('country', error_fields)
+        # Verify the error message mentions coordinates
+        for error in result.errors['errors']:
+            if error['field'] in ['name', 'address', 'country']:
+                self.assertIn('coordinates', error['detail'])

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (2)

191-206: Consider consistent f-string formatting in error messages.

The error message uses redundant f-string prefixes when the field variable is already interpolated.

                     {
                         "field": field,
                         "detail": (
-                            f"Field {field} is required when coordinates "
-                            f"are provided."
+                            f"Field {field} is required when coordinates are provided."
                         ),
                     }

---

208-234: Grammar improvement in the error message construction.

When listing provided fields, consider using proper conjunction formatting for better readability.

-        verb = 'is' if len(provided_fields) == 1 else 'are'
-        provided_list = ', '.join(provided_fields)
+        if len(provided_fields) == 1:
+            verb = 'is'
+            provided_list = provided_fields[0]
+        elif len(provided_fields) == 2:
+            verb = 'are'
+            provided_list = ' and '.join(provided_fields)
+        else:
+            verb = 'are'
+            provided_list = ', '.join(provided_fields[:-1]) + ', and ' + provided_fields[-1]

This would produce more natural English: "when name and address are provided" instead of "when name, address are provided".

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 335591f and 0986538.

⛔ Files ignored due to path filters (4)

• src/django/test_attachment_1-test_contributor_1-20250912070039055.jpg is excluded by !**/*.jpg
• src/django/test_attachment_2-test_contributor_1-20250912070039057.jpeg is excluded by !**/*.jpeg
• src/django/test_attachment_3-test_contributor_1-20250912070039057.png is excluded by !**/*.png
• src/django/test_attachment_4-test_contributor_1-20250912070039058.pdf is excluded by !**/*.pdf

📒 Files selected for processing (6)

• src/django/api/exceptions.py (1 hunks)
• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4 hunks)
• src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_post_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_schema_serializer.py (3 hunks)
• src/django/api/tests/test_location_contribution_strategy.py (5 hunks)

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

• src/django/api/exceptions.py

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#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/serializers/v1/production_location_post_schema_serializer.py
• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py
• src/django/api/tests/test_location_contribution_strategy.py

📚 Learning: 2025-09-09T09:02:07.247Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-02-12T08:10:53.132Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#507
File: src/react/src/components/Contribute/ProductionLocationDetails.jsx:21-25
Timestamp: 2025-02-12T08:10:53.132Z
Learning: In the ProductionLocationDetails component, the OS ID parameter extracted from the URL path does not require validation as it's only used for string comparison with historical OS IDs in the filter operation.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-11-12T11:42:07.768Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-12-17T15:59:02.112Z

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

Applied to files:

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

🧬 Code graph analysis (5)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

• ProductionLocationSchemaSerializer (9-97)
• set_core_required (67-70)
• validate (84-97)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

src/django/api/serializers/v1/production_location_schema_serializer.py (2)

• ProductionLocationSchemaSerializer (9-97)
• set_core_required (67-70)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (6)

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

• MissingRequiredFieldsException (32-35)

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

• ModerationEvent (10-166)
• RequestType (14-17)

src/django/api/services/production_locations_lookup.py (5)

• fetch_required_fields (6-41)
• get_missing_required_fields (48-50)
• has_all_required_fields (53-54)
• has_some_required_fields (57-59)
• is_coordinates_without_all_required_fields (62-63)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• ProductionLocationPatchSchemaSerializer (6-21)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

src/django/api/serializers/v1/number_of_workers_serializer.py (2)

• NumberOfWorkersSerializer (4-69)
• validate (12-26)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• validate (13-21)

src/django/api/serializers/v1/coordinates_serializer.py (1)

• validate (18-40)

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

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (2)

• ProductionLocationDataProcessor (41-361)
• _transform_errors (322-345)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

src/django/api/moderation_event_actions/creation/moderation_event_creator.py (1)

• perform_event_creation (13-32)

🪛 Ruff (0.12.2)

src/django/api/serializers/v1/production_location_schema_serializer.py

62-65: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (12)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

1-10: LGTM!

The serializer correctly extends the base class and enforces required fields for POST requests.

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

9-11: LGTM!

The serializer properly makes core fields optional for PATCH requests.

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

10-11: LGTM!

The abstract pattern and core fields definition provide a clean separation between POST and PATCH serializers.

---

67-70: LGTM!

The method provides a clean way to toggle the required state of core fields for different use cases.

---

72-82: LGTM!

The safer validation with isinstance check before isdigit() prevents potential AttributeErrors.

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

34-35: LGTM!

Correctly imports and uses the new POST serializer for create operations.

---

824-852: LGTM!

Test correctly validates error transformation with the new POST serializer.

---

1155-1196: LGTM!

The helper method properly creates a complete test facility with all required relationships.

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4)

50-59: LGTM!

The PATCH validation and backfill logic is properly placed before ContriCleaner validation, with appropriate error handling.

---

61-67: LGTM!

Good defensive programming by catching potential exceptions from serializer preparation.

---

132-139: LGTM!

Clean serializer selection based on request type.

---

173-189: LGTM!

The validation logic correctly handles different scenarios for missing required fields.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (11)

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

32-35: Make default_code domain-specific and add a short docstring.

Improves error categorization in logs/clients and self-documents intent.

-class MissingRequiredFieldsException(APIException):
-    status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
-    default_detail = 'The request body is invalid.'
-    default_code = 'unprocessable_entity'
+class MissingRequiredFieldsException(APIException):
+    "Raised when PATCH payload lacks required fields after applying v1 rules."
+    status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
+    default_detail = 'The request body is invalid.'
+    default_code = 'missing_required_fields'

src/django/api/serializers/v1/production_location_schema_serializer.py (2)

58-66: Prefer a concise guard; current message trips Ruff (TRY003).

Functionally fine; shorten the message to satisfy lint and keep clarity.

-        if self.__class__ is ProductionLocationSchemaSerializer:
-            raise TypeError(
-                'ProductionLocationSchemaSerializer is abstract;'
-                ' use a concrete subclass.'
-            )
+        if type(self) is ProductionLocationSchemaSerializer:
+            raise TypeError('Abstract base serializer; use a concrete subclass.')

---

72-81: Harden numeric-string check against whitespace.

Catches values like " 123 ".

-            isinstance(data[field_name], str) and
-            data[field_name].isdigit()
+            isinstance(data[field_name], str) and
+            data[field_name].strip().isdigit()

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

1-3: Use DRF’s NON_FIELD_ERRORS_KEY instead of hardcoding.

Avoids mismatch if the key is customized in settings and keeps consistency with your transformer.

-from rest_framework import serializers
+from rest_framework import serializers
+from rest_framework.serializers import NON_FIELD_ERRORS_KEY
@@
-        if not data:
-            raise serializers.ValidationError([
-                {
-                    'field': 'non_field_errors',
-                    'detail': 'No fields provided.'
-                }
-            ])
+        if not data:
+            raise serializers.ValidationError([
+                {
+                    'field': NON_FIELD_ERRORS_KEY,
+                    'detail': 'No fields provided.'
+                }
+            ])

Also applies to: 13-21

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

346-346: Use status constants for 500 assertions.

Improves readability and consistency with other assertions.

-        self.assertEqual(result.status_code, 500)
+        self.assertEqual(result.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)

Also applies to: 412-412

---

534-541: Avoid shadowing built-in name 'list'.

Minor readability fix; prevents accidental confusion in IDEs and reviews.

-        list = FacilityList.objects.create(
+        facility_list = FacilityList.objects.create(
             header='header', file_name='one', name='New List Test'
         )
         source = Source.objects.create(
             source_type=Source.LIST,
-            facility_list=list,
+            facility_list=facility_list,
             contributor=existing_location_contributor
         )

---

977-1196: Add a success test for PATCH with zero core fields (backfill path).

Covers the primary OSDEV-2122 case: no name/address/country and no coordinates → automatic backfill from existing facility. Ensures precedence order is exercised end-to-end.

Example to append near existing PATCH tests:

def test_patch_no_required_fields_triggers_backfill_and_succeeds(self):
    existing_facility = self._create_existing_facility()

    input_data = {
        'source': 'API',
        # only optional fields, forcing backfill of name/address/country
        'sector': ['Updated Sector']
    }

    event_dto = CreateModerationEventDTO(
        contributor=self.contributor,
        raw_data=input_data,
        request_type=ModerationEvent.RequestType.UPDATE.value,
        os=existing_facility
    )
    result = self.moderation_event_creator.perform_event_creation(event_dto)

    self.assertEqual(result.status_code, status.HTTP_202_ACCEPTED)
    self.assertIsNotNone(result.moderation_event)
    self.assertEqual(result.moderation_event.request_type, 'UPDATE')
    self.assertEqual(result.moderation_event.os.id, existing_facility.id)
    # sanity-check backfill landed in cleaned data
    cleaned = result.moderation_event.cleaned_data
    self.assertEqual(cleaned['name'], 'Existing Facility Name')
    self.assertEqual(cleaned['address'], 'Existing Address')
    self.assertEqual(cleaned['country_code'], 'US')

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4)

61-68: Drop redundant try/except around serializer construction.

__prepare_serializer doesn’t raise MissingRequiredFieldsException; errors surface during serializer.is_valid(). Simplify the flow.

Apply:

-        try:
-            serializer = self.__prepare_serializer(cc_ready_data, event_dto)
-        except MissingRequiredFieldsException as e:
-            event_dto.errors = e.detail
-            event_dto.status_code = e.status_code
-            return event_dto
+        serializer = self.__prepare_serializer(cc_ready_data, event_dto)

---

126-129: Tighten the comment.

Small clarity/punctuation tweak.

-        # Save the cleaned data in case of successful
-        # ContriCleaner serialization.
+        # Save cleaned data after successful ContriCleaner processing.

---

141-172: Use a shallow copy instead of deepcopy; add a debug breadcrumb.

We only set top-level fields; deepcopy is unnecessary and allocates more.

-            # Create a deep copy to avoid mutating the original data.
-            backfilled_data = copy.deepcopy(cc_ready_data)
+            # Shallow copy is sufficient; we only set top-level fields.
+            backfilled_data = dict(cc_ready_data)
@@
-            return backfilled_data
+            log.debug(
+                "[API V1 Location Upload] Backfilled core fields for OS id=%s.",
+                getattr(event_dto.os, "id", "unknown"),
+            )
+            return backfilled_data

---

132-140: Make request-type routing explicit; CLAIM does not reach this processor

LocationContribution is only used by ProductionLocations.create/partial_update (request_type is CREATE or UPDATE); facility-claim flows use FacilityClaim viewset and don't route here—make UPDATE explicit and keep PATCH as a safe fallback.

-    def __prepare_serializer(cc_ready_data: Dict,
-                             event_dto: CreateModerationEventDTO):
-        if event_dto.request_type == ModerationEvent.RequestType.CREATE.value:
-            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
-
-        # Handle v1 PATCH requests (backfill already happened earlier).
-        return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+    def __prepare_serializer(cc_ready_data: Dict,
+                             event_dto: CreateModerationEventDTO):
+        rt = event_dto.request_type
+        if rt == ModerationEvent.RequestType.CREATE.value:
+            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
+        if rt == ModerationEvent.RequestType.UPDATE.value:
+            # v1 PATCH (backfill already happened earlier).
+            return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+        # Fallback remains PATCH for backward compatibility; adjust if CLAIM is introduced here.
+        return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 335591f and f6e6ff6.

📒 Files selected for processing (6)

• src/django/api/exceptions.py (1 hunks)
• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4 hunks)
• src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_post_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_schema_serializer.py (3 hunks)
• src/django/api/tests/test_location_contribution_strategy.py (5 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#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_location_contribution_strategy.py
• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-12-17T15:59:02.112Z

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

Applied to files:

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

📚 Learning: 2025-09-09T09:02:07.247Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-02-12T08:10:53.132Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#507
File: src/react/src/components/Contribute/ProductionLocationDetails.jsx:21-25
Timestamp: 2025-02-12T08:10:53.132Z
Learning: In the ProductionLocationDetails component, the OS ID parameter extracted from the URL path does not require validation as it's only used for string comparison with historical OS IDs in the filter operation.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-11-12T11:42:07.768Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

🧬 Code graph analysis (5)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

• ProductionLocationSchemaSerializer (9-97)
• set_core_required (67-70)
• validate (84-97)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

src/django/api/serializers/v1/production_location_schema_serializer.py (2)

• ProductionLocationSchemaSerializer (9-97)
• set_core_required (67-70)

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

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (2)

• ProductionLocationDataProcessor (41-361)
• _transform_errors (322-345)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

src/django/api/moderation_event_actions/creation/moderation_event_creator.py (1)

• perform_event_creation (13-32)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (7)

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

• MissingRequiredFieldsException (32-35)

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

• ModerationEvent (10-166)
• RequestType (14-17)

src/django/api/services/production_locations_lookup.py (5)

• fetch_required_fields (6-41)
• get_missing_required_fields (48-50)
• has_all_required_fields (53-54)
• has_some_required_fields (57-59)
• is_coordinates_without_all_required_fields (62-63)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• ProductionLocationPatchSchemaSerializer (6-21)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

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

• APIV1CommonErrorMessages (237-249)

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

src/django/api/serializers/v1/number_of_workers_serializer.py (2)

• NumberOfWorkersSerializer (4-69)
• validate (12-26)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• validate (13-21)

src/django/api/serializers/v1/coordinates_serializer.py (1)

• validate (18-40)

🪛 Ruff (0.12.2)

src/django/api/serializers/v1/production_location_schema_serializer.py

62-65: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (9)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

5-10: LGTM: explicit POST serializer with required core fields.

Clear separation of POST/PATCH behavior. No issues.

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

34-35: LGTM: swap to ProductionLocationPostSchemaSerializer in tests.

Keeps tests aligned with create-vs-patch split.

---

846-847: LGTM: updated serializer references in error-structure tests.

Matches the new POST serializer requirements as intended.

Also applies to: 866-868, 911-913

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (6)

12-12: Import of API-level 422 exception looks correct.

Consistent with the error shaping below. No issues.

---

21-33: Serializer split and lookup-service imports are appropriate.

Clear separation for POST vs PATCH and the required helpers are scoped here. Good move.

---

50-60: Backfill-before-ContriCleaner is the right order.

Catching MissingRequiredFieldsException and short-circuiting avoids running CC with invalid payloads. Looks good.

---

173-189: Validation guardrails are correct and minimal.

Early returns and targeted raises keep the backfill path exclusive to “no core fields provided.” Good.

---

191-207: 422 payload for coordinates-without-core-fields is well-formed.

Per-field errors with COMMON_REQ_BODY_ERROR are consistent with v1 conventions.

---

209-235: Partial-core-fields 422 message reads cleanly.

Stable field order and singular/plural agreement handled. Nicely done.

[roman-stolar]

minor request changes

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

21-21: Clarify PATCH validation semantics and backfill precedence for OSDEV-2122.

Spell out the 422 error case, empty-payload rejection, and the exact backfill order to reduce ambiguity for API consumers.

-* [OSDEV-2122](https://opensupplyhub.atlassian.net/browse/OSDEV-2122) - Enhanced PATCH `/api/v1/production-locations/{os_id}/` endpoint validation to enforce required field constraints when coordinates are provided. Implemented automatic backfill of missing required fields (name, address, country) from existing facility data when no required fields are provided in PATCH requests.
+* [OSDEV-2122](https://opensupplyhub.atlassian.net/browse/OSDEV-2122) - Enhanced PATCH `/api/v1/production-locations/{os_id}/` endpoint validation:
+  - Returns HTTP 422 when `lat`/`lng` are provided without all required fields (`name`, `address`, `country`).
+  - Empty PATCH bodies are rejected.
+  - When no required fields are provided, the system auto-backfills required fields using this precedence:
+    1) last promoted match in `FacilityListItem` by OS ID; 2) latest `FacilityListItem` by OS ID; 3) `Facility` table by OS ID.

---

90-95: Standardize endpoint paths and trailing slashes in OSDEV-2062 entry.

Use /api/v1/.../ to match the rest of the document and keep slashes consistent.

-* [OSDEV-2062](https://opensupplyhub.atlassian.net/browse/OSDEV-2062) - Updated GET `v1/production-locations` API endpoint to query production locations by claim status. Introduced `claimed_at` response field which is taken from `updated_at` column in the `api_facilityclaim` table. Added these query parameters:
+* [OSDEV-2062](https://opensupplyhub.atlassian.net/browse/OSDEV-2062) - Updated GET `/api/v1/production-locations/` to query production locations by claim status. Introduced `claimed_at` response field (sourced from `api_facilityclaim.updated_at`). Added these query parameters:

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6e6ff6 and 5c29c72.

📒 Files selected for processing (1)

• doc/release/RELEASE-NOTES.md (3 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

🔇 Additional comments (1)

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

146-147: Verify image names/tags and note pinning for OSDEV-2123.

Confirm bitnamilegacy/kafka and bitnamilegacy/zookeeper image names and pinned tags used in infra code; consider adding the exact tags here for auditability.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4)

61-67: Remove unnecessary try/except around serializer preparation.

__prepare_serializer does not raise MissingRequiredFieldsException; the try/except adds noise.

Apply this diff:

-        try:
-            serializer = self.__prepare_serializer(cc_ready_data, event_dto.request_type)
-        except MissingRequiredFieldsException as e:
-            event_dto.errors = e.detail
-            event_dto.status_code = e.status_code
-            return event_dto
+        serializer = self.__prepare_serializer(cc_ready_data, event_dto.request_type)

---

56-59: Add log for PATCH required-fields failures.

We swallow MissingRequiredFieldsException into DTO without a log, which hampers observability.

Apply this diff:

         except MissingRequiredFieldsException as e:
+            log.info(
+                '[API V1 Location Upload] PATCH required-fields error: %s',
+                getattr(e, 'detail', str(e)),
+            )
             event_dto.errors = e.detail
             event_dto.status_code = e.status_code
             return event_dto

---

132-139: Be explicit about request_type mapping (avoid defaulting everything non-CREATE to PATCH).

To reduce accidental misrouting (e.g., CLAIM), branch explicitly and fail fast or log for unexpected types.

Proposed change:

-    def __prepare_serializer(cc_ready_data: Dict, request_type: str):
-        if request_type == ModerationEvent.RequestType.CREATE.value:
-            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
-
-        # Handle v1 PATCH requests (backfill already happened earlier).
-        return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+    def __prepare_serializer(cc_ready_data: Dict, request_type: str):
+        if request_type == ModerationEvent.RequestType.CREATE.value:
+            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
+        if request_type == ModerationEvent.RequestType.UPDATE.value:
+            # Handle v1 PATCH requests (backfill already happened earlier).
+            return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+        log.warning('Unexpected request_type for production location: %s', request_type)
+        return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)

---

126-128: Minor comment/style nits.

• Merge split comments into single lines and end with periods.
• Drop backslashes for line continuations in method calls.

Apply this diff:

@@
-        # Save the cleaned data in case of successful
-        # ContriCleaner serialization.
+        # Save the cleaned data in case of successful ContriCleaner serialization.
@@
-            # Add the required fields directly
-            # (before ContriCleaner processing).
+            # Add the required fields directly (before ContriCleaner processing).
@@
-            ProductionLocationDataProcessor. \
-                __raise_coordinates_validation_error(missing_fields)
+            ProductionLocationDataProcessor.__raise_coordinates_validation_error(missing_fields)
@@
-            ProductionLocationDataProcessor. \
-                __raise_partial_fields_validation_error(
-                    raw_data, missing_fields
-                )
+            ProductionLocationDataProcessor.__raise_partial_fields_validation_error(
+                raw_data, missing_fields
+            )

Also applies to: 158-160, 180-186

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c29c72 and ac545f3.

📒 Files selected for processing (1)

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#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/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-09-09T09:02:07.247Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-02-12T08:10:53.132Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#507
File: src/react/src/components/Contribute/ProductionLocationDetails.jsx:21-25
Timestamp: 2025-02-12T08:10:53.132Z
Learning: In the ProductionLocationDetails component, the OS ID parameter extracted from the URL path does not require validation as it's only used for string comparison with historical OS IDs in the filter operation.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-11-12T11:42:07.768Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

🧬 Code graph analysis (1)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (7)

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

• MissingRequiredFieldsException (32-35)

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

• ModerationEvent (10-166)
• RequestType (14-17)

src/django/api/services/production_locations_lookup.py (5)

• fetch_required_fields (6-41)
• get_missing_required_fields (48-50)
• has_all_required_fields (53-54)
• has_some_required_fields (57-59)
• is_coordinates_without_all_required_fields (62-63)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• ProductionLocationPatchSchemaSerializer (6-21)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

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

• APIV1CommonErrorMessages (241-253)

⏰ 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: get-base-branch-dd-cov
• GitHub Check: get-base-branch-contricleaner-cov
• GitHub Check: run-contricleaner-code-quality
• GitHub Check: get-base-branch-countries-cov
• GitHub Check: run-dd-code-quality
• GitHub Check: run-django-code-quality
• GitHub Check: run-eslint-linter-and-prettier-formatter
• GitHub Check: run-countries-code-quality
• GitHub Check: run-flake8-linter
• 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/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (1)

12-33: LGTM: imports and dependencies are consistent with usage.

New imports (exception, serializers, lookup helpers, RequestType) align with the code below.

[roman-stolar]

LGTM

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

src/django/api/serializers/v1/production_location_schema_serializer.py (1)

72-76: Numeric-only check misses decimals and signs — tighten the predicate.

isdigit() ignores values like "12.5" and "+10". Consider a stricter numeric-like check.

Apply:

-        if (
-            field_name in data and
-            isinstance(data[field_name], str) and
-            data[field_name].isdigit()
-        ):
+        val = data.get(field_name)
+        if isinstance(val, str):
+            s = val.strip()
+            # treat pure numbers (ints/decimals with optional sign) as invalid
+            import re  # safe at function scope
+            if s and (s.isdigit() or re.fullmatch(r'[+-]?\d+(?:\.\d+)?', s)):

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4)

165-171: Avoid repeating the required-field tuple — reuse the serializer’s core_fields.

Reduces drift with schema changes.

Apply:

-            for field in ('name', 'address', 'country'):
+            from api.serializers.v1.production_location_schema_serializer \
+                import ProductionLocationSchemaSerializer
+            for field in ProductionLocationSchemaSerializer.core_fields:

Alternatively, expose a shared REQUIRED_FIELDS constant in production_locations_lookup and import it here and in the serializer.

---

224-239: Minor: DRY the required-fields list here too.

Reuse the same source as above for consistency.

Apply:

-        required_fields = ('name', 'address', 'country')
+        from api.serializers.v1.production_location_schema_serializer \
+            import ProductionLocationSchemaSerializer
+        required_fields = ProductionLocationSchemaSerializer.core_fields

---

56-59: Optional: add a log at warn level when emitting 422s from backfill pre-checks.

Aids support/debug when clients send coords or partial required fields.

Apply:

-            except MissingRequiredFieldsException as e:
-                event_dto.errors = e.detail
-                event_dto.status_code = e.status_code
+            except MissingRequiredFieldsException as e:
+                log.warning('[API V1 Location Upload] PATCH required-fields 422: %s', getattr(e, 'detail', str(e)))
+                event_dto.errors = e.detail
+                event_dto.status_code = e.status_code
                 return event_dto

---

130-137: Guard against unexpected request types in serializer prep.

Repo search shows only ModerationEvent.RequestType.CREATE and UPDATE are passed here; still add an explicit branch/raise so other/future values (e.g. CLAIM) don't fall back to PATCH silently.

File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

     def __prepare_serializer(cc_ready_data: Dict, request_type: str):
-        if request_type == ModerationEvent.RequestType.CREATE.value:
-            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
-
-        # Handle v1 PATCH requests (backfill already happened earlier).
-        return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+        if request_type == ModerationEvent.RequestType.CREATE.value:
+            return ProductionLocationPostSchemaSerializer(data=cc_ready_data)
+        if request_type == ModerationEvent.RequestType.UPDATE.value:
+            # Handle v1 PATCH requests (backfill already happened earlier).
+            return ProductionLocationPatchSchemaSerializer(data=cc_ready_data)
+        raise ValueError(f'Unsupported request_type: {request_type}')

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c9ddbd and 9c79f93.

📒 Files selected for processing (4)

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (4 hunks)
• src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_post_schema_serializer.py (1 hunks)
• src/django/api/serializers/v1/production_location_schema_serializer.py (3 hunks)

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

• src/django/api/serializers/v1/production_location_post_schema_serializer.py
• src/django/api/serializers/v1/production_location_patch_schema_serializer.py

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:27-30
Timestamp: 2024-12-17T15:59:02.112Z
Learning: In tests for the PATCH endpoint `/api/v1/moderation-events/{moderation_id}/production-locations/`, it's acceptable to use POST requests in the tests.

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

Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#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/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-09-09T09:02:07.247Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#728
File: src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py:156-163
Timestamp: 2025-09-09T09:02:07.247Z
Learning: For PATCH requests to `/api/v1/production-locations/{os_id}/`, the OS ID is always present as a URL parameter, so `event_dto.os` should never be None during validation in the ProductionLocationDataProcessor.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2025-02-12T08:10:53.132Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#507
File: src/react/src/components/Contribute/ProductionLocationDetails.jsx:21-25
Timestamp: 2025-02-12T08:10:53.132Z
Learning: In the ProductionLocationDetails component, the OS ID parameter extracted from the URL path does not require validation as it's only used for string comparison with historical OS IDs in the filter operation.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

📚 Learning: 2024-11-12T11:42:07.768Z

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.

Applied to files:

• src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py

🧬 Code graph analysis (2)

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (7)

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

• MissingRequiredFieldsException (32-35)

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

• ModerationEvent (10-166)
• RequestType (14-17)

src/django/api/services/production_locations_lookup.py (5)

• fetch_required_fields (6-41)
• get_missing_required_fields (48-50)
• has_all_required_fields (53-54)
• has_some_required_fields (57-59)
• is_coordinates_without_all_required_fields (62-63)

src/django/api/serializers/v1/production_location_post_schema_serializer.py (1)

• ProductionLocationPostSchemaSerializer (5-10)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• ProductionLocationPatchSchemaSerializer (6-21)

src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)

• CreateModerationEventDTO (12-22)

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

• APIV1CommonErrorMessages (241-253)

src/django/api/serializers/v1/production_location_schema_serializer.py (3)

src/django/api/serializers/v1/number_of_workers_serializer.py (2)

• NumberOfWorkersSerializer (4-69)
• validate (12-26)

src/django/api/serializers/v1/production_location_patch_schema_serializer.py (1)

• validate (13-21)

src/django/api/serializers/v1/coordinates_serializer.py (1)

• validate (18-40)

🪛 Ruff (0.12.2)

src/django/api/serializers/v1/production_location_schema_serializer.py

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

🔇 Additional comments (10)

src/django/api/serializers/v1/production_location_schema_serializer.py (5)

10-11: Core fields centralization — good.

Defining core_fields once reduces drift between POST/PATCH serializers.

---

48-48: One-line nested serializer init — fine.

No behavior change; keeps intent clear.

---

66-70: Protected helper to flip required flags — LGTM.

Keeps POST/PATCH behavior explicit in child serializers.

---

85-95: Validation composition — good.

Per-field error struct matches _transform_errors consumer.

---

57-65: Shorten base-class instantiation error (Ruff TRY003); verify no direct instantiations

Replace with:

-        if self.__class__ is ProductionLocationSchemaSerializer:
-            raise TypeError(
-                'ProductionLocationSchemaSerializer is abstract;'
-                ' use a concrete subclass.'
-            )
+        if self.__class__ is ProductionLocationSchemaSerializer:
+            raise TypeError('Use a concrete subclass.')

Quick grep found only the class declaration at src/django/api/serializers/v1/production_location_schema_serializer.py and no direct constructor calls; confirm there are no dynamic/reflective instantiations.

src/django/api/moderation_event_actions/creation/location_contribution/processors/production_location_data_processor.py (5)

50-60: Backfill/validation before ContriCleaner — correct ordering.

Ensures 422s for coords-without-required-fields and enables safe backfill.

---

61-66: Dynamic serializer selection — good.

Clear split between POST and PATCH paths.

---

144-151: Empty PATCH payload passthrough — good.

Leaves “No fields provided.” to the PATCH serializer as intended.

---

178-193: Required-field error routing — LGTM.

Coordinates-without-required and partial-required cases are covered, mutually exclusive.

---

195-211: 422 payload shape for coords case — good.

Error list per missing field aligns with API v1 format.

[roman-stolar]

LGTM

[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

[vlad-shapik]

LGTM

[roman-stolar]

Approved.