fix: address Copilot review comments from PR #412#413
Merged
Conversation
…action on id-update; tighten test assertions) Three Copilot review comments on #412: 1. `IntegrationConfiguration.objects.get(id=...)` in validate() wasn't scoped to the integration being patched. A nonexistent id surfaced as a 500 from DoesNotExist, and an id belonging to a different integration was accepted and later repointed via update_or_create's defaults. Scope the lookup to `self.instance.configurations` and raise a clean 400 with "Configuration '...' was not found on this integration." 2. `update()` passed the full `config_data` (including any payload `action`) as `defaults` to `update_or_create`. When `id` was present that could repoint the row's action — colliding with the (integration, action) UniqueConstraint and bypassing the schema validation in validate(). Pop `action` from defaults when `id` is present so id-updates can only change `data`. 3. Tests asserted on `str(response.json())` — brittle. Assert against `response.json()["non_field_errors"][0]` instead. Added three regression tests: - unknown config id → 400 (not 500) - id belonging to a different integration → 400, original config untouched - payload `action` alongside `id` is ignored (action stays put) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses follow-up Copilot review items from PR #412 by tightening nested integration-configuration PATCH behavior: it scopes configuration id lookups to the integration being updated, prevents action from being mutated on id-based updates, and strengthens test assertions to check structured error payloads.
Changes:
- Scope configuration
idvalidation toself.instance.configurationsand return a clean 400 when the id is missing or belongs to a different integration. - Ignore payload
actionwhen updating configurations byidto prevent action-repointing and(integration, action)uniqueness collisions. - Update/add tests to cover unknown ids, cross-integration ids, and “ignore action on id update”, and tighten error assertions to
non_field_errors[0].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
cdip_admin/api/v2/serializers.py |
Scopes config-id lookup to the integration; ignores action in defaults for id-based updates. |
cdip_admin/api/v2/tests/test_integrations_api.py |
Adds regression tests for scoped id validation and immutable action-on-id-update; tightens error assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses three Copilot review comments on #412 (release-20260508 → main). Merges into
release-20260508so the fixes flow into #412 automatically.1. Scope
idlookup to the integration (was a 500/leak risk)IntegrationConfiguration.objects.get(id=...)invalidate()wasn't scoped to the integration being patched.DoesNotExist→ unhandled 500.update_or_create's defaults (i.e., one integration could mutate another's config row).Fix: scope to
self.instance.configurationsand raise a clean 400 with"Configuration '...' was not found on this integration.".2. Freeze
actionon id-updatesupdate()was passing the fullconfig_data(including any payloadaction) asdefaultstoupdate_or_create. Whenidwas present, that could repoint the row's action — colliding with the(integration, action)UniqueConstraintand bypassing the schema validation invalidate()(which keyed off the existing action).Fix: pop
actionfrom defaults whenidis present. The portal sometimes echoesactionback for client convenience — it's now correctly ignored on id-updates.3. Tighten test assertions
Replaced
assert "..." in str(response.json())withassert "..." in response.json()["non_field_errors"][0].Test plan
test_patch_integration_config_rejects_unknown_config_id— bogus id → 400 (not 500)test_patch_integration_config_rejects_id_from_different_integration— id from another integration → 400, original config untouchedtest_patch_integration_config_ignores_action_on_id_update— payloadactionalongsideidis ignored; row's action stays putpytest -k "config or create_er or create_lotek or test_update_or_create or rejects_* or ignores_*")Related
🤖 Generated with Claude Code