Fix/case tags - #270
Merged
Merged
Conversation
TagSerializerMixin.update() read the tags out of validated_data with an empty list as default and set them no matter what, so any partial update that did not mention them wiped them. Closing a case from the list sends exactly that, a patch with only the state, and the case lost every tag. It now only touches them when the request carries them, which is what TaggitSerializer does upstream, and asking for an empty list still empties them because that is a request that does mention them. The tests of the case did not load tests/edge.json, so no state transition was allowed and none of them could close a case; it is added to reproduce the real path.
The tags state of the case form holds whatever it was filled with: the names the api returns, until the select replaces them with its own objects. Saving always read tag.name, so a case edited without touching the select sent the string "undefined" once per tag. The case ended up tagged "undefined" and a tag with that name was created. Both submits now take the name whichever shape it is in.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates case tag handling end-to-end so that partial updates (PATCH) don’t inadvertently wipe tags, while keeping tag serialization consistent between the API (string tag names) and the UI (react-select objects). It also extends API coverage around tag behavior and updates test formatting.
Changes:
- Frontend:
FormCase.jsxnow serializes tags correctly whether the form state holds strings or tag objects. - Backend:
TagSerializerMixinupdates tags only when present in the request payload to avoid accidental tag removal on PATCH. - Tests/fixtures: adds fixture
tests/edge.json, adds PATCH tag behavior tests, and reformats some test setup for readability.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ngen/tests/api/test_cases.py | Adds PATCH tag regression tests, adds edge.json fixture, and reformats test data setup. |
| ngen/serializers/common/mixins.py | Changes tag update semantics to avoid wiping tags when omitted from partial updates. |
| frontend/src/views/case/components/FormCase.jsx | Ensures tags are appended correctly to FormData whether tags are strings or objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A form field with no value is not sent at all, and a field that is not sent is left untouched, so a case edited down to zero tags kept the ones it had: the request simply did not mention them. The form now says it out loud with an explicit empty list, which is what the tag field already parses, and there are tests for both full update paths: the one that omits the tags keeps them and the one that empties them clears them.
The tags come from a single serializer mixin, so Case and Event have to answer the same way and there was no test for either. The behaviour is written once and each resource says how to build one of its objects: read, create with and without tags, partial and full updates that send them, omit them, empty them and null them, plus the two shapes the forms send over form data (repeated fields and a comma separated string). Running them against the previous mixin fails the same three tests on both resources, one of them a 500: the field takes null and setting it crashed.
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.
This pull request improves the handling of tags in both the frontend and backend, ensuring that partial updates to cases do not unintentionally remove tags. It also adds comprehensive tests to verify correct tag behavior and makes minor formatting improvements to test files.
Tag handling improvements:
FormCase.jsx) to handle tags as either strings or objects, ensuring correct serialization when submitting forms. This prevents errors when tags are not modified in the UI. [1] [2] [3]TagSerializerMixininmixins.py) to only update tags if they are present in the request, preventing accidental tag removal during partial updates (PATCH). Also ensures that tags can be cleared explicitly.Test enhancements:
test_cases.pyto verify that:tests/edge.jsonto test fixtures for completeness.Code formatting:
test_cases.pyfor better readability and consistency. [1] [2]