[OSDEV-2199] [Climate TRACE] Display partner field "label" and "unit" in Production Location Page#807
Conversation
…ion Page + added simple tests fro unit value UI
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 37.18%Your code coverage diff: 0.09% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 81.51%Your code coverage diff: 0.01% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThis PR extends the PartnerField model with a new RichTextField Changes
Sequence Diagram(s)sequenceDiagram
participant DB as Database
participant FS as FacilityIndexSerializer
participant EC as ExtendedFieldListSerializer
participant FD as FacilityDetailsDetail
participant Client as React Client
DB->>FS: Query PartnerField objects with source_by, unit, label
FS->>FS: Build context: {source_by, unit, label} from PartnerField
FS->>EC: Pass partner_fields list + context overrides
EC->>EC: Map fields to serializers or context values
EC->>FS: Return serialized partner fields
FS->>Client: Send JSON with sourceBy, unit, label
Client->>FD: Render with sourceBy (HTML), unit text
FD->>FD: Render sourceBy via dangerouslySetInnerHTML
FD->>FD: Render unit inline with primary value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.4)src/django/api/migrations/0185_add_source_by_to_partner_field.py9-11: Mutable class attributes should be annotated with (RUF012) 13-19: Mutable class attributes should be annotated with (RUF012) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/react/src/components/FacilityDetailsGeneralFields.jsx (2)
151-159: Use fieldName in React key to avoid collisions.If two partner fields share the same display label, keys collide. Prefer fieldName for uniqueness.
Apply:
- <Grid item xs={12} md={6} key={`partner-${label}`}> + <Grid item xs={12} md={6} key={`partner-${fieldName}`}>
231-233: Hardcoded "Name" label — consider i18n.If localization is planned, route this through your i18n/strings source.
Confirm whether this should use a translated string.
doc/release/RELEASE-NOTES.md (2)
6-26: Add static assets note for CKEditor.Since django-ckeditor assets are new, confirm your deploy runs collectstatic or otherwise ships static files.
Does your pipeline already run collectstatic for Django static apps?
21-25: Fix markdown list indentation (MD007).Indent sub-bullets by two spaces to satisfy markdownlint.
Apply:
- * `migrate` - * `reindex_database` + * `migrate` + * `reindex_database`src/django/oar/settings.py (1)
505-521: Tighten CKEditor toolbar (remove "Source").Exposing Source increases risk of unsafe markup. Since rendering uses dangerouslySetInnerHTML, prefer removing it unless strictly needed.
Apply:
- ['RemoveFormat', 'Source'], + ['RemoveFormat'],Server-side sanitization is expected; confirm bleach cleaning is applied for PartnerField.source_by.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/admin.py(3 hunks)src/django/api/constants.py(1 hunks)src/django/api/migrations/0184_add_source_by_to_partner_field.py(1 hunks)src/django/api/models/partner_field.py(3 hunks)src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py(2 hunks)src/django/api/serializers/facility/facility_index_serializer.py(5 hunks)src/django/api/tests/test_facility_index_serializer.py(1 hunks)src/django/oar/settings.py(2 hunks)src/django/requirements.txt(1 hunks)src/react/src/__tests__/components/FacilityDetailsDetail.test.js(1 hunks)src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js(1 hunks)src/react/src/components/FacilityDetailsDetail.jsx(3 hunks)src/react/src/components/FacilityDetailsGeneralFields.jsx(2 hunks)src/react/src/components/FacilityDetailsItem.jsx(3 hunks)src/react/src/components/FacilityDetailsLocationFields.jsx(1 hunks)src/react/src/util/util.js(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2024-12-03T10:29:02.409Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 436
File: src/react/src/components/DialogTooltip.jsx:0-0
Timestamp: 2024-12-03T10:29:02.409Z
Learning: In the `DialogTooltip` component (`src/react/src/components/DialogTooltip.jsx`), there are existing tooltip tests that cover common cases, and further detailed tests are not necessary.
Applied to files:
src/react/src/__tests__/components/FacilityDetailsDetail.test.js
📚 Learning: 2025-01-17T16:12:18.285Z
Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 483
File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:0-0
Timestamp: 2025-01-17T16:12:18.285Z
Learning: In the SearchByNameAndAddressSuccessResult component's tests, attempting to test internal navigation logic through mocking useHistory is not feasible, and button presence/click events should be covered in the main rendering test case.
Applied to files:
src/react/src/__tests__/components/FacilityDetailsDetail.test.jssrc/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js
📚 Learning: 2025-06-02T13:24:57.659Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 641
File: doc/release/RELEASE-NOTES.md:6-35
Timestamp: 2025-06-02T13:24:57.659Z
Learning: The Open Supply Hub team keeps placeholders in release notes until code freeze, then fills in the actual content once all changes are finalized for the release.
Applied to files:
doc/release/RELEASE-NOTES.md
📚 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/components/FacilityDetailsGeneralFields.jsxsrc/react/src/components/FacilityDetailsItem.jsxsrc/react/src/components/FacilityDetailsDetail.jsxsrc/react/src/components/FacilityDetailsLocationFields.jsx
📚 Learning: 2024-12-05T10:43:59.922Z
Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 437
File: src/react/src/components/Filters/StyledSelect.jsx:31-39
Timestamp: 2024-12-05T10:43:59.922Z
Learning: In the `StyledSelect` component (`src/react/src/components/Filters/StyledSelect.jsx`), the `label` prop is optional. The `InputLabel` should be conditionally rendered only when a label is provided to prevent unnecessary DOM elements, extra padding, and browser console warnings.
Applied to files:
src/react/src/components/FacilityDetailsGeneralFields.jsxsrc/react/src/components/FacilityDetailsDetail.jsxsrc/react/src/components/FacilityDetailsLocationFields.jsx
📚 Learning: 2024-11-28T11:29:28.139Z
Learnt from: Innavin369
Repo: opensupplyhub/open-supply-hub PR: 431
File: src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py:23-24
Timestamp: 2024-11-28T11:29:28.139Z
Learning: The files `src/django/api/tests/test_facility_claim_view_set.py`, `src/django/api/views/facility/facilities_view_set.py`, and `src/django/api/facility_actions/processing_facility_api.py` are not part of the V1 codebase in the Open Supply Hub project.
Applied to files:
src/django/api/tests/test_facility_index_serializer.py
📚 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/components/FacilityDetailsDetail.jsxsrc/react/src/util/util.js
📚 Learning: 2024-12-10T09:00:29.628Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 436
File: src/react/src/util/styles.js:567-571
Timestamp: 2024-12-10T09:00:29.628Z
Learning: In the Open Supply Hub project, when defining font sizes in styles (e.g., in `src/react/src/util/styles.js`), prefer using `px` units directly rather than using `theme.typography.pxToRem()` function, as `px` units are used consistently throughout the project.
Applied to files:
src/react/src/components/FacilityDetailsDetail.jsx
📚 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/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
🧬 Code graph analysis (6)
src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
src/react/src/components/FacilityDetailsDetail.jsx (1)
FacilityDetailsDetail(60-104)
src/django/api/admin.py (1)
src/django/api/models/partner_field.py (2)
Meta(24-25)PartnerField(8-76)
src/django/api/tests/test_facility_index_serializer.py (3)
src/django/api/models/partner_field.py (1)
save(69-71)src/django/api/models/extended_field.py (1)
ExtendedField(7-111)src/django/api/serializers/facility/facility_index_serializer.py (1)
FacilityIndexSerializer(47-570)
src/react/src/components/FacilityDetailsDetail.jsx (1)
src/react/src/util/util.js (1)
primary(1791-1791)
src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py (1)
src/react/src/util/util.js (1)
fields(1618-1618)
src/django/api/serializers/facility/facility_index_serializer.py (1)
src/django/api/models/partner_field.py (1)
PartnerField(8-76)
🪛 Biome (2.1.2)
src/react/src/components/FacilityDetailsDetail.jsx
[error] 94-94: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.4)
src/django/api/migrations/0184_add_source_by_to_partner_field.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-19: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/django/api/admin.py
255-255: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: get-base-branch-dd-cov
- 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-contricleaner-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (8)
src/react/src/components/FacilityDetailsLocationFields.jsx (1)
75-85: Label precedence correctly ensures "Address" displays for the primary field while preserving partner field labels.The change places
label="Address"after{...addressField}to ensure the primary address always displays "Address" as its label. Partner address fields inotherAddressesare rendered separately in the drawer (line 109 of FacilityDetailsItem.jsx:<FacilityDetailsDetail {...item} />), preserving their custom labels through prop spreading.src/react/src/components/FacilityDetailsGeneralFields.jsx (1)
154-156: Partner label fallback LGTM.Using topValue.label with a fallback is correct and matches UX.
src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js (1)
315-376: Tests cover label override and fallback well.Good coverage for partner field label behavior. No changes requested.
src/django/oar/settings.py (1)
138-139: Enable CKEditor app — OK.App registration looks correct and matches requirements.
Confirm no need for ckeditor_uploader (uploads are disabled in config).
src/django/requirements.txt (1)
21-21: Add django-ckeditor — OK.Pinned version aligns with Django 3.2 usage.
src/django/api/migrations/0184_add_source_by_to_partner_field.py (1)
13-19: Migration for PartnerField.source_by — OK.Field type and options look correct; aligns with CKEditor default config.
src/django/api/constants.py (1)
302-302: Constant rename verification complete — no issues found.The codebase contains no stale references to
PARTNER_FIELD_NAMES_LIST_KEY. The new constantPARTNER_FIELD_LIST_KEYis properly defined at line 302 insrc/django/api/constants.pyand correctly imported and used in all locations:facility_index_serializer.pyandpartner_field.py.src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
50-166: Sanitization not applied to sourceBy—XSS vulnerability.Backend sends
source_by(RichTextField in PartnerField) unsanitized through the API. Frontend renders it withdangerouslySetInnerHTML, creating an HTML injection vulnerability. Although django_bleach is available in the codebase, it's not applied to this field.Convert PartnerField.source_by to BleachField or sanitize in the serializer before response.
⛔ Skipped due to learnings
Learnt from: VadimKovalenkoSNF Repo: opensupplyhub/open-supply-hub PR: 391 File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py:25-31 Timestamp: 2024-11-13T07:17:11.796Z Learning: In the Open Supply Hub project, query parameters used in OpenSearch queries are properly sanitized in related serializers and validators, ensuring protection against injection attacks.Learnt from: VadimKovalenkoSNF Repo: opensupplyhub/open-supply-hub PR: 436 File: src/react/src/components/DialogTooltip.jsx:0-0 Timestamp: 2024-12-03T10:29:02.409Z Learning: In the `DialogTooltip` component (`src/react/src/components/DialogTooltip.jsx`), there are existing tooltip tests that cover common cases, and further detailed tests are not necessary.Learnt from: Innavin369 Repo: opensupplyhub/open-supply-hub PR: 483 File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:0-0 Timestamp: 2025-01-17T16:12:18.285Z Learning: In the SearchByNameAndAddressSuccessResult component's tests, attempting to test internal navigation logic through mocking useHistory is not feasible, and button presence/click events should be covered in the main rendering test case.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py(2 hunks)
🔇 Additional comments (2)
src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py (2)
21-22: LGTM: New fields added correctly.The additions of
source_by,unit, andlabelalign with the PR objectives to display PartnerField metadata in the serialized output.
32-40: Excellent refactoring to dispatch table.The refactoring from explicit conditionals to a
field_serializersdispatch table significantly improves code maintainability. The separation between dedicated serializers and context-driven overrides is clear and logical.
src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py
Show resolved
Hide resolved
…abel-unit-partner-field
…abel-unit-partner-field
…abel-unit-partner-field
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Pls, apply minor fix for tests.
…abel-unit-partner-field
…abel-unit-partner-field
|
3 participants
OSDEV-2199 [Climate TRACE] Display partner field "label" and "unit" in Production Location Page
unitandlabelmetadata fromPartnerFieldto the serialized partner fields payload.unitinline with field values and display custom partner fieldlabel.Depend on -> #792