[OSDEV-2295] Add divider for isic 4 records of the same contribution

[VadimKovalenkoSNF]

Fix OSDEV-2295

Added divider between isic 4 records of same contribution.

[barecheck]

React App | Jest test suite - Code coverage report

Total: 40.2%

Your code coverage diff: 0.09% ▴

✅ All code changes are covered

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a CONTRIBUTION_DATA_DIVIDER token and renders visual dividers between ISIC‑4 blocks: preserves ISIC order, produces stable sequence keys, exposes a showDivider prop on FacilityDetailsItem, updates render utilities/constants, updates tests, and documents the change in release notes. (50 words)

Changes

Cohort / File(s)	Summary
Documentation doc/release/RELEASE-NOTES.md	Added release note documenting the new UI divider between ISIC‑4 records (OSDEV-2295).
Component Props & Rendering src/react/src/components/FacilityDetailsItem.jsx	Added showDivider prop (PropTypes.bool, default false); thread rendering to conditionally render Material‑UI Divider between additionalContent items when showDivider is true; adjusted item key fallback to `item.key
Rendering Utilities src/react/src/util/renderUtils.jsx	Exported CONTRIBUTION_DATA_DIVIDER; refactored renderUniqueListItems signature to (fieldValue, fieldName = '', { preserveOrder = false } = {}); added order-preservation for ISIC (fieldName === 'isic_4'), sequence-based stable keys, and special-case rendering of divider token as a Material‑UI Divider.
Utility Constants src/react/src/util/constants.jsx	Imported and used CONTRIBUTION_DATA_DIVIDER in EXTENDED_FIELD_TYPES for isic_4; simplified reducer callback signature and updated divider insertion to use the divider token when accumulator already has content.
Tests src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js	Updated expectations to descriptive ISIC‑4 labels (Section/Division/Group/Class); added assertions for single-block vs multi-block ISIC rendering and divider presence; adjusted feature-flag related expectations accordingly.
Backend Tests src/django/api/tests/test_source_contributor_reassignment.py	Added new Django test verifying reassigning a Source updates ExtendedField.contributor_id, FacilityIndex entries, and serialized output.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Contribution as "Contribution data"
  participant Constants as "constants.jsx"
  participant RenderUtils as "renderUtils.jsx"
  participant Component as "FacilityDetailsItem.jsx"
  participant UI as "Browser UI"

  Note over Contribution,Constants: map/reduce ISIC‑4 input -> insert divider tokens
  Contribution->>Constants: provide raw `isic_4` entries
  Constants->>RenderUtils: map/reduce -> values (include CONTRIBUTION_DATA_DIVIDER markers)

  Note over RenderUtils,Component: build render nodes with stable sequence keys
  RenderUtils->>Component: return node list (spans/anchors + divider markers, sequence keys)
  Component->>Component: if `showDivider` true -> render divider markers as <Divider/>
  Component->>UI: mount final list (spans/anchors and Divider components)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• [OSDEV-2244] Display ISIC 4 hierarchy fields on production location profiles #812 — modifies ISIC‑4 rendering and EXTENDED_FIELD_TYPES; closely related to divider insertion and label formatting.
• [OSDEV-2185] [Climate TRACE] Display Source By Text #792 — touches FacilityDetailsItem prop and render behavior; related to the new showDivider prop.
• [Backmerge][OSDEV-2285] Adjust multiupload for isic_4 fields #829 — updates frontend ISIC‑4 rendering utilities and tests; directly related to order/divider logic.

Suggested reviewers

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

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 'OSDEV-2295 Add divider for isic 4 records of the same contribution' accurately summarizes the primary change: adding dividers between ISIC-4 records within the same contribution.
Description check	✅ Passed	The description references OSDEV-2295, mentions adding dividers between isic 4 records of same contribution, and is directly related to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

---

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.}

[barecheck]

Dedupe Hub App | Unittest test suite - Code coverage report

Total: 55.73%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[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.61%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[protsack-stephan]

Looks good, one small comment from me.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

33-35: Consider making the field-specific behavior configurable.

The hardcoded fieldName === 'isic_4' check couples the utility to a specific field. While the sentinel token CONTRIBUTION_DATA_DIVIDER is generic, the divider rendering is tied to this field name.

For better reusability, consider adding a fieldNamesWithDividers option to the configuration object, or making divider rendering conditional on the presence of the sentinel token alone (regardless of field name).

This would allow other fields to use dividers in the future without modifying this utility.

🔎 Possible refactor to make divider rendering generic

 const renderUniqueListItems = (
     fieldValue,
     fieldName = '',
-    { preserveOrder = false } = {},
+    { preserveOrder = false, enableDividers = false } = {},
 ) => {
     if (!Array.isArray(fieldValue)) {
         return fieldValue;
     }
 
     if (fieldValue.length > 0 && React.isValidElement(fieldValue[0])) {
         return fieldValue;
     }
 
-    const shouldPreserveOrder = preserveOrder || fieldName === 'isic_4';
+    const shouldPreserveOrder = preserveOrder || enableDividers;
 
     const values = shouldPreserveOrder ? fieldValue : [...new Set(fieldValue)];
 
     let keySeq = 0;
 
     return values.map(value => {
         keySeq += 1;
         const key = `${fieldName}-${keySeq}`;
 
-        if (fieldName === 'isic_4' && value === CONTRIBUTION_DATA_DIVIDER) {
+        if (enableDividers && value === CONTRIBUTION_DATA_DIVIDER) {
             return <Divider key={key} style={dividerStyle} />;
         }

Then callers would pass { enableDividers: true } for fields that need divider support.

---

37-41: Key generation is functional but could be more stable.

The keySeq counter ensures uniqueness, which is essential for duplicate values. However, using map with the index would provide more stable keys across renders:

return values.map((value, index) => {
    const key = `${fieldName}-${index}`;
    // ...
});

This would help React's reconciliation algorithm recognize that the same items are in the same positions across renders, avoiding unnecessary unmounts/remounts.

For the simple components being rendered (spans, dividers), the current approach is acceptable, but stable keys are a best practice.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f56974 and b567b4b.

📒 Files selected for processing (2)

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

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

• src/react/src/util/constants.jsx

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2024-11-08T14:17:38.915Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 397
File: src/react/src/components/Dashboard/DashboardContributionRecord.jsx:25-35
Timestamp: 2024-11-08T14:17:38.915Z
Learning: In `src/react/src/components/Dashboard/DashboardContributionRecord.jsx`, using inline styles with the `List` component is acceptable and functions correctly in this context.

Applied to files:

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

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

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

Applied to files:

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

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

🔇 Additional comments (7)

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

1-2: LGTM! Divider import is correct for Material-UI v3.

The import is appropriate for the project's Material-UI version.

---

4-9: Well-documented sentinel token.

The JSDoc clearly explains the purpose and collision risk. The name CONTRIBUTION_DATA_DIVIDER is more generic than the previous ISIC_DIVIDER, which addresses the concern about making the divider mechanism reusable.

---

16-18: LGTM! Divider styling follows established pattern.

The Object.freeze usage is consistent with blockStyle, and the vertical margin provides appropriate spacing.

---

43-45: LGTM! Divider rendering is correctly implemented.

The conditional check and Divider component rendering are appropriate. The use of the generated key ensures React can track the component properly.

---

47-59: LGTM! Parent company link rendering preserved correctly.

The special handling for parent_company_os_id is maintained with the updated key generation. Security attributes (rel="noopener noreferrer") are appropriately applied.

---

61-65: LGTM! Default value rendering is correct.

The fallback case renders values as spans with appropriate styling and keys.

---

20-24: No breaking change—the third parameter was newly added, not refactored.

The original signature was (fieldValue, fieldName = '') with no third parameter. The new signature adds an optional { preserveOrder = false } = {} parameter. Since this parameter is optional with a default value, all existing call sites (which pass 1–2 arguments) remain compatible. The concern about breaking existing calls is unfounded.

[protsack-stephan]

Good work! One small comment, feel free to ignore.

[roman-stolar]

In general LGTM, please double check @protsack-stephan comment.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

33-35: Implicit isic_4 ordering behavior is intentional but consider documenting it.

The fieldName === 'isic_4' check ensures ISIC-4 fields always preserve order regardless of the preserveOrder option. This is a reasonable default for backward compatibility, but consider adding a brief inline comment explaining why isic_4 is special-cased here for future maintainers.

🔎 Suggested inline comment

-    const shouldPreserveOrder = preserveOrder || fieldName === 'isic_4';
+    // isic_4 always preserves order to maintain contribution groupings with dividers
+    const shouldPreserveOrder = preserveOrder || fieldName === 'isic_4';

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

84-88: Refactor: Use next() instead of list comprehension with slice.

The pattern of building a full list via comprehension and then taking [0] appears three times (lines 84-88, 104-108, 120-122). Using next() with a generator expression is more efficient and Pythonic—it stops after finding the first match rather than scanning all elements.

🔎 Proposed refactor using next()

For lines 84-88:

-        initial_entry = [
-            ef
-            for ef in facility_index.extended_fields
-            if ef["id"] == self.extended_field.id
-        ][0]
+        initial_entry = next(
+            ef
+            for ef in facility_index.extended_fields
+            if ef["id"] == self.extended_field.id
+        )

For lines 104-108:

-        updated_entry = [
-            ef
-            for ef in facility_index.extended_fields
-            if ef["id"] == self.extended_field.id
-        ][0]
+        updated_entry = next(
+            ef
+            for ef in facility_index.extended_fields
+            if ef["id"] == self.extended_field.id
+        )

For lines 120-122:

-        serialized_entry = [
-            ef for ef in serialized_fields if ef["id"] == self.extended_field.id
-        ][0]
+        serialized_entry = next(
+            ef for ef in serialized_fields if ef["id"] == self.extended_field.id
+        )

Based on static analysis hints from Ruff.

Also applies to: 104-108, 120-122

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b567b4b and 820e871.

📒 Files selected for processing (2)

• src/django/api/tests/test_source_contributor_reassignment.py
• src/react/src/util/renderUtils.jsx

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2024-11-08T14:17:38.915Z

Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 397
File: src/react/src/components/Dashboard/DashboardContributionRecord.jsx:25-35
Timestamp: 2024-11-08T14:17:38.915Z
Learning: In `src/react/src/components/Dashboard/DashboardContributionRecord.jsx`, using inline styles with the `List` component is acceptable and functions correctly in this context.

Applied to files:

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

📚 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/renderUtils.jsx

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

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

Applied to files:

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

🧬 Code graph analysis (2)

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

src/react/src/__tests__/utils.tests.js (1)

• value (771-771)

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

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

• FacilityIndex (13-225)

🪛 Ruff (0.14.10)

src/django/api/tests/test_source_contributor_reassignment.py

84-88: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

---

104-108: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

---

120-122: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

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

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

🔇 Additional comments (6)

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

1-9: LGTM! Well-documented sentinel constant.

The JSDoc comment clearly explains the purpose of CONTRIBUTION_DATA_DIVIDER, the collision assumption, and the naming rationale. The renaming from ISIC_DIVIDER to a more generic name aligns with the feedback to make it reusable for other contribution data types.

---

20-24: Good API evolution using options object.

Changing from a positional preserveOrder parameter to an options object { preserveOrder = false } improves extensibility for future options without breaking existing callers.

---

37-66: Sequential keys correctly handle duplicate values.

Using keySeq ensures unique keys even when preserveOrder is true and duplicates exist in the array. This is necessary since value-based keys would fail with duplicates. The approach is deterministic and works well for this use case.

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

1-127: Clarify: File appears unrelated to PR objectives.

The PR objectives describe adding visual dividers for ISIC-4 records, but this test file validates source contributor reassignment and extended field attribution propagation. There's no apparent connection between the two.

Please verify whether this file was intended for this PR or if there's been a mismatch in the file selection.

---

19-81: LGTM: Comprehensive test fixture.

The setUp method creates a well-structured test fixture with properly linked objects covering the entire data graph needed for the reassignment test.

---

82-127: LGTM: Thorough verification of propagation.

The test systematically verifies that source contributor reassignment cascades correctly through:

1. The ExtendedField model instance
2. The FacilityIndex.extended_fields JSON array
3. The serialized FacilityIndexDetailsSerializer output

This multi-layer validation ensures data consistency across the stack.

[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

[roman-stolar]

Re-Approved