[OSDEV-2066] Enable POST/PATCH fields permission by attribute #737
[OSDEV-2066] Enable POST/PATCH fields permission by attribute #737roman-stolar merged 23 commits intomainfrom
Conversation
React App | Jest test suite - Code coverage reportTotal: 35.38%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
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 |
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.19%Your code coverage diff: 0.10% ▴ ✅ All code changes are covered |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/django/api/models/contributor/contributor.py (2)
133-138: Remove null=True from ManyToManyField; it’s not a valid/used optionDjango ignores null on ManyToManyField; blank=True controls emptiness. Keeping null=True is misleading and may trigger unnecessary diffs. Consider also adding a clear reverse name.
- partner_fields = models.ManyToManyField( - 'PartnerField', - blank=True, - null=True, - help_text='Partner fields that this contributor can access' - ) + partner_fields = models.ManyToManyField( + 'PartnerField', + blank=True, + related_name='contributors', + help_text='Partner fields that this contributor can access.' + )
145-145: Excluding partner_fields from simple-history is redundantsimple-history doesn’t track M2M changes; excluding this field is harmless but unnecessary. Keep if you prefer explicitness; otherwise, simplify.
- history = HistoricalRecords( - excluded_fields=['uuid', 'origin_source', 'partner_fields'] - ) + history = HistoricalRecords( + excluded_fields=['uuid', 'origin_source'] + )
📜 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/management/commands/sync_databases.py(1 hunks)src/django/api/models/contributor/contributor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
⏰ 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: get-base-branch-contricleaner-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (1)
src/django/api/management/commands/sync_databases.py (1)
97-99: Good call excluding partner_fields from sync payloadM2M fields aren’t handled in _meta.fields iteration; explicitly excluding avoids accidental handling and aligns with the intended behavior.
…66-enable-patch-permission-by-attribute # Conflicts: # doc/release/RELEASE-NOTES.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (3)
19-19: Avoid field name “type” (collides with built-in); prefer field_typePrevents shadowing and improves readability. If you must keep DB column as
type, setdb_column='type'.- ('type', models.CharField(help_text='The partner field type.', max_length=200, blank=False, null=False, choices=[('int','int'),('float','float'),('string','string'),('object','object')])), + ('field_type', models.CharField(db_column='type', help_text='The partner field type.', max_length=200, choices=[('int','int'),('float','float'),('string','string'),('object','object')])),
31-35: Add a related_name for cleaner reverse access from PartnerFieldGives you
partner_field.contributors.all()instead of the genericcontributor_set.- field=models.ManyToManyField( + field=models.ManyToManyField( blank=True, help_text='Partner fields that this contributor can access', - to='api.partnerfield' + related_name='contributors', + to='api.partnerfield' ),
9-11: Ruff RUF012 on migration class: exclude migrations or annotateMigrations commonly use mutable class attrs (
dependencies,operations). Prefer excluding migrations from RUF012 to avoid churn.Example pyproject config:
[tool.ruff] extend-exclude = ["**/migrations/**"]Also applies to: 13-37
📜 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/migrations/0178_add_partner_fields_to_contributor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
🧰 Additional context used
🪛 Ruff (0.12.2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-37: 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-contricleaner-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
src/django/api/migrations/0178_add_partner_fields_to_contributor.py
Outdated
Show resolved
Hide resolved
...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
...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: 1
♻️ Duplicate comments (2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (2)
24-25: Fix pluralization for Django Admin.Use “partner fields”.
- 'verbose_name_plural': 'partner field', + 'verbose_name_plural': 'partner fields',
17-19: Prefer UUID as PK; avoid CharField PK on name.Reduces FK size, speeds joins, and makes renames safe. Align model class accordingly.
- ('name', models.CharField(help_text='The partner field name.', max_length=200, primary_key=True, serialize=False)), - ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, help_text='Unique identifier for the partner field.', unique=True)), + ('uuid', models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False, help_text='Primary key for the partner field.')), + ('name', models.CharField(help_text='The partner field name.', max_length=200, unique=True)),If you adopt this, ensure the model at src/django/api/models/partner_field.py matches the migration (PK uuid, name unique).
🧹 Nitpick comments (3)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (3)
19-19: Right-size thetypecolumn.
max_length=200is excessive for fixed choices; trim to 16.- ('type', models.CharField(help_text='The partner field type.', max_length=200, blank=False, null=False, choices=[('int','int'),('float','float'),('string','string'),('object','object')])), + ('type', models.CharField(help_text='The partner field type.', max_length=16, blank=False, null=False, choices=[('int','int'),('float','float'),('string','string'),('object','object')])),
13-26: Seed canonical PartnerField rows (idempotent RunPython).If the permission processor expects a known set, add a follow-up migration to populate them to avoid empty-prod scenarios.
Example skeleton (adjust names/types to your canonical list):
from django.db import migrations def seed_partner_fields(apps, schema_editor): PartnerField = apps.get_model('api', 'PartnerField') data = [ # ('uuid-string-if-needed', 'name', 'type') # ('', 'number_of_workers', 'int'), # ('', 'country', 'string'), ] for _, name, type_ in data: PartnerField.objects.get_or_create(name=name, defaults={'type': type_}) class Migration(migrations.Migration): dependencies = [('api', '0178_add_partner_fields_to_contributor')] operations = [migrations.RunPython(seed_partner_fields, migrations.RunPython.noop)]Want me to draft this with your canonical fields?
9-11: Ignore RUF012 for migrations.Ruff flags class attributes in migrations; safe to suppress per-file to avoid noise.
pyproject.toml snippet:
[tool.ruff.lint.per-file-ignores] "**/migrations/*.py" = ["RUF012"]Also applies to: 13-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py(1 hunks)src/django/api/moderation_event_actions/creation/location_contribution/processors/permission_processor.py(1 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
🧬 Code graph analysis (1)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (1)
src/react/src/util/util.js (1)
fields(1558-1558)
🪛 Ruff (0.12.2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-38: 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-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-fe-code-quality
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/django/api/models/partner_field.py (5)
21-23: Fix pluralization in Meta.Use correct singular/plural; current value is singular.
Apply:
class Meta: - verbose_name_plural = "partner field" + verbose_name = "partner field" + verbose_name_plural = "partner fields"
35-41: Avoid naming the fieldtype; narrow length; wire TextChoices.
typeshadows the built‑in; rename now while still easy. Also cap length.Two options:
- Rename (recommended):
- type = models.CharField( - max_length=200, - null=False, - blank=False, - choices=TYPE_CHOICES, - help_text=('The partner field type.')) + value_type = models.CharField( + max_length=16, + choices=ValueType.choices, + help_text="The partner field type.")
- Keep name but modernize:
- type = models.CharField( - max_length=200, - null=False, - blank=False, - choices=TYPE_CHOICES, - help_text=('The partner field type.')) + type = models.CharField( + max_length=16, + choices=ValueType.choices, + help_text="The partner field type.")
30-35: Consider case‑insensitive uniqueness onname(PostgreSQL).Prevents duplicates like “Field” vs “field”.
Apply:
+from django.db.models.functions import Lower + class PartnerField(models.Model): @@ class Meta: - verbose_name_plural = "partner field" + verbose_name = "partner field" + verbose_name_plural = "partner fields" + constraints = [ + models.UniqueConstraint( + Lower("name"), + name="uniq_partnerfield_name_ci", + ), + ]Note: Requires Postgres; drop
unique=Trueonnameor keep both only if CITEXT not used.
42-43: Optional: index for housekeeping queries.If you commonly sort/filter by time, add an index on
updated_at.class Meta: @@ - verbose_name_plural = "partner field" + verbose_name = "partner field" + verbose_name_plural = "partner fields" + indexes = [ + models.Index(fields=["updated_at"], name="pf_updated_at_idx"), + ]
6-8: Tighten the docstring.Make the purpose explicit.
-""" -Partner Field that will be protected for contribution. -""" +"""Field whose contribution is gated by per‑contributor permissions."""
📜 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/models/partner_field.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
⏰ 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-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (2)
src/django/api/models/partner_field.py (2)
45-46: LGTM on str.Returning
nameis appropriate.
24-29: Confirm PartnerField migration: UUID as PK; remove any PK onname.No CreateModel("PartnerField") found in src/django/api/migrations — only AddField/AlterField results. Ensure a migration creates PartnerField with uuid primary_key=True (editable=False) and that no existing/stale migration sets
nameas the primary key; if the CreateModel exists elsewhere, point to that migration file.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (3)
24-25: Pluralization fix in verbose_name_pluralUse the proper plural so Django Admin labels are correct.
- 'verbose_name_plural': 'partner field', + 'verbose_name_plural': 'partner fields',
31-34: Remove null=True from ManyToManyField (Django ignores it)Keeps schema clean and avoids noisy future diffs.
- field=models.ManyToManyField( - blank=True, - null=True, - help_text='Partner fields that this contributor can access', - to='api.partnerfield' - ), + field=models.ManyToManyField( + blank=True, + help_text='Partner fields that this contributor can access', + to='api.partnerfield' + ),
13-26: Seed canonical PartnerField rows (idempotent RunPython) or confirm ops planIf production relies on a known set of fields, add a follow-up data migration to seed them with stable UUIDs; else confirm who creates them and when.
I can draft
0179_seed_partner_fields.pywith an idempotentRunPythonthat upserts rows byuuid. Say the word and I’ll provide it.
🧹 Nitpick comments (2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (2)
18-18: Nit: help_text should reflect PKMinor copy tweak for accuracy.
- ('uuid', models.UUIDField(default=uuid.uuid4, primary_key=True, editable=False, help_text='Unique identifier for the partner field.')), + ('uuid', models.UUIDField(default=uuid.uuid4, primary_key=True, editable=False, help_text='Primary key for the partner field.')),
9-11: Ruff RUF012 on migrations: ignore per-file rather than annotatingMigrations commonly trigger RUF012 for class attrs (
dependencies,operations). Prefer per-file ignore.# pyproject.toml (or ruff.toml) +[tool.ruff.per-file-ignores] +"**/migrations/*.py" = ["RUF012"]Also applies to: 13-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-38: 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-integration-test-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (3)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (3)
18-18: LGTM on UUID PKGood move using UUID as the PK; it avoids string-PK bloat and eases renames.
19-19: No action needed — migration choices match model constants.
Migration choices in src/django/api/migrations/0178_add_partner_fields_to_contributor.py ('int','float','string','object') exactly match PartnerField.TYPE_CHOICES in src/django/api/models/partner_field.py; no follow-up "alter field choices" migration required.
17-17: Make name non-null and drop serialize=Falseunique=True with null=True allows multiple NULLs and undermines logical uniqueness; remove serialize=False to match the model.
- ('name', models.CharField(help_text='The partner field name.', max_length=200, unique=True, null=True, serialize=False)), + ('name', models.CharField(help_text='The partner field name.', max_length=200, unique=True)),Confirm models.PartnerField.name is declared non-null to avoid a follow-up migration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (2)
31-36: Remove null=True from ManyToManyField (ignored by Django; creates churn).Django ignores null on M2M and will keep generating “remove null” migrations.
field=models.ManyToManyField( - blank=True, - null=True, + blank=True, help_text='Partner fields that this contributor can access', to='api.partnerfield' ),
24-25: Fix verbose_name_plural.Use plural for proper Admin labeling.
- 'verbose_name_plural': 'partner field', + 'verbose_name_plural': 'partner fields',
🧹 Nitpick comments (2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (2)
9-11: Silence Ruff RUF012 by annotating class vars.Migrations commonly hit RUF012; annotate dependencies/operations as ClassVar.
-from django.db import migrations, models +from django.db import migrations, models +from typing import ClassVar @@ - dependencies = [ + dependencies: ClassVar[list] = [ ('api', '0177_add_new_certifications'), ] @@ - operations = [ + operations: ClassVar[list] = [ migrations.CreateModel(Also applies to: 13-38, 1-1
17-17: Drop serialize=False from CharField.Not needed for built‑ins; avoids surprising deconstruction diffs later.
- ('name', models.CharField(help_text='The partner field name.', max_length=200, unique=True, null=False, serialize=False)), + ('name', models.CharField(help_text='The partner field name.', max_length=200, unique=True, null=False)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (1)
src/react/src/util/util.js (1)
fields(1558-1558)
🪛 Ruff (0.12.2)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-38: 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: run-dd-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (1)
src/django/api/migrations/0178_add_partner_fields_to_contributor.py (1)
13-26: ```shell
#!/bin/bash
set -euo pipefail
echo "=== show migration file (if present) ==="
MIGRATION="src/django/api/migrations/0178_add_partner_fields_to_contributor.py"
if [ -f "$MIGRATION" ]; then
sed -n '1,240p' "$MIGRATION"
else
echo "MIGRATION FILE NOT FOUND: $MIGRATION"
fiecho
echo "=== search repository for PartnerField-related patterns ==="
PATTERN='PartnerField|partner_fields|partner field|permitted.*field|allowed.*field|partner_field|class PartnerField|numberOfWorkers|country|partnerField|partner-field'
if command -v rg >/dev/null 2>&1; then
command rg -n -C2 -i "$PATTERN" || echo "NO MATCHES"
else
echo "rg not available, falling back to grep"
grep -RIn --exclude-dir=.git -n -C2 -E "$PATTERN" . || true
fiecho
echo "=== list python files under src/django/api ==="
if [ -d src/django/api ]; then
find src/django/api -type f -name "*.py" -maxdepth 4 -print || true
else
echo "Directory not found: src/django/api"
fiecho
echo "=== search for example canonical names in fixtures/tests ==="
if command -v rg >/dev/null 2>&1; then
command rg -n -i 'numberOfWorkers|country|partnerField|partner_field' || true
else
grep -RIn --exclude-dir=.git -e 'numberOfWorkers' -e 'country' -e 'partnerField' -e 'partner_field' . || true
fi</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->



OSDEV-2066 Enable POST/PATCH fields permission by attribute
Introduces the new
PartnerFieldmodel and establishes aManyToManyFieldrelationship (partner_fields) with theContributormodel. Additionally, it implements aPermission Processorto validate the contribution of Partner Fields based on attribute permission check.