[OSDEV-2185] [Climate TRACE] Display Source By Text#792
Conversation
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
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
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
…85-display-source-by
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
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/oar/settings.py (1)
531-535: Environment key required in all envs will break local/tests.Gate GOOGLE_SERVER_SIDE_API_KEY behind not DEBUG (and/or BATCH_MODE), mirroring other settings.
-GOOGLE_SERVER_SIDE_API_KEY = os.getenv('GOOGLE_SERVER_SIDE_API_KEY') -if GOOGLE_SERVER_SIDE_API_KEY is None: - raise ImproperlyConfigured( - 'Invalid GOOGLE_SERVER_SIDE_API_KEY provided, must be set') +GOOGLE_SERVER_SIDE_API_KEY = os.getenv('GOOGLE_SERVER_SIDE_API_KEY') +if GOOGLE_SERVER_SIDE_API_KEY is None and not DEBUG: + raise ImproperlyConfigured( + 'Invalid GOOGLE_SERVER_SIDE_API_KEY provided, must be set')
🧹 Nitpick comments (5)
src/react/src/util/util.js (1)
1779-1801: source_by → sourceBy mapping is correct; normalize whitespace.Trim and nullify whitespace-only values to avoid rendering empty containers.
export const formatExtendedField = ({ value, field_name, created_at, - source_by, + source_by, contributor_name, is_from_claim, is_verified, formatValue = rawValue => rawValue, }) => { const primary = renderUniqueListItems(formatValue(value), field_name); const secondary = formatAttribution(created_at, contributor_name); return { primary, secondary, - sourceBy: source_by, + sourceBy: (typeof source_by === 'string' && source_by.trim()) || null, embeddedSecondary: formatAttribution(created_at), isVerified: is_verified, isFromClaim: is_from_claim, key: uuidv4(), }; };doc/release/RELEASE-NOTES.md (1)
6-22: Tidy heading levels and list indentation; confirm release date.
- Use “### Migrations” instead of “####” to avoid level jumps; adjust bullets to standard 2‑space indent.
- Release date (November 15, 2025) is in the future; keep as placeholder but remember to finalize at freeze.
Based on learnings
src/django/oar/settings.py (1)
505-521: CKEditor config: reduce attack surface and align with bleach.
- Consider removing “Source” button unless strictly needed.
- Align allowed tags/attrs with BLEACH_*; if links open in new tab, allow and set rel="noopener noreferrer".
- Add BLEACH_ALLOWED_PROTOCOLS to restrict href schemes.
CKEDITOR_CONFIGS = { 'default': { - 'toolbar': [ + 'toolbar': [ ['Bold', 'Italic', 'Underline', 'Strike'], ['NumberedList', 'BulletedList'], - ['Link', 'Unlink'], - ['RemoveFormat', 'Source'], + ['Link', 'Unlink'], + ['RemoveFormat'], ], } } + +# Bleach protocol hardening +BLEACH_ALLOWED_PROTOCOLS = ['http', 'https', 'mailto'] +# If you want to enforce rel on links, also allow it: +BLEACH_ALLOWED_ATTRIBUTES = { + 'a': ['href', 'target', 'title', 'rel'], +}src/react/src/components/FacilityDetailsItem.jsx (1)
59-67: LGTM on sourceBy propagation; drop unused props to child.FacilityDetailsDetail doesn’t consume lat/lng; remove to reduce noise.
- <FacilityDetailsDetail - primary={primary} - lat={lat} - lng={lng} + <FacilityDetailsDetail + primary={primary} locationLabeled={locationLabeled} secondary={!embed ? secondary : null} sourceBy={!embed ? sourceBy : null} isVerified={isVerified} isFromClaim={isFromClaim} /> ... - <FacilityDetailsDetail - primary={primary || `${lat}, ${lng}` || null} + <FacilityDetailsDetail + primary={primary || `${lat}, ${lng}` || null} secondary={!embed ? secondary : null} sourceBy={!embed ? sourceBy : null} isVerified={isVerified} isFromClaim={isFromClaim} />Also applies to: 94-101
src/django/api/admin.py (1)
246-255: Consider letting Django auto-generate the widget.The
source_byfield is explicitly defined withCKEditorWidget, but since the model field is already aRichTextFieldfromckeditor.fields, Django's ModelForm should automatically provide the appropriate CKEditor widget. The explicit definition may be unnecessary unless you need specific customization.If no custom configuration is needed, you can simplify to:
class PartnerFieldAdminForm(forms.ModelForm): - source_by = forms.CharField( - required=False, - widget=CKEditorWidget() - ) - class Meta: model = PartnerField fields = ['name', 'type', 'unit', 'label', 'source_by']Alternatively, if the explicit widget is intentional for customization, this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
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/components/FacilityDetailsDetail.jsx(3 hunks)src/react/src/components/FacilityDetailsItem.jsx(3 hunks)src/react/src/util/util.js(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 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/FacilityDetailsDetail.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/FacilityDetailsDetail.jsx
📚 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.js
📚 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-27T07:20:43.334Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 470
File: src/react/src/__tests__/components/ProductionLocationDialog.test.js:0-0
Timestamp: 2025-01-27T07:20:43.334Z
Learning: In the Open Supply Hub project, buttons wrapped by the `DialogTooltip` component control their disabled state through CSS pointer-events rather than the HTML disabled attribute, requiring tests to check `getComputedStyle().pointerEvents` instead of using `toBeDisabled()`.
Applied to files:
src/react/src/__tests__/components/FacilityDetailsDetail.test.js
📚 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: 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-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-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_index_serializer.py
🧬 Code graph analysis (4)
src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
src/react/src/components/FacilityDetailsDetail.jsx (1)
FacilityDetailsDetail(52-94)
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-566)
src/django/api/admin.py (1)
src/django/api/models/partner_field.py (2)
Meta(24-25)PartnerField(8-76)
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] 84-84: 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
12-12: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.3)
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
254-254: 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). (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-countries-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (14)
src/django/requirements.txt (1)
21-21: CKEditor dependency addition looks good.Version pin is reasonable for Django 3.2; INSTALLED_APPS and config are present in settings.
src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
50-123: Good coverage for sourceBy HTML rendering and null/empty cases.Once sanitization is implemented, add one test that verifies unsafe tags/attrs are stripped.
src/django/api/constants.py (1)
302-302: Cache key rename verified—no legacy references remain.The old
PARTNER_FIELD_NAMES_LIST_KEYconstant has been completely removed from the codebase. The newPARTNER_FIELD_LIST_KEYis properly defined and actively used in cache operations acrossfacility_index_serializer.pyandpartner_field.py. No stale cache keys found.src/django/api/tests/test_facility_index_serializer.py (2)
140-180: LGTM! Comprehensive test for source_by propagation.The test properly validates that HTML content in the
source_byfield is correctly serialized and propagated through the facility index serializer.
182-219: LGTM! Good coverage for null case.The test correctly verifies that
source_byis serialized asNonewhen not set on the partner field.src/django/api/admin.py (1)
258-260: LGTM! Appropriate admin configuration.The admin updates correctly integrate the new
source_byfield into the list display and search functionality.src/django/api/migrations/0184_add_source_by_to_partner_field.py (1)
1-20: LGTM! Standard Django migration.The migration correctly adds the
source_byRichTextField to the PartnerField model with appropriate nullable and blank attributes.src/django/api/models/partner_field.py (2)
53-61: LGTM! Well-defined RichTextField.The
source_byfield is properly configured with appropriate constraints and clear help text.
71-75: LGTM! Cache invalidation properly updated.The cache key change from
PARTNER_FIELD_NAMES_LIST_KEYtoPARTNER_FIELD_LIST_KEYcorrectly reflects the shift to caching full PartnerField objects.src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py (2)
21-21: LGTM! Field added to serialization.The
source_byfield is correctly added to the serializer's field list.
53-56: LGTM! Proper context-based serialization.The logic correctly retrieves
source_byfrom the serialization context when present, with appropriate null checking and fallback behavior.src/django/api/serializers/facility/facility_index_serializer.py (3)
95-136: LGTM! Well-implemented shift to PartnerField objects.The method correctly processes full PartnerField objects, extracting both
field_nameandsource_by, and properly passessource_bythrough the serializer context. Error handling and sorting logic remain intact.
196-208: LGTM! Appropriate caching of PartnerField objects.The method correctly caches full PartnerField objects instead of just names, enabling access to the
source_byfield. Thelist()call properly forces queryset evaluation before caching.
435-444: LGTM! Consistent method usage.The calls to the renamed method and parameter passing are correct and consistent with the refactored implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/django/api/admin.py (2)
247-256: Consider usingMeta.widgetsinstead of redefining the field.Since
source_byis already aRichTextFieldin the model, explicitly defining it as aCharFieldwithCKEditorWidgetis redundant. The cleaner Django pattern is to use thewidgetsattribute inMeta:class PartnerFieldAdminForm(forms.ModelForm): - source_by = forms.CharField( - required=False, - widget=CKEditorWidget() - ) - class Meta: model = PartnerField fields = ['name', 'type', 'unit', 'label', 'source_by'] + widgets = { + 'source_by': CKEditorWidget() + }This approach avoids duplicating field configuration and lets Django handle the field type automatically from the model.
261-261: Searching HTML content includes markup tags.Adding
source_bytosearch_fieldswill search the raw HTML content, including tags like<p>,<a>, etc. While functional, this might match unintended results when users search for terms that appear in HTML markup.This is acceptable for most use cases, but if cleaner search behavior is desired, you could create a plain-text version of the field for searching or exclude it from
search_fieldsif the HTML noise outweighs the benefit.doc/release/RELEASE-NOTES.md (1)
20-21: Align list indentation with linter standards.Lines 20–21 use 4-space indentation for nested list items, but the Markdown linter expects 2 spaces (MD007). While this matches the existing pattern throughout the file (e.g., Release 2.15.0), standardizing to 2 spaces improves consistency with Markdown conventions.
~### Release instructions ~* Ensure that the following commands are included in the `post_deployment` command: ~ ~ * `migrate` ~ ~ * `reindex_database`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/admin.py(3 hunks)src/django/api/tests/test_facility_index_serializer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/tests/test_facility_index_serializer.py
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
src/django/api/admin.py (1)
src/django/api/models/partner_field.py (2)
Meta(24-25)PartnerField(8-76)
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
12-12: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.3)
src/django/api/admin.py
254-254: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (3)
src/django/api/admin.py (2)
3-3: LGTM! Imports are appropriate.The new imports for
formsandCKEditorWidgetare correctly added to support the admin form customization.Also applies to: 19-19
260-260: Verify HTML rendering in the admin list view.Adding
source_by(which contains rich text/HTML) directly tolist_displaymay display raw HTML tags in the admin list, affecting readability.Please verify how the HTML content renders in the admin list view. If raw tags are displayed, consider using a custom method to show sanitized or truncated plain text:
def source_by_preview(self, obj): from django.utils.html import strip_tags if obj.source_by: plain = strip_tags(obj.source_by) return plain[:50] + '...' if len(plain) > 50 else plain return '-' source_by_preview.short_description = 'Source By' list_display = ('name', 'type', 'label', 'unit', 'source_by_preview', 'created_at')doc/release/RELEASE-NOTES.md (1)
13-16: Content is accurate and aligns with PR objectives.The migration and feature descriptions for
source_byRichTextField inPartnerFieldcorrectly capture the key changes: optional field with CKEditor support, rich formatting (bold, italic, links, lists), and integration with facility index serializers and views. The release notes are ready for deployment.
…85-display-source-by
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)
doc/release/RELEASE-NOTES.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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-countries-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
6-23: Release notes for 2.16.0 are well-structured and comprehensive.The release notes follow the established pattern from Release 2.15.0 and earlier. The heading hierarchy is correct, the migration is clearly documented, the "What's new" section provides adequate detail, and release instructions are appropriate. The content appears finalized (not placeholder text), which aligns well with the PR objectives for adding the
source_byfield feature.
# Conflicts: # doc/release/RELEASE-NOTES.md
src/django/api/serializers/facility/facility_index_serializer.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/django/api/serializers/facility/facility_index_serializer.py (1)
202-204: Consider optimizing the database query for better performance.The query now fetches all fields of
PartnerFieldobjects, including the potentially largesource_byRichTextField. However, based on the code in__serialize_and_sort_partner_fields(lines 105-107), onlyfield.nameandfield.source_byare actually used in this code path.If
unitandlabelfields are not needed here, consider optimizing the query:- partner_fields = list( - PartnerField.objects.all() - ) + partner_fields = list( + PartnerField.objects.only('name', 'source_by') + )This would reduce memory usage and improve cache efficiency, especially if there are many partner fields or if
source_bycontains substantial HTML content.Note: If
unitandlabelare needed elsewhere in the serialization flow, this optimization may not be applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(2 hunks)src/django/api/serializers/facility/facility_index_serializer.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
src/django/api/serializers/facility/facility_index_serializer.py (1)
src/django/api/models/partner_field.py (1)
PartnerField(8-76)
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
23-23: Multiple headings with the same content
(MD024, no-duplicate-heading)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (12)
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
doc/release/RELEASE-NOTES.md (1)
37-38: Resolve list indentation to comply with project standards.Lines 37–38 use 4-space indentation for nested list items, but the project's Markdown linter (markdownlint-cli2) expects 2-space indentation (MD007 rule). Although you previously declined this fix, maintaining consistent indentation across all releases improves maintainability. Please apply 2-space indentation:
### Release instructions * Ensure that the following commands are included in the `post_deployment` command: - * `migrate` - * `reindex_database` + * `migrate` + * `reindex_database`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
23-23: Multiple headings with the same content
(MD024, no-duplicate-heading)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
40-40: The review comment is incorrect. The file already has the required spacing.The current file contains two blank lines (lines 39-40) between the Release 2.15.1 section and the Release 2.15.0 section, which matches the stated project convention. No additional blank line is needed.
Likely an incorrect or invalid review comment.
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Approved.
P.S. - I would rather change source_by to something like contributed_by since we consistently use the term source across the system to refer to APIs or list uploads.
… in Production Location Page (#807) **[OSDEV-2199](https://opensupplyhub.atlassian.net/browse/OSDEV-2199) [Climate TRACE] Display partner field "label" and "unit" in Production Location Page** - Added `unit` and `label` metadata from `PartnerField` to the serialized partner fields payload. - Production Location detail pages now render the `unit` inline with field values and display custom partner field `label`. <img width="806" height="232" alt="Screenshot 2025-11-11 at 11 13 17" src="https://github.com/user-attachments/assets/8f6c1e3e-80d8-4e1d-bc47-ae38c58975cd" /> _**Depend on**_ -> #792 [OSDEV-2199]: https://opensupplyhub.atlassian.net/browse/OSDEV-2199?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/react/src/components/FacilityDetailsDetail.jsx (1)
90-96: Unsafe HTML injection: confirm sanitization pathRendering sourceBy via dangerouslySetInnerHTML is an XSS sink. If you already sanitize on the server (e.g., bleach allow‑list), we're good; otherwise sanitize client‑side (DOMPurify) or tighten allowed tags/attrs.
If server-side: please point to the sanitizer. If not, consider this minimal FE change:
+import DOMPurify from 'dompurify'; ... - dangerouslySetInnerHTML={{ __html: sourceBy }} + dangerouslySetInnerHTML={{ + __html: DOMPurify.sanitize(sourceBy, { + ALLOWED_TAGS: ['a','strong','em','ul','ol','li','br','p','code'], + ALLOWED_ATTR: ['href','title','target','rel'], + }), + }}
🧹 Nitpick comments (1)
src/django/api/serializers/facility/facility_index_serializer.py (1)
200-213: Avoid caching ORM instances; cache plain dicts insteadCaching PartnerField model instances increases memory/serialization overhead and can be brittle across deploys/workers. Cache only the fields you need.
- def __get_cached_partner_fields(): - cached_names = cache.get(PARTNER_FIELD_LIST_KEY) + def __get_cached_partner_fields(): + cached = cache.get(PARTNER_FIELD_LIST_KEY) - if cached_names is not None: - return cached_names + if cached is not None: + return cached - partner_fields = list( - PartnerField.objects.all() - ) - cache.set(PARTNER_FIELD_LIST_KEY, partner_fields, 60) - return partner_fields + partner_fields = list( + PartnerField.objects.values('name', 'source_by', 'unit', 'label') + ) + cache.set(PARTNER_FIELD_LIST_KEY, partner_fields, 60) + return partner_fieldsAnd adjust usage in __serialize_and_sort_partner_fields:
- for field in partner_fields: - field_name = field.name - source_by = field.source_by - unit = field.unit - label = field.label + for field in partner_fields: + field_name = field['name'] + source_by = field['source_by'] + unit = field['unit'] + label = field['label']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
doc/release/RELEASE-NOTES.md(2 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/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)
✅ Files skipped from review due to trivial changes (1)
- src/react/src/components/FacilityDetailsLocationFields.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/src/util/util.js
- src/django/api/tests/test_facility_index_serializer.py
🧰 Additional context used
🧠 Learnings (8)
📚 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/FacilityDetailsItem.jsxsrc/react/src/components/FacilityDetailsDetail.jsx
📚 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: 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: 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: 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.jsx
📚 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.jsx
📚 Learning: 2024-12-03T06:52:22.170Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 436
File: src/react/src/components/ProductionLocationDialog.jsx:255-257
Timestamp: 2024-12-03T06:52:22.170Z
Learning: In `src/react/src/components/ProductionLocationDialog.jsx`, the 'Submit another Location' button's `onClick` handler logs to the console as the endpoint is not yet implemented on the frontend.
Applied to files:
src/react/src/components/FacilityDetailsDetail.jsx
📚 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
🧬 Code graph analysis (2)
src/django/api/serializers/facility/facility_index_serializer.py (1)
src/django/api/models/partner_field.py (1)
PartnerField(8-76)
src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
src/react/src/components/FacilityDetailsDetail.jsx (1)
FacilityDetailsDetail(60-104)
🪛 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: Multiple headings with the same content
(MD024, no-duplicate-heading)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (6)
src/react/src/__tests__/components/FacilityDetailsDetail.test.js (1)
50-69: Solid coverage for sourceBy and unit renderingThese tests validate HTML rendering, null/empty handling, and unit placement well. Nice work.
Also applies to: 71-90, 91-109, 111-127, 129-149, 151-165
src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js (1)
315-347: Label fallback behavior well exercisedBoth cases (explicit label vs. fallback) are covered and match the intended UI.
Also applies to: 348-379
src/react/src/components/FacilityDetailsGeneralFields.jsx (1)
150-158: Correct partner-field label precedenceUsing topValue.label with fallback to generated label is correct and matches the UX.
src/react/src/components/FacilityDetailsItem.jsx (1)
96-104: Confirm repeated source info in drawerAdditional entries also render sourceBy (since {...item} is passed). If the intent is to show source once per field (top only), null it for additionalContent items; otherwise keep as is.
Option to show top only:
- <FacilityDetailsDetail - primary={primary || `${lat}, ${lng}` || null} - secondary={!embed ? secondary : null} - sourceBy={!embed ? sourceBy : null} - unit={!embed ? unit : null} - isVerified={isVerified} - isFromClaim={isFromClaim} - /> + <FacilityDetailsDetail + primary={primary || `${lat}, ${lng}` || null} + secondary={!embed ? secondary : null} + sourceBy={!embed ? sourceBy : null} + unit={!embed ? unit : null} + isVerified={isVerified} + isFromClaim={isFromClaim} + /> ... - {isOpen && additionalContent.map(item => ( + {isOpen && additionalContent.map(item => ( <div className={classes.itemWrapper} key={item.key}> - <FacilityDetailsDetail {...item} /> + <FacilityDetailsDetail {...{ ...item, sourceBy: null, unit: null }} /> </div> ))}doc/release/RELEASE-NOTES.md (1)
21-41: Release 2.15.1 entry reads well; migration number matches codeMigration 0185 reference and feature notes align with the changes in this PR.
Also applies to: 29-35
src/django/api/serializers/facility/facility_index_serializer.py (1)
210-210: Cache TTL cut to 60s — intended?Previously 600s; confirm the lower TTL is deliberate to reflect frequent admin edits to partner fields. If not, consider reverting to reduce DB hits.
src/django/api/serializers/facility/facility_index_extended_field_list_serializer.py
Show resolved
Hide resolved
3 participants
OSDEV-2185 [Climate TRACE] Display Source By Text
source_byfield to thePartnerFieldmodel. This allows administrators to provide rich text descriptions of data sources. The source information is displayed on the facility details page below each partner field value, supporting HTML formatting for links, emphasis, and lists.source_byin the partner fields response only when the field contains content.0184_add_source_by_to_partner_field.pyadds asource_byRichTextField to thePartnerFieldmodel, allowing administrators to document the data source for each partner field using rich text formatting (bold, italic, links, lists). The field is optional and uses CKEditor for content editing.