[Part-1] [OSDEV-2065] Update v1 production locations POST/PATCH endpoint to include partner field type validation#750
Conversation
React App | Jest test suite - Code coverage reportTotal: 35.39%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Django App | Unittest test suite - Code coverage reportTotal: 81.27%Your code coverage diff: -0.02% ▾ ✅ All code changes are covered |
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Outdated
Show resolved
Hide resolved
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
20-22: Fix nested list indentation to satisfy markdownlint (MD007)Indent sub-bullets by 2 spaces (not 4).
Apply this diff:
* [OSDEV-2065](https://opensupplyhub.atlassian.net/browse/OSDEV-2065) - Updated v1 production locations `POST/PATCH` endpoints to include partner fields: - * Added `unit` field to `PartnerField` model - * Added type validation for submitted partner fields + * Added `unit` field to `PartnerField` model + * Added type validation for submitted partner fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: 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-integration-test-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
16-16: Grammar/style tweak for clarityUse an article and avoid ampersands in prose.
[ suggest_nitpick_refactor ]
Apply this diff:-* 0180_add_unit_to_partner_field.py - This migration added new field `unit` to `PartnerField` model & updated `partner_fields` field in `Contributor` model. +* 0180_add_unit_to_partner_field.py - This migration added a new `unit` field to the `PartnerField` model and updated the `partner_fields` field in the `Contributor` model.
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Outdated
Show resolved
Hide resolved
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/django/api/migrations/0180_add_unit_to_partner_field.py (1)
13-22: Blocking: Adding CharField without NULL/default will fail on existing rowsblank=True is form-level only. Add a one-off default (and disable preserve) to avoid breaking on populated tables. Matches model semantics (non-null).
migrations.AddField( model_name='partnerfield', name='unit', - field=models.CharField( - max_length=200, - blank=True, - help_text=('The partner field unit.') - ), + field=models.CharField( + max_length=200, + blank=True, + help_text=('The partner field unit.'), + default='', + ), + preserve_default=False, ),src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (1)
15-24: Fix validator naming and float handling; exclude bools
- Rename TYPE_VALIDATORDS -> TYPE_VALIDATORS (typo).
- float should accept ints (JSON numbers often arrive as ints).
- Keep bool excluded from int/float.
- Add precise types; annotate as ClassVar to satisfy Ruff RUF012.
-class PermissionProcessor(ContributionProcessor): - - TYPE_VALIDATORDS = { - 'int': lambda value: isinstance(value, int) - and not isinstance(value, bool), - 'float': lambda value: isinstance(value, float) - and not isinstance(value, bool), - 'string': lambda value: isinstance(value, str), - 'object': lambda value: isinstance( - value, (dict, list) - ), - } +class PermissionProcessor(ContributionProcessor): + + TYPE_VALIDATORS: ClassVar[Mapping[str, Callable[[Any], bool]]] = { + 'int': lambda value: isinstance(value, int) and not isinstance(value, bool), + 'float': lambda value: isinstance(value, (int, float)) and not isinstance(value, bool), + 'string': lambda value: isinstance(value, str), + 'object': lambda value: isinstance(value, (dict, list)), + }
🧹 Nitpick comments (4)
src/django/api/migrations/0180_add_unit_to_partner_field.py (1)
5-7: Nit: Docstring grammarPrefer “Add unit field to PartnerField model.”.
- """ - Migration add unit field to PartnerField model. - """ + """ + Add unit field to PartnerField model. + """src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (3)
60-66: Update reference after renameUse TYPE_VALIDATORS.
- self.TYPE_VALIDATORDS + self.TYPE_VALIDATORS
95-106: Tighten parameter typing for error transformerUse concrete tuple shape to aid callers/tooling.
- def __transform_type_errors( - invalid_type_fields: List[tuple] - ) -> Dict: + def __transform_type_errors( + invalid_type_fields: List[Tuple[str, str, object]] + ) -> Dict:
1-1: Add typing imports used aboveRequired by the suggested fixes and satisfies Ruff’s ClassVar hint.
-from typing import Dict, List, Mapping, Tuple, Iterable +from typing import Any, Callable, ClassVar, Dict, List, Mapping, Tuple, Iterable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/django/api/migrations/0180_add_unit_to_partner_field.py(1 hunks)src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (3)
src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)
CreateModerationEventDTO(12-22)src/django/api/models/partner_field.py (1)
PartnerField(5-50)src/django/api/constants.py (1)
APIV1CommonErrorMessages(241-253)
🪛 Ruff (0.12.2)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
15-24: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/django/api/migrations/0180_add_unit_to_partner_field.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-23: 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-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
🔇 Additional comments (1)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (1)
21-23: Confirm spec: should “object” allow lists?If “object” means JSON object (mapping), restrict to dict only. Otherwise leave as-is.
- 'object': lambda value: isinstance(value, (dict, list)), + # If spec says JSON object only: + 'object': lambda value: isinstance(value, dict),
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (4)
15-24: Rename constant, fix validators, and annotate as ClassVar (RUF012).
- Misspelling: TYPE_VALIDATORDS → TYPE_VALIDATORS.
- Float should accept ints but exclude bool.
- Prefer dict-only for "object" unless arrays are explicitly supported.
- Add ClassVar to satisfy Ruff RUF012.
Apply:
- TYPE_VALIDATORDS = { - 'int': lambda value: isinstance(value, int) - and not isinstance(value, bool), - 'float': lambda value: isinstance(value, float) - and not isinstance(value, bool), - 'string': lambda value: isinstance(value, str), - 'object': lambda value: isinstance( - value, (dict, list) - ), - } + TYPE_VALIDATORS: ClassVar[Mapping[str, Callable[[Any], bool]]] = { + 'int': lambda value: isinstance(value, int) and not isinstance(value, bool), + 'float': lambda value: isinstance(value, (int, float)) and not isinstance(value, bool), + 'string': lambda value: isinstance(value, str), + 'object': lambda value: isinstance(value, dict), + }If arrays should be allowed, switch the object validator to
isinstance(value, (dict, list)). Please confirm intent.
61-66: Broken reference to misspelled constant.Use the corrected name.
- self.TYPE_VALIDATORDS + self.TYPE_VALIDATORS
1-1: Missing typing imports for upcoming fixes.Import Any, Callable, ClassVar.
-from typing import Dict, List, Mapping, Tuple, Iterable +from typing import Any, Callable, ClassVar, Dict, Iterable, List, Mapping, Tuple
111-119: Syntax error and weak typing in collector.
- Invalid annotated assignment at Line 118.
- Use typing.Callable/Any and precise generics.
- def __collect_invalid_type_fields( - raw: Mapping[str, object], - partner_fields: Mapping[str, str], - validators: Mapping[str, callable], - ) -> List[Tuple[str, str, object]]: + def __collect_invalid_type_fields( + raw: Mapping[str, Any], + partner_fields: Mapping[str, str], + validators: Mapping[str, Callable[[Any], bool]], + ) -> List[Tuple[str, str, Any]]: - invalid_fields = List[Tuple[str, str, object]] = [] + invalid_fields: List[Tuple[str, str, Any]] = []
🧹 Nitpick comments (2)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (2)
95-99: Tighten types for error transformer.Be explicit about tuple contents.
- def __transform_type_errors( - invalid_type_fields: List[tuple] - ) -> Dict: + def __transform_type_errors( + invalid_type_fields: List[Tuple[str, str, Any]] + ) -> Dict:
79-110: Consider single-underscore helpers instead of name-mangling.Double-underscore static methods are unusual and hinder discoverability; single underscore is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (3)
src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)
CreateModerationEventDTO(12-22)src/django/api/models/partner_field.py (1)
PartnerField(5-50)src/django/api/constants.py (1)
APIV1CommonErrorMessages(241-253)
🪛 Ruff (0.12.2)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
15-24: 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-countries-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (3)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (3)
31-34: Early return on empty payload looks good.Keeps the fast path and avoids unnecessary DB calls.
37-44: Efficient partner-field lookup.Filtering by incoming keys and projecting to name/type is appropriate.
48-56: Permission check is correct.Set-diff against contributor’s allowed fields is clear and efficient.
If unknown partner-field names appear in the payload but don’t exist in DB, they bypass both permission and type validation. Is this the intended contract (i.e., only configured PartnerFields are guarded)?
...i/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (2)
108-116: Type hints: use Callable and Any; drop builtincallable.Keeps signatures precise and mypy/ruff-friendly.
- def __collect_invalid_type_fields( - raw: Mapping[str, object], - partner_fields: Mapping[str, str], - validators: Mapping[str, callable], - ) -> List[Tuple[str, str, object]]: + def __collect_invalid_type_fields( + raw: Mapping[str, Any], + partner_fields: Mapping[str, str], + validators: Mapping[str, Callable[[Any], bool]], + ) -> List[Tuple[str, str, Any]]: @@ - invalid_fields: List[Tuple[str, str, object]] = [] + invalid_fields: List[Tuple[str, str, Any]] = []Also applies to: 112-113, 115-116
15-24: Fix numeric/object validators and annotate as ClassVar.
- Float fields should accept ints but exclude bool.
- “object” should be dict-only (no array type defined in PartnerField.TYPE_CHOICES).
- Add ClassVar to satisfy RUF012.
- TYPE_VALIDATORS = { - 'int': lambda value: isinstance(value, int) - and not isinstance(value, bool), - 'float': lambda value: isinstance(value, float) - and not isinstance(value, bool), - 'string': lambda value: isinstance(value, str), - 'object': lambda value: isinstance( - value, (dict, list) - ), - } + TYPE_VALIDATORS: ClassVar[Mapping[str, Callable[[Any], bool]]] = { + 'int': lambda value: isinstance(value, int) and not isinstance(value, bool), + 'float': lambda value: isinstance(value, (int, float)) and not isinstance(value, bool), + 'string': lambda value: isinstance(value, str), + 'object': lambda value: isinstance(value, dict), + }
🧹 Nitpick comments (3)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (2)
1-1: Import missing typing symbols (Any, Callable, ClassVar, Set).Required for fixes above and to avoid NameError.
-from typing import Dict, List, Mapping, Tuple, Iterable +from typing import Any, Callable, ClassVar, Dict, Iterable, List, Mapping, Tuple, Set
65-73: Minor: style-only nit.Line continuation with backslash for status is unnecessary; prefer parentheses. No functional change.
- event_dto.status_code = status \ - .HTTP_422_UNPROCESSABLE_ENTITY + event_dto.status_code = ( + status.HTTP_422_UNPROCESSABLE_ENTITY + )doc/release/RELEASE-NOTES.md (1)
20-23: Fix list indentation (markdownlint MD007).Adjust nested bullets under OSDEV-2065 to two-space indent.
-* [OSDEV-2065](https://opensupplyhub.atlassian.net/browse/OSDEV-2065) - Updated v1 production locations `POST/PATCH` endpoints to include partner fields: - * Added `unit` field to `PartnerField` model - * Added type validation for submitted partner fields +* [OSDEV-2065](https://opensupplyhub.atlassian.net/browse/OSDEV-2065) - Updated v1 production locations `POST/PATCH` endpoints to include partner fields: + * Added `unit` field to `PartnerField` model. + * Added type validation for submitted partner fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (3)
src/django/api/moderation_event_actions/creation/dtos/create_moderation_event_dto.py (1)
CreateModerationEventDTO(12-22)src/django/api/models/partner_field.py (1)
PartnerField(5-50)src/django/api/constants.py (1)
APIV1CommonErrorMessages(241-253)
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.12.2)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py
15-24: 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-countries-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (5)
src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (4)
58-64: LGTM: early type validation pipeline before delegating.Clear flow and short-circuiting; good.
76-91: LGTM: permission error shape and copy align with API constants.Consistent with APIV1CommonErrorMessages; message reads well.
92-107: LGTM: type error payload is actionable.Includes expected/received; good.
46-51: Make annotations Python-compat safe: replace built-in generics or enable postponed annotationsset[str] is invalid on Python <3.9 and will fail at import. Replace with typing.Set[str] (and add
from typing import Setif missing) or addfrom __future__ import annotations. Confirm the target Python for this component — repo contains Dockerfiles using python:3.7 and python:3.12.2, so runtime is not uniform.File: src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py (lines 46–51)
- contributor_allowed: set[str] = set( + contributor_allowed: Set[str] = set( event_dto.contributor.partner_fields.values_list("name", flat=True) ) - requested_partner_field_names: set[str] = set(partner_fields.keys()) + requested_partner_field_names: Set[str] = set(partner_fields.keys())doc/release/RELEASE-NOTES.md (1)
16-16: LGTM: migration entry for unit field reads clearly.Accurate, past tense.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
doc/release/RELEASE-NOTES.md (2)
16-16: Grammar fix: add missing articles and backticks placement.Use “a new … to the … model” for clarity.
-* 0180_add_unit_to_partner_field.py - This migration added new field `unit` to `PartnerField` model. +* 0180_add_unit_to_partner_field.py - This migration added a new `unit` field to the `PartnerField` model.
20-22: Align copy with PR objective and fix list indentation (MD007).
- Rephrase to explicitly mention “partner-field type validation” per OSDEV-2065.
- Reduce nested bullet indent from 4 → 2 spaces to satisfy markdownlint MD007.
-* [OSDEV-2065](https://opensupplyhub.atlassian.net/browse/OSDEV-2065) - Updated v1 production locations `POST/PATCH` endpoints to include partner fields: - * Added `unit` field to `PartnerField` model - * Added type validation for submitted partner fields +* [OSDEV-2065](https://opensupplyhub.atlassian.net/browse/OSDEV-2065) - Updated v1 production locations `POST/PATCH` endpoints to include partner‑field type validation: + * Added `unit` field to the `PartnerField` model + * Added type validation for submitted partner fields
📜 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
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: 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-flake8-linter
- GitHub Check: run-countries-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-fe-cov



[Part-1] OSDEV-2065 Update v1 production locations POST/PATCH endpoint to include partner field type validation
unitfield toPartnerFieldmodel