[Backmerge][OSDEV-2285] Adjust multiupload for isic_4 fields

[VadimKovalenkoSNF]

Backmerge of #825

Fix for OSDEV-2285

1. Updated the backfill script, including support for the --os-id parameter alongside --singleisic.
2. Updated multiupload handling for isic_4 fields on both BE and FE.

[barecheck]

React App | Jest test suite - Code coverage report

Total: 37.6%

Your code coverage diff: 0.15% ▴

✅ All code changes are covered

[coderabbitai]

📝 Walkthrough

Walkthrough

Normalize and store multi-entry ISIC‑4 data end-to-end: add an ISIC4 entry serializer and schema field, normalize input into list-valued ExtendedField raw_value (with filtering), refactor backfill to bulk-create normalized rows, update frontend formatting/rendering for grouped multi-entry display, and add tests.

Changes

Cohort / File(s)	Summary
Backend: ISIC normalization & creation src/django/api/extended_fields.py	Normalize isic_4 values: handle None, accept lists, normalize and filter entries, return {'raw_value': [..]}; create_extendedfield now uses normalized result and skips empty normalized values.
Backend: Backfill command refactor src/django/api/management/commands/backfill_isic_4_extended_fields.py	Added CommandError import; added _normalize_isic_entries and _build_extended_field helpers; added --os-id option (requires --singleisic); replaced per-item creation with bulk_create; skip empty/invalid normalized entries and surface skipped counts.
Backend: serializers & schema src/django/api/serializers/v1/isic4_entry_serializer.py, src/django/api/serializers/v1/production_location_schema_serializer.py	New ISIC4EntrySerializer (fields: class/group/section/division with validation); ProductionLocationSchemaSerializer now has isic_4 as an optional ListField of ISIC4EntrySerializer (min_length=1, max_length=15).
Frontend: rendering & formatting src/react/src/components/FacilityDetailsGeneralFields.jsx, src/react/src/util/constants.jsx	Render path for isic_4 now groups entries by contributorKey, shows top group with remaining count and additionalContent; formatting logic updated to handle multiple entries, blank-line separation, and robust null/array handling.
Frontend: utility change src/react/src/util/renderUtils.jsx	renderUniqueListItems signature extended to renderUniqueListItems(fieldValue, fieldName = '', { preserveOrder = false } = {}) to optionally preserve order instead of deduplicating.
Tests src/django/api/tests/test_isic4_entry_serializer.py, src/django/api/tests/test_moderation_events_add_production_location.py, src/django/api/tests/base_moderation_events_production_location_test.py	Added serializer tests, added integration test asserting multi-entry isic_4 preserved as list in ExtendedField, and adjusted base test expectation to use full isic_4 list instead of first element.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant API as API Endpoint
    participant Serializer as ProductionLocationSchemaSerializer
    participant EntryValidator as ISIC4EntrySerializer
    participant Normalizer as get_isic_4_extendedfield_value
    participant DB as Database

    Client->>API: POST production location with isic_4: [entry...]
    API->>Serializer: Validate payload (includes isic_4 list)
    Serializer->>EntryValidator: Validate each ISIC4 entry
    EntryValidator-->>Serializer: Validated entries
    Serializer-->>API: Validated data
    API->>Normalizer: Normalize isic_4 -> filter/normalize entries
    Normalizer-->>API: { raw_value: [normalized entries] } or empty
    alt normalized entries present
        API->>DB: Create ExtendedField (bulk_create when applicable) with raw_value list
        DB-->>API: Created
        API-->>Client: 201 Created
    else no entries
        API-->>Client: 400/skip (depending on flow)
    end

Loading

sequenceDiagram
    participant DB as Database
    participant Frontend as FacilityDetailsGeneralFields
    participant Grouper as Grouping logic
    participant Renderer as Grid Renderer
    participant Browser as User Display

    DB->>Frontend: Fetch ExtendedField with raw_value=[entries...]
    Frontend->>Grouper: Group entries by contributorKey
    Grouper->>Grouper: Determine top group and remainingCount
    Grouper-->>Frontend: Top group + remaining
    Frontend->>Renderer: Format top group lines and additionalContent
    Renderer-->>Frontend: Rendered Grid item + expandable remainder
    Frontend-->>Browser: Display grouped ISIC-4 entries

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review areas needing attention:
  • get_isic_4_extendedfield_value — edge cases for None, non-dict entries, and empty-entry filtering.
  • Backfill command changes — correctness of --os-id enforcement, bulk_create batching, and skipped-count logic.
  • Frontend grouping/rendering — correctness of contributorKey grouping and remaining-count display.
  • Serializer validation — aggregated type-error reporting in ISIC4EntrySerializer.validate.
  • renderUniqueListItems signature change — ensure callers handle new options object.

Possibly related PRs

• [Backmerge][OSDEV-2260] Update the logic about saving extended fields for the Claim flow #814 — overlaps on extended_fields parsing/normalization patterns and ISIC handling.
• [RBA][OSDEV-2246] Create backfill script to insert isic_4 to api_extendedfield table #796 — related backend changes to ISIC‑4 normalization and backfill logic.
• [OSDEV-1549] ContriCleaner does not fully validate the new schema for the v1 API endpoints. #489 — earlier work adding ISIC‑4 handling into ProductionLocationSchemaSerializer.

Suggested reviewers

• protsack-stephan
• roman-stolar
• vlad-shapik

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 clearly summarizes the main change: adjusting multiupload handling for isic_4 fields, which is reflected across backend and frontend changes in the changeset.
Description check	✅ Passed	The description accurately relates to the changeset, referencing the backfill script updates, --os-id parameter support, and multiupload handling changes for isic_4 fields on both backend and frontend.

✨ 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 Backmerge-Fix-multi-isic_4-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 (3)

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

1-73: LGTM! Good test coverage for ISIC-4 serialization.

The tests appropriately cover valid payloads, optional fields, blank string rejection, and multiple entry acceptance.

Consider adding a test for the max_length=15 constraint to verify the boundary is enforced:

def test_max_isic_entries_exceeded(self):
    payload = {
        **self.base_payload,
        'isic_4': [self.valid_isic_entry] * 16,
    }
    serializer = ProductionLocationPostSchemaSerializer(data=payload)
    
    self.assertFalse(serializer.is_valid())
    self.assertIn('isic_4', serializer.errors)

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

222-271: Test name is misleading - consider renaming.

The test method is named test_isic4_multiple_entries_create_multiple_extended_fields, but it asserts isic_fields.count() == 1, verifying that multiple ISIC-4 entries are stored in a single ExtendedField record.

Consider renaming to better reflect the actual behavior:

-    def test_isic4_multiple_entries_create_multiple_extended_fields(self):
+    def test_isic4_multiple_entries_stored_in_single_extended_field(self):

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

120-121: Consider using .at(-1) for last element access.

Per the static analysis hint, modern JavaScript's .at(-1) is more readable than [length - 1] for accessing the last array element.

-                const lastGroup =
-                    groupedContributions[groupedContributions.length - 1];
+                const lastGroup = groupedContributions.at(-1);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between efe731b and 1fd614f.

📒 Files selected for processing (9)

• src/django/api/extended_fields.py (2 hunks)
• src/django/api/management/commands/backfill_isic_4_extended_fields.py (7 hunks)
• src/django/api/serializers/v1/production_location_schema_serializer.py (2 hunks)
• src/django/api/tests/base_moderation_events_production_location_test.py (1 hunks)
• src/django/api/tests/test_isic4_entry_serializer.py (1 hunks)
• src/django/api/tests/test_moderation_events_add_production_location.py (1 hunks)
• src/react/src/components/FacilityDetailsGeneralFields.jsx (1 hunks)
• src/react/src/util/constants.jsx (1 hunks)
• src/react/src/util/renderUtils.jsx (1 hunks)

🧰 Additional context used

🧠 Learnings (7)

📚 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/base_moderation_events_production_location_test.py
• src/django/api/tests/test_isic4_entry_serializer.py
• src/django/api/tests/test_moderation_events_add_production_location.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/base_moderation_events_production_location_test.py
• src/django/api/tests/test_moderation_events_add_production_location.py

📚 Learning: 2024-12-26T09:07:37.571Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 454
File: src/tests/v1/moderator/test_moderation_event_record.py:53-62
Timestamp: 2024-12-26T09:07:37.571Z
Learning: No dynamic creation test is needed in test_moderation_event_record.py because src/tests/v1/test_moderation_events.py assumes responsibility for creating moderation events, and the test relies on a persistent mock environment with a hardcoded moderation ID.

Applied to files:

• src/django/api/tests/base_moderation_events_production_location_test.py
• src/django/api/tests/test_moderation_events_add_production_location.py

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

Learnt from: mazursasha1990
Repo: opensupplyhub/open-supply-hub PR: 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/base_moderation_events_production_location_test.py
• src/django/api/tests/test_moderation_events_add_production_location.py

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

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 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/tests/test_moderation_events_add_production_location.py

📚 Learning: 2025-02-28T08:20:33.579Z

Learnt from: roman-stolar
Repo: opensupplyhub/open-supply-hub PR: 536
File: src/django/api/views/v1/moderation_events.py:195-196
Timestamp: 2025-02-28T08:20:33.579Z
Learning: In the `update_production_location` method of the `ModerationEvents` class, no additional check for event.status is needed before sending the approval email with `send_slc_contribution_approval_email` as the method's flow already ensures appropriate validation.

Applied to files:

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

📚 Learning: 2025-02-12T11:45:01.562Z

Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 511
File: src/react/src/util/util.js:1436-1447
Timestamp: 2025-02-12T11:45:01.562Z
Learning: The `generateRangeField` utility function in `src/react/src/util/util.js` is meant for data transformation only. Input validation is handled separately on the frontend for specific input fields.

Applied to files:

• src/react/src/util/renderUtils.jsx

🧬 Code graph analysis (5)

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

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

• CoordinatesSerializer (4-40)

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

• NumberOfWorkersSerializer (4-69)

src/react/src/util/constants.jsx (1)

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

• value (1902-1902)

src/django/api/management/commands/backfill_isic_4_extended_fields.py (2)

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

• get_isic_4_extendedfield_value (61-80)

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

• ExtendedField (7-113)

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

src/react/src/util/renderUtils.jsx (1)

• values (17-17)

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

• primary (1791-1791)
• value (1902-1902)

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

• FacilityDetailsItem (33-116)

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

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

• login_as_superuser (107-110)

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

• ExtendedField (7-113)

🪛 GitHub Check: SonarCloud Code Analysis

src/react/src/components/FacilityDetailsGeneralFields.jsx

[warning] 121-121: Prefer .at(…) over [….length - index].

See more on https://sonarcloud.io/project/issues?id=opensupplyhub_open-supply-hub&issues=AZrYhGXEn-LOpiqqXQgb&open=AZrYhGXEn-LOpiqqXQgb&pullRequest=829

🪛 Ruff (0.14.6)

src/django/api/management/commands/backfill_isic_4_extended_fields.py

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

🔇 Additional comments (10)

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

58-70: LGTM! Well-structured isic_4 field definition.

The field correctly uses ListField with ISIC4EntrySerializer as child, appropriate constraints (min 1, max 15 entries), and clear custom error messages. The allow_empty=False combined with min_length=1 provides explicit error messaging for different edge cases.

src/django/api/management/commands/backfill_isic_4_extended_fields.py (3)

18-35: LGTM! Clean extraction of helper methods.

The _normalize_isic_entries and _build_extended_field static methods provide good abstraction for reuse across single-item and batch backfill paths. The normalization delegates to get_isic_4_extendedfield_value ensuring consistency with the extended fields module.

---

67-75: LGTM! Proper validation for --os-id argument.

The --os-id argument is correctly restricted to work only with --singleisic, with a clear error message when misused.

Also applies to: 85-90

---

164-166: Verify: Extended field stores all normalized entries in a single record.

The code creates one ExtendedField containing all normalized ISIC-4 entries as a list in raw_value. This aligns with the test expectation in test_isic4_multiple_entries_create_multiple_extended_fields which asserts isic_fields.count() == 1.

Note that the method name test_isic4_multiple_entries_create_multiple_extended_fields is slightly misleading since it actually creates a single ExtendedField with multiple entries, not multiple ExtendedField records.

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

311-317: LGTM! Assertion updated to reflect list-based ISIC-4 storage.

The expected value now correctly expects the entire isic_4 list in raw_value, consistent with the updated get_isic_4_extendedfield_value behavior that normalizes entries into a list structure.

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

61-80: LGTM! Robust normalization for ISIC-4 values.

The function properly handles all edge cases:

• None returns empty list in raw_value
• Single dict is wrapped in a list
• List entries are filtered to remove None, empty strings, empty dicts, non-dict values, and dicts where all values are empty

This ensures consistent list-based storage for ISIC-4 data.

---

152-156: LGTM! Proper early-return for empty normalized values.

The early return when raw_value is empty prevents creation of ExtendedField records with no meaningful data.

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

111-197: LGTM! Multi-entry ISIC-4 rendering with contributor grouping.

The implementation correctly groups ISIC-4 entries by contributor, formats them with separator spacing, and renders the top group with additional groups in a drawer. The logic aligns with the backend changes that now store multiple ISIC-4 entries as a list.

The grouping logic using value_count and remaining counter ensures entries from the same contributor are aggregated together.

src/react/src/util/constants.jsx (1)

1133-1157: LGTM! Well-structured multi-entry ISIC 4 formatting.

The implementation correctly handles multiple ISIC 4 entries with proper normalization, blank line separators, and graceful handling of edge cases (null/undefined values, empty entries, single vs. multiple entries).

src/react/src/util/renderUtils.jsx (1)

8-17: LGTM! Clean backward-compatible extension.

The optional preserveOrder parameter is well-designed, maintaining backward compatibility (default false preserves existing deduplication behavior) while enabling order preservation for multi-entry fields like ISIC 4.

[barecheck]

Dedupe Hub App | Unittest test suite - Code coverage report

Total: 55.73%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

5-41: Clarify required vs optional semantics and clean up unused required messages

All four CharFields are declared with required=False, but you still provide 'required' error messages; DRF will never emit those for optional fields, which is slightly misleading for future maintainers. If these subfields are actually mandatory per entry, they should be required=True (or enforced explicitly in validate). If they’re truly optional, consider dropping the 'required' messages to avoid dead config.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fd614f and d970992.

📒 Files selected for processing (1)

• src/django/api/serializers/v1/isic4_entry_serializer.py (1 hunks)

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

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

🔇 Additional comments (1)

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

5-14: The serializer's input format is intentionally 'class' (not 'isic_class'), as confirmed by all test cases. The validate() method correctly checks initial_data['class'], and errors are appropriately keyed by 'class' which matches the actual input field name. The field naming, while unconventional, is consistent and functional across the codebase.

Likely an incorrect or invalid review comment.

[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

[barecheck]

Django App | Unittest test suite - Code coverage report

Total: 81.51%

Your code coverage diff: -0.01% ▾

✅ All code changes are covered