[Backmerge][OSDEV-2260] Update the logic about saving extended fields for the Claim flow

[vlad-shapik]

Removed the unnecessary facility_type checks in the facilities_view_set.py to support proper saving of the data. The function assumed that the value is an array and that caused errors in the creation of extended fields.

On front-end data format has been changed to send array values as a pipe-separated (|) string instead of a comma-separated one. The extended_fields.py file has been updated to correctly parse this new pipe-delimited format, ensuring data is handled correctly.

[barecheck]

React App | Jest test suite - Code coverage report

Total: 37.19%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes modify facility type and production type handling in facility claims. The backend now explicitly passes facility_type during claim creation instead of automatically deriving production types from facility type data. The frontend delimiter for serializing facility types changed from comma-space to pipe character.

Changes

Cohort / File(s)	Change Summary
Backend facility type handling src/django/api/extended_fields.py, src/django/api/views/facility/facilities_view_set.py	Enhanced field_value_capitalized to handle string inputs with "|" delimiter in addition to list inputs; removed auto-derivation logic for production types from facility type data; added explicit facility_type parameter to FacilityClaim creation.
Test expectations src/django/api/tests/test_facility_claim.py	Updated expected value for claim.facility_production_types from ["Cutting", "Sewing", "Manufacturing"] to ["Cutting", "Sewing"].
Frontend serialization src/react/src/util/util.js	Changed array-to-FormData delimiter in appendFacilityType from ", " to "|".

Sequence Diagram

sequenceDiagram
    actor User
    participant Frontend as React Frontend
    participant Backend as Django Backend
    participant DB as Database

    rect rgb(200, 220, 255)
    Note over Frontend,Backend: Old Flow (Auto-Derivation)
    User->>Frontend: Submit facility claim
    Frontend->>Backend: POST facility_type: "Cutting, Sewing, Manufacturing"
    Backend->>Backend: get_facility_and_processing_type()<br/>derives production_types
    Backend->>DB: FacilityClaim.create() with auto-populated<br/>facility_production_types
    end

    rect rgb(220, 255, 220)
    Note over Frontend,Backend: New Flow (Explicit Passing)
    User->>Frontend: Submit facility claim
    Frontend->>Backend: POST facility_type: "Cutting|Sewing"
    Backend->>Backend: extended_fields.field_value_capitalized<br/>splits on "|", capitalizes
    Backend->>DB: FacilityClaim.create() with explicit<br/>facility_type parameter
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Focus areas during review:
  • Verify the string-to-list parsing logic in extended_fields.py correctly handles the new "|" delimiter and trims whitespace
  • Confirm the removal of auto-derivation logic from facilities_view_set.py doesn't break dependent features
  • Check that the test expectation change aligns with the new behavior and data flow
  • Validate that both frontend and backend delimiter usage ("|") are synchronized correctly

Possibly related PRs

• [OSDEV-2206] Expand facility claim process with new fields #771: Directly modifies facility_type and production_type handling in FacilitiesViewSet.claim (adds auto-population logic that this PR removes).
• [Backmerge][OSDEV-2231] Fix location type display in the Django admin panel for claims #808: Modifies FacilityClaim.facility_type field constraints and type handling alongside changes to claim creation.
• [Backmerge][OSDEV-2259] Store Company Phone as facility_phone_number #803: Updates how claim fields (including facility_production_types) are populated on FacilityClaim creation.

Suggested reviewers

• roman-stolar
• VadimKovalenkoSNF

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main objective: updating logic for saving extended fields in the Claim flow, which aligns with the PR's core changes across backend and frontend.
Description check	✅ Passed	The description is directly related to the changeset, explaining the removal of facility_type checks, the format change from comma to pipe-separated values, and updates to extended_fields.py parsing.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch OSDEV-2260-backmerge-update-saving-of-extended-fields

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

35-48: Consider using elif and handling edge cases for empty values.

The current implementation uses two separate if statements. While this works in practice (since a value can't be both a list and a string), using elif would make the intent clearer:

 if isinstance(field_value, list):
     field_value_capitalized = (
         [value.capitalize() for value in field_value]
     )
 
-if isinstance(field_value, str):
+elif isinstance(field_value, str):
     separated_vals = field_value.split("|")
     field_value_capitalized = (
         [value.strip().capitalize() for value in separated_vals]
     )

Additionally, consider filtering out empty values after splitting to avoid creating entries like "||" when the input contains consecutive delimiters or leading/trailing delimiters:

elif isinstance(field_value, str):
    separated_vals = [v.strip() for v in field_value.split("|") if v.strip()]
    field_value_capitalized = [value.capitalize() for value in separated_vals]

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 090f56b and 9beb455.

📒 Files selected for processing (4)

• src/django/api/extended_fields.py (1 hunks)
• src/django/api/tests/test_facility_claim.py (1 hunks)
• src/django/api/views/facility/facilities_view_set.py (1 hunks)
• src/react/src/util/util.js (1 hunks)

🧰 Additional context used

🧠 Learnings (10)

📚 Learning: 2024-12-10T15:07:54.819Z

Learnt from: roman-stolar
Repo: opensupplyhub/open-supply-hub PR: 448
File: src/react/src/util/util.js:1317-1317
Timestamp: 2024-12-10T15:07:54.819Z
Learning: Changes to date handling functions like `formatDate` in `src/react/src/util/util.js` should be made carefully, considering that different parts of the codebase may intentionally use local time instead of UTC, and broad changes to `moment()` usage may not be appropriate without context.

Applied to files:

• src/react/src/util/util.js

📚 Learning: 2024-12-10T07:09:35.641Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 436
File: src/react/src/components/Contribute/ProductionLocationDialog.jsx:33-181
Timestamp: 2024-12-10T07:09:35.641Z
Learning: In the `ProductionLocationDialog.jsx` component, hardcoded facility data is acceptable for now as per the project's requirements.

Applied to files:

• src/react/src/util/util.js

📚 Learning: 2025-01-29T09:00:01.638Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 470
File: src/react/src/util/util.js:1441-1472
Timestamp: 2025-01-29T09:00:01.638Z
Learning: The data passed to parseContribData() function in src/react/src/util/util.js is pre-parsed and validated before reaching the function, so additional type checking is not required.

Applied to files:

• src/react/src/util/util.js

📚 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_claim.py
• src/django/api/views/facility/facilities_view_set.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_claim.py
• src/django/api/views/facility/facilities_view_set.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_claim.py
• src/django/api/views/facility/facilities_view_set.py

📚 Learning: 2024-12-17T13:51:50.519Z

Learnt from: roman-stolar
Repo: opensupplyhub/open-supply-hub PR: 438
File: src/django/api/tests/base_moderation_events_production_location_test.py:278-280
Timestamp: 2024-12-17T13:51:50.519Z
Learning: In the file `src/django/api/tests/base_moderation_events_production_location_test.py`, the method `assert_facilitymatch_creation` correctly uses the parameter name `match_status`, and it should not be changed to `match_class`.

Applied to files:

• src/django/api/tests/test_facility_claim.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/tests/test_facility_claim.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_claim.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_claim.py

🧬 Code graph analysis (2)

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

src/react/src/actions/claimForm.js (1)

• formattedKey (51-51)

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

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

• value (1902-1902)

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

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

🔇 Additional comments (3)

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

1326-1333: LGTM! Delimiter change aligns with backend parsing.

The change from ", " to "|" correctly aligns with the backend parsing logic in src/django/api/extended_fields.py (line 43), which splits on "|". This ensures proper handling of multiple facility type values in the claim flow.

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

966-966: Verification completed; current code is correctly implemented.

Based on verification:

1. facility_type IS properly saved: The facility_type field is defined in FacilityCreateClaimSerializer (line 308 as CharField(required=False, allow_blank=True, max_length=300)) and is correctly passed to FacilityClaim.objects.create() at line 966. The FacilityClaim model has the field defined with proper choices.

2. Test expectations align with behavior: The test's expectation of facility_type=None is correct—it reflects the field's optional nature when not provided in request data.

3. Cannot verify "removed" logic: The git history shows no recent commits modifying facility_type in facilities_view_set.py, and the provided diff only shows additions (line 966), not removals. Without visibility into the removed code mentioned in the PR summary ("Removed unnecessary facility_type checks"), I cannot verify what was actually deleted or if removing previous logic was necessary.

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

392-399: Verify that facility_type is excluded from FacilityClaim creation logic.

After examining the codebase, I found a contradiction:

The view (src/django/api/views/facility/facilities_view_set.py:918) explicitly passes facility_type=validated_data.get("facility_type") to FacilityClaim.objects.create(), which should persist the value to the database. However, the test expects claim.facility_type to be None despite sending "facility_type": "Manufacturing" in the request.

Additionally, the serializer accepts and validates facility_type as a CharField with no exclusion logic, and the model field is defined to allow null=True.

The test correctly verifies other list-type fields (facility_production_types, facility_product_types) are saved as expected, making this facility_type expectation inconsistent with the current view logic.

Clarify whether:

1. The view should exclude facility_type from the create() call (intentional design to use facility_type only for processing, not storage)
2. The test assertion is outdated and facility_type should actually be saved
3. This mismatch indicates a bug in the implementation

[barecheck]

Countries App | Unittest test suite - Code coverage report

Total: 100%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[barecheck]

Contricleaner App | Unittest test suite - Code coverage report

Total: 98.75%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[VadimKovalenkoSNF]

Approved.

[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