[OSDEV-2266] [Partner Fields Enhancement] Add JSON Schema support for the Object type field#816
Conversation
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
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
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.45%Your code coverage diff: 0.00% ▴ ✅ 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
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%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
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Django App | Unittest test suite - Code coverage reportTotal: 81.52%Your code coverage diff: 0.02% ▴ ✅ All code changes are covered |
…66-add-json-schema-support
📝 WalkthroughWalkthroughAdds optional JSON Schema support to PartnerField (model + migration), an admin JSON editor and JS visibility toggle, a PartnerFieldTypeProcessor that validates partner-field inputs (type and JSON Schema) in the moderation pipeline, integration changes removing type checks from PermissionProcessor, tests for JSON Schema validation (duplicated in diff), settings/dependency updates, and two CloudFront cache behaviors for jsoneditor static assets. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocationContrib as LocationContribution.serialize
participant PartnerProc as PartnerFieldTypeProcessor
participant PermProc as PermissionProcessor
participant SourceProc as SourceProcessor
Client->>LocationContrib: submit event_dto
LocationContrib->>PartnerProc: process(event_dto)
PartnerProc->>PartnerProc: load partner field defs & parse json_schema
alt no partner fields
PartnerProc->>PermProc: pass-through (no validation)
else partner fields present
PartnerProc->>PartnerProc: validate fields (type checks / JSON Schema)
alt validation errors
PartnerProc-->>Client: HTTP 422 with structured errors
else
PartnerProc->>PermProc: pass validated event_dto
end
end
PermProc->>SourceProc: process(event_dto)
SourceProc-->>Client: response (e.g., 202)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 3
🧹 Nitpick comments (3)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (1)
24-41: Permission logic remains correct; consider simplifying partner_field_names handlingThe updated lookup of partner field names via
values_list("name", flat=True)keeps the original semantics: only definedPartnerFieldnames are considered, andunauthorizedstill correctly computes the difference between requested and contributor‑allowed fields. This should continue to satisfy the existing permission tests, including the “invalid partner field” case.You can slightly simplify the data flow by making
partner_field_namesasetdirectly, avoiding the extralist→setconversion later:- partner_field_names = list( - PartnerField.objects - .filter(name__in=incoming_keys) - .values_list("name", flat=True) - ) - - if not partner_field_names: + partner_field_names: set[str] = set( + PartnerField.objects + .filter(name__in=incoming_keys) + .values_list("name", flat=True) + ) + + if not partner_field_names: return super().process(event_dto) @@ - requested_partner_field_names: set[str] = set(partner_field_names) + requested_partner_field_names: set[str] = partner_field_namessrc/django/api/tests/test_location_contribution_strategy.py (1)
1875-2238: New JSON Schema tests exercise key scenarios; minor consistency and lint considerationsThe three new tests for JSON Schema–backed partner fields look solid:
- They set up
PartnerFieldinstances withtype=PartnerField.OBJECTand appropriatejson_schema.- They assert:
- 202 for valid data.
- 422 plus a single field‑level error for missing required properties and wrong types.
- 422 with a nested error message mentioning the problematic path (
contact) for the nested schema case.- The assertions are resilient (checking presence of substrings like
'age'/'name'rather than brittle full messages) and match the error structure produced byPartnerFieldTypeProcessor.Two small points to consider:
CREATE +
oscombination
In the JSON Schema tests you constructCreateModerationEventDTOwithrequest_type=ModerationEvent.RequestType.CREATE.valuewhile also passingos=production_location. In the actual API flow forcreate, the DTO is built withoutos. Please double‑check that this combination doesn’t affect the processing chain (e.g., by unintentionally exercising an UPDATE‑like path). If it’s unused, you might simplify by omittingosfor these CREATE tests.Ruff S105 on hardcoded passwords in tests
The new tests introduce additional hardcoded passwords (e.g.,existing_location_user_password = '4567test'). This is standard practice in tests and not a real security issue. If Ruff’s S105 is enforced on test code, you may want to add# noqa: S105on those assignments (or adjust your Ruff configuration) rather than changing the test data itself.src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py (1)
19-31: TYPE_VALIDATORS and FORMAT_CHECKER design is solid; consider minor polishThe
TYPE_VALIDATORSmapping andFORMAT_CHECKERare well‑chosen:
int/floatexcludebool, avoiding the commonisinstance(True, int)pitfall.objectaccepting bothdictandlistmatches the earlier tests’ expectations for object‑type partner fields.FORMAT_CHECKERenables things like email format validation that your new tests rely on.Two optional refinements you might consider:
Tie the keys to
PartnerFieldconstants for tighter coupling to the model API:TYPE_VALIDATORS = {
'int': ...,'float': ...,'string': ...,'object': ...,}
- TYPE_VALIDATORS = {
PartnerField.INT: ...,PartnerField.FLOAT: ...,PartnerField.STRING: ...,PartnerField.OBJECT: ...,- }
- If you want to satisfy Ruff’s `RUF012`, you could annotate these as `ClassVar[...]` or move them to module‑level constants, but that’s purely stylistic and not functionally necessary. These are non‑blocking and can be deferred. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3e2e37b4878e6d836f0c92d235ae13c24eba4ce1 and 64794f0f9a185d4dadc611de1c9def88be65ac8a. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `deployment/terraform/cdn.tf` (1 hunks) * `doc/release/RELEASE-NOTES.md` (1 hunks) * `src/django/api/admin.py` (2 hunks) * `src/django/api/migrations/0186_add_json_schema_to_partner_field.py` (1 hunks) * `src/django/api/models/partner_field.py` (1 hunks) * `src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py` (2 hunks) * `src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py` (1 hunks) * `src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py` (2 hunks) * `src/django/api/tests/test_location_contribution_strategy.py` (1 hunks) * `src/django/oar/settings.py` (1 hunks) * `src/django/requirements.txt` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (6)</summary> <details> <summary>📚 Learning: 2024-12-17T16:45:39.671Z</summary>Learnt from: mazursasha1990
Repo: opensupplyhub/open-supply-hub PR: 438
File: src/django/api/moderation_event_actions/approval/event_approval_template.py:143-191
Timestamp: 2024-12-17T16:45:39.671Z
Learning: In the filesrc/django/api/moderation_event_actions/approval/event_approval_template.py, coordinate validation is not required in the__apply_manual_locationmethod.**Applied to files:** - `src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py` - `src/django/api/tests/test_location_contribution_strategy.py` </details> <details> <summary>📚 Learning: 2024-11-21T13:06:56.072Z</summary>Learnt from: mazursasha1990
Repo: opensupplyhub/open-supply-hub PR: 400
File: src/django/api/moderation_event_actions/approval/add_production_location_strategy.py:193-194
Timestamp: 2024-11-21T13:06:56.072Z
Learning: Insrc/django/api/moderation_event_actions/approval/add_production_location_strategy.py, within theAddProductionLocationStrategyclass, when processing a moderation event for adding a production location,item.geocoded_pointis guaranteed to be notNonedue to prior assignments in the__apply_geocode_resultor__apply_manual_locationmethods.**Applied to files:** - `src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py` </details> <details> <summary>📚 Learning: 2024-12-12T14:59:19.694Z</summary>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: Insrc/django/api/tests/test_moderation_events_add_production_location.py, the tests have been refactored, and the use ofPOSTmethods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.**Applied to files:** - `src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py` - `src/django/api/tests/test_location_contribution_strategy.py` </details> <details> <summary>📚 Learning: 2025-06-02T13:24:57.659Z</summary>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` </details> <details> <summary>📚 Learning: 2024-11-12T11:42:07.768Z</summary>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 theModerationEventsAPI endpoint, parameter validation is handled insrc/django/api/serializers/v1/moderation_events_serializer.py.**Applied to files:** - `src/django/api/tests/test_location_contribution_strategy.py` - `src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py` </details> <details> <summary>📚 Learning: 2024-12-26T09:07:37.571Z</summary>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/test_location_contribution_strategy.py` </details> </details><details> <summary>🧬 Code graph analysis (5)</summary> <details> <summary>src/django/api/admin.py (1)</summary><blockquote> <details> <summary>src/django/api/models/partner_field.py (2)</summary> * `Meta` (24-25) * `PartnerField` (8-85) </details> </blockquote></details> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py (1)</summary><blockquote> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py (1)</summary> * `PartnerFieldTypeProcessor` (17-218) </details> </blockquote></details> <details> <summary>src/django/api/tests/test_location_contribution_strategy.py (7)</summary><blockquote> <details> <summary>src/django/api/models/user.py (1)</summary> * `User` (71-213) </details> <details> <summary>src/django/api/views/v1/production_locations.py (1)</summary> * `create` (167-217) </details> <details> <summary>src/django/api/models/partner_field.py (2)</summary> * `save` (78-80) * `PartnerField` (8-85) </details> <details> <summary>src/django/api/models/contributor/contributor.py (1)</summary> * `Contributor` (14-159) </details> <details> <summary>src/django/api/models/moderation_event.py (3)</summary> * `Source` (24-26) * `ModerationEvent` (10-166) * `RequestType` (14-17) </details> <details> <summary>src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)</summary> * `CreateModerationEventDTO` (12-22) </details> <details> <summary>src/django/api/moderation_event_actions/creation/moderation_event_creator.py (1)</summary> * `perform_event_creation` (13-32) </details> </blockquote></details> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (1)</summary><blockquote> <details> <summary>src/django/api/models/partner_field.py (1)</summary> * `PartnerField` (8-85) </details> </blockquote></details> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py (4)</summary><blockquote> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/processors/contribution_processor.py (1)</summary> * `ContributionProcessor` (14-46) </details> <details> <summary>src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)</summary> * `CreateModerationEventDTO` (12-22) </details> <details> <summary>src/django/api/models/partner_field.py (1)</summary> * `PartnerField` (8-85) </details> <details> <summary>src/django/api/constants.py (1)</summary> * `APIV1CommonErrorMessages` (241-253) </details> </blockquote></details> </details><details> <summary>🪛 Ruff (0.14.5)</summary> <details> <summary>src/django/api/migrations/0186_add_json_schema_to_partner_field.py</summary> 8-10: Mutable class attributes should be annotated with `typing.ClassVar` (RUF012) --- 12-18: Mutable class attributes should be annotated with `typing.ClassVar` (RUF012) </details> <details> <summary>src/django/api/admin.py</summary> 265-265: Mutable class attributes should be annotated with `typing.ClassVar` (RUF012) </details> <details> <summary>src/django/api/tests/test_location_contribution_strategy.py</summary> 1879-1879: Possible hardcoded password assigned to: "existing_location_user_password" (S105) --- 1980-1980: Possible hardcoded password assigned to: "existing_location_user_password" (S105) --- 2116-2116: Possible hardcoded password assigned to: "existing_location_user_password" (S105) </details> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/processors/partner_field_type_processor.py</summary> 19-28: Mutable class attributes should be annotated with `typing.ClassVar` (RUF012) --- 133-133: Consider moving this statement to an `else` block (TRY300) --- 140-140: Do not catch blind exception: `Exception` (BLE001) --- 141-141: Use explicit conversion flag Replace with conversion flag (RUF010) </details> </details> </details> <details> <summary>⏰ 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). (9)</summary> * GitHub Check: run-integration-test-code-quality * GitHub Check: run-contricleaner-code-quality * GitHub Check: run-flake8-linter * GitHub Check: run-django-code-quality * GitHub Check: run-countries-code-quality * GitHub Check: get-base-branch-countries-cov * GitHub Check: run-dd-code-quality * GitHub Check: get-base-branch-contricleaner-cov * GitHub Check: get-base-branch-django-cov </details> <details> <summary>🔇 Additional comments (7)</summary><blockquote> <details> <summary>src/django/oar/settings.py (1)</summary><blockquote> `139-140`: **Registering `jsoneditor` app looks correct** Adding `jsoneditor` to `INSTALLED_APPS` aligns with the new admin widget usage and is consistent with existing app registrations. No issues from a Django config perspective. </blockquote></details> <details> <summary>deployment/terraform/cdn.tf (1)</summary><blockquote> `677-719`: **CloudFront behaviors for JSON editor static assets are consistent** The new `ordered_cache_behavior` entries for `/static/jsoneditor/*` and `/static/django-jsoneditor/*` mirror existing static asset behaviors (origin, methods, headers, cookies, TTLs, HTTPS redirect), so they should integrate cleanly without affecting other paths. </blockquote></details> <details> <summary>src/django/api/models/partner_field.py (1)</summary><blockquote> `63-71`: **`json_schema` JSONField on `PartnerField` is well‑scoped** The optional `json_schema = models.JSONField(...)` cleanly extends `PartnerField` for OBJECT‑type validation and matches how the new processor reads `name`, `type`, and `json_schema`. Cache invalidation remains centralized in `save`/`delete`, so no extra work is needed here. </blockquote></details> <details> <summary>src/django/api/moderation_event_actions/creation/location_contribution/location_contribution.py (1)</summary><blockquote> `12-13`: **Partner field processor is correctly wired into the contribution pipeline** Adding `PartnerFieldTypeProcessor` after `PermissionProcessor` and before `SourceProcessor` ensures partner field type/JSON Schema validation happens early and can short‑circuit with a 422 response before doing more expensive work. This matches the existing processor chaining pattern. Also applies to: 40-46 </blockquote></details> <details> <summary>src/django/api/migrations/0186_add_json_schema_to_partner_field.py (1)</summary><blockquote> `1-18`: **Migration matches model change for `PartnerField.json_schema`** The migration cleanly adds `json_schema` to `PartnerField` with the same null/blank and help_text semantics as the model, and dependency ordering (0185 → 0186) is correct. Nothing else needed here. </blockquote></details> <details> <summary>src/django/api/admin.py (1)</summary><blockquote> `20-21`: **✓ Import and admin form configuration verified against `django-jsoneditor==0.2.4`** All checks pass: - `INSTALLED_APPS` correctly lists `'jsoneditor'` (src/django/oar/settings.py:139) - Import `from jsoneditor.forms import JSONEditor` matches documented API (src/django/api/admin.py:20) - `PartnerFieldAdminForm` properly wires `json_schema` as `forms.JSONField` with `JSONEditor` widget, code/tree modes, and full-width fixed-height styling (src/django/api/admin.py:248–266) - Version pinned at 0.2.4 in requirements.txt </blockquote></details> <details> <summary>src/django/requirements.txt (1)</summary><blockquote> `49-51`: **JSON tooling dependencies verified—compatible and secure** jsonschema 4.17.3 requires Python >=3.7 and includes Python 3.8 classifiers, rfc3987 1.3.8 is a py2.py3 wheel with broad Python 3.x support, and django-jsoneditor 0.2.4 is usable under Django 3.2 with no declared incompatibility. No published high-severity CVEs were identified for any of these three versions. Versions are compatible with Python 3.8 and Django 3.2; proceed with confidence. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
...tion_event_actions/creation/location_contribution/processors/partner_field_type_processor.py
Show resolved
Hide resolved
...tion_event_actions/creation/location_contribution/processors/partner_field_type_processor.py
Show resolved
Hide resolved
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Left few comments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/django/api/static/admin/js/partner_field_admin.js (1)
1-63: LGTM! Clean implementation of dynamic field visibility.The JavaScript module correctly implements the visibility toggle for the
json_schemafield based on the selectedtypevalue. The code is well-structured with proper error handling for missing DOM elements.Minor observations:
- The boolean return values from
toggleJsonSchemaField(lines 30, 33) andsetupJsonSchemaToggle(lines 42, 47, 56) are not used by any callers. Consider refactoring tovoidfunctions if the return values aren't needed for future functionality.- Line 41: The check for
!jsonSchemaField.lengthis redundant sincefindJsonSchemaFieldRow()already performs this check internally, but keeping it doesn't hurt and makes the code more defensive.src/django/api/admin.py (1)
267-268: Remove unnecessary__init__method.The
__init__method only callssuper()without adding any additional logic. In Python/Django, this is unnecessary and can be safely removed.Apply this diff to remove the empty method:
- def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployment/terraform/cdn.tf(1 hunks)doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/admin.py(3 hunks)src/django/api/static/admin/js/partner_field_admin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployment/terraform/cdn.tf
🧰 Additional context used
🧠 Learnings (2)
📚 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:
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-85)
🪛 Ruff (0.14.5)
src/django/api/admin.py
265-265: 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). (13)
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (5)
doc/release/RELEASE-NOTES.md (1)
12-18: LGTM! Release notes properly documented.The release notes correctly document the database changes, schema changes, and new feature:
- Migration 0186 is clearly described with its purpose
- Schema changes section addresses the previous review request
- What's New section appropriately describes the JSON schema validation feature for object-type partner fields
The merge conflict mentioned in previous review comments appears to have been resolved (lines 70-74 now show clean entries for OSDEV-2260 and OSDEV-2264 without conflict markers).
Also applies to: 23-23
src/django/api/admin.py (4)
20-20: LGTM!The import is correctly added to support the JSONEditor widget used in the form.
253-261: LGTM!The json_schema field is properly configured with the JSONEditor widget, providing both code and tree editing modes. The field correctly matches the model's optional nature (required=False).
265-265: LGTM!The Meta.fields list correctly includes the new json_schema field. Note: The static analysis hint (RUF012) about ClassVar annotation is a false positive—Django's ModelForm Meta.fields follows standard Django conventions and does not require ClassVar annotation.
277-282: LGTM!The Media class correctly includes the necessary JavaScript files. The custom
partner_field_admin.jsimplements the visibility toggling for the json_schema field based on the selected type, addressing the concerns raised in previous review discussions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/django/api/static/admin/js/partner_field_admin.js (3)
4-12: Remove redundant.first()call on line 8.The
.closest()method already returns the first matching ancestor, so calling.first()afterward is unnecessary.Apply this diff:
function findJsonSchemaFieldRow() { const jsonSchemaField = $('#id_json_schema'); if (jsonSchemaField.length) { - return jsonSchemaField.closest('.field-json_schema, tr, .form-row').first(); + return jsonSchemaField.closest('.field-json_schema, tr, .form-row'); } return $('.field-json_schema').first(); }
14-35: Consider simplifying return values.The boolean return values are never used by calling code. You could simplify by removing explicit returns or making them void functions.
Apply this diff if you prefer simpler code:
function toggleJsonSchemaField() { const typeField = $('#id_type'); const jsonSchemaFieldRow = findJsonSchemaFieldRow(); if (!typeField.length) { - return false; + return; } if (!jsonSchemaFieldRow.length) { - return false; + return; } const currentType = typeField.val(); if (currentType === 'object') { jsonSchemaFieldRow.show(); - return true; } else { jsonSchemaFieldRow.hide(); - return true; } }
37-57: Consider removing redundant field existence checks.Lines 38-43 and 45-48 duplicate the checks already present in
toggleJsonSchemaField()(lines 18-24). SincetoggleJsonSchemaField()handles missing elements gracefully, you could simplify this function.Apply this diff to reduce duplication:
function setupJsonSchemaToggle() { const typeField = $('#id_type'); - const jsonSchemaField = $('#id_json_schema'); - if (!typeField.length || !jsonSchemaField.length) { - return false; - } - - const jsonSchemaFieldRow = findJsonSchemaFieldRow(); - if (!jsonSchemaFieldRow.length) { - return false; + if (!typeField.length) { + return; } toggleJsonSchemaField(); typeField.on('change', function() { toggleJsonSchemaField(); }); - - return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/static/admin/js/partner_field_admin.js(1 hunks)
🔇 Additional comments (1)
src/django/api/static/admin/js/partner_field_admin.js (1)
59-62: LGTM!The document ready handler and jQuery detection pattern (using
django.jQuerywith a fallback tojQuery) follow Django admin best practices.
# Conflicts: # doc/release/RELEASE-NOTES.md
|
3 participants
OSDEV-2266 [Partner Fields Enhancement] Add JSON Schema support for the Object type field
0186_add_json_schema_to_partner_field.pymigration adds ajson_schemaJSONField to thePartnerFieldmodel, allowing administrators to define JSON schemas for validating object type partner fields. The field is optional and is used to validate the structure and content of contributed data when the partner field type isobject.