fix: prevent duplicate tags on leads#464
Conversation
📝 WalkthroughWalkthroughAdds a Duplicate tag prevention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/leads/views.py (1)
140-158: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
assign_tagsnow rejects valid replace requests and can delete tags before erroring.This action is documented as replacing the lead's tag set, but any request that includes an already-associated tag now returns 400. For example, if the lead already has
A, a request for[A, C]hits Line 141 first, then Lines 148-155 rejectAas a duplicate instead of keeping it and addingC. Treat existing rows as retained members of the replacement set, and only validate true duplicates in the incoming payload before any delete/create work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/leads/views.py` around lines 140 - 158, The assign_tags flow in backend/leads/views.py is treating already-associated tags as duplicates and can delete existing LeadTag rows before rejecting the request. Update the assign_tags logic so it first validates the incoming tag payload for true duplicates within the request, then treats tags already on the lead as retained members of the replacement set rather than errors, and only after validation performs the delete/create replacement work. Use the assign_tags view’s LeadTag, Tag, and existing_tag_ids handling to locate and adjust this behavior.
🧹 Nitpick comments (1)
backend/leads/models.py (1)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't keep
unique_togetherandUniqueConstraintfor the same pair.
unique_together = ('lead', 'tag')already enforces this rule, so Lines 79-80 add a second uniqueness declaration on the same columns. If you want the named constraint, replaceunique_togetherwith it instead of stacking both, and generate the matching migration to drop the legacy rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/leads/models.py` around lines 78 - 80, Remove the duplicate uniqueness declaration for the lead/tag pair in the model’s Meta definition: `unique_together` and the `UniqueConstraint` in `models.py` are enforcing the same rule twice. Keep only one approach in the model (prefer the named `UniqueConstraint` if you need the explicit constraint name) and update the related migration so the legacy `unique_together` rule is removed cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/leads/serializers.py`:
- Around line 39-51: The tag replacement flow in the lead serializer is mutating
state before validation and incorrectly rejecting valid full-replacement
payloads. Update the tag-handling logic in the serializer method that uses
LeadTag.objects so it first validates duplicates only within the incoming
tag_ids set, then applies the replace contract by deleting omitted LeadTag
relations and creating only truly new ones. Keep the existing tags that are
included in the request, and avoid raising ValidationError for tags already
assigned when they are part of the replacement payload.
In `@frontend/leads.html`:
- Around line 74-78: The disabled tag filter chip is preventing the hover
explanation from being reachable because `.tag-filter-chip.disabled` blocks
pointer events and the `title` is on the disabled element itself. Update the
`tag-filter-chip`/disabled chip markup and styling so the message is shown on a
wrapper or nearby element instead of the disabled button, keeping the
explanatory text accessible without relying on hover over the disabled control.
---
Outside diff comments:
In `@backend/leads/views.py`:
- Around line 140-158: The assign_tags flow in backend/leads/views.py is
treating already-associated tags as duplicates and can delete existing LeadTag
rows before rejecting the request. Update the assign_tags logic so it first
validates the incoming tag payload for true duplicates within the request, then
treats tags already on the lead as retained members of the replacement set
rather than errors, and only after validation performs the delete/create
replacement work. Use the assign_tags view’s LeadTag, Tag, and existing_tag_ids
handling to locate and adjust this behavior.
---
Nitpick comments:
In `@backend/leads/models.py`:
- Around line 78-80: Remove the duplicate uniqueness declaration for the
lead/tag pair in the model’s Meta definition: `unique_together` and the
`UniqueConstraint` in `models.py` are enforcing the same rule twice. Keep only
one approach in the model (prefer the named `UniqueConstraint` if you need the
explicit constraint name) and update the related migration so the legacy
`unique_together` rule is removed cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c9c6c45-3650-41b7-9d6e-330f23d69613
📒 Files selected for processing (5)
backend/leads/migrations/0006_leadtag_unique_constraint.pybackend/leads/models.pybackend/leads/serializers.pybackend/leads/views.pyfrontend/leads.html
| LeadTag.objects.filter(lead=lead).exclude(tag__in=tags).delete() | ||
| # Add new tags that are not already assigned | ||
| existing_tag_ids = set( | ||
| LeadTag.objects.filter(lead=lead).values_list('tag_id', flat=True) | ||
| ) | ||
| for tag in tags: | ||
| if tag.id not in existing_tag_ids: | ||
| LeadTag.objects.create(lead=lead, tag=tag, organization=org) | ||
| new_tags = [tag for tag in tags if tag.id not in existing_tag_ids] | ||
| duplicate_tags = [tag for tag in tags if tag.id in existing_tag_ids] | ||
| if duplicate_tags: | ||
| duplicate_names = [tag.name for tag in duplicate_tags] | ||
| raise serializers.ValidationError( | ||
| {"tag_ids": f"Tags already assigned to this lead: {', '.join(duplicate_names)}"} | ||
| ) | ||
| for tag in new_tags: | ||
| LeadTag.objects.create(lead=lead, tag=tag, organization=org) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
This breaks the "replace tags" contract and can mutate state before returning 400.
tag_ids is documented as the full replacement set, so a request like [existing_tag, new_tag] should keep the existing tag and add the new one. Instead, Line 39 deletes omitted tags first, then Lines 45-49 reject the still-present tag as a duplicate. That turns a valid replace into an error and leaves the lead partially updated. Validate only repeated IDs in the incoming payload before mutating, then delete missing relations and create only the genuinely new ones.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/leads/serializers.py` around lines 39 - 51, The tag replacement flow
in the lead serializer is mutating state before validation and incorrectly
rejecting valid full-replacement payloads. Update the tag-handling logic in the
serializer method that uses LeadTag.objects so it first validates duplicates
only within the incoming tag_ids set, then applies the replace contract by
deleting omitted LeadTag relations and creating only truly new ones. Keep the
existing tags that are included in the request, and avoid raising
ValidationError for tags already assigned when they are part of the replacement
payload.
| .tag-filter-chip.disabled { | ||
| opacity: 0.4; | ||
| cursor: not-allowed; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding template and any related markup behavior in the same file.
ast-grep outline frontend/leads.html --view expandedRepository: Kuldeeep18/LeadOrbit
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l frontend/leads.html
sed -n '60,90p' frontend/leads.html
printf '\n---\n'
sed -n '660,690p' frontend/leads.html
printf '\n---\n'
rg -n "tag-filter-chip|Already assigned to this lead|pointer-events|disabled" frontend/leads.htmlRepository: Kuldeeep18/LeadOrbit
Length of output: 3521
Move the explanation off the disabled button
title="Already assigned to this lead" won’t be reachable on the disabled chip, and .disabled { pointer-events: none; } blocks hover entirely. If that message matters, show it on a wrapper/adjacent element instead of the disabled button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/leads.html` around lines 74 - 78, The disabled tag filter chip is
preventing the hover explanation from being reachable because
`.tag-filter-chip.disabled` blocks pointer events and the `title` is on the
disabled element itself. Update the `tag-filter-chip`/disabled chip markup and
styling so the message is shown on a wrapper or nearby element instead of the
disabled button, keeping the explanatory text accessible without relying on
hover over the disabled control.
|
Hi @davi713albano-coder 👋 LeadOrbit Bot here 🤖 We noticed you've opened a Pull Request but haven't starred the repository yet. ⭐ Starring the repository is mandatory for PR review and merge. Please:
Once you've done that, the bot will continue processing your PR. Note: PRs from contributors who haven't starred the repository will remain pending until this requirement is completed. Thanks for contributing to LeadOrbit! 🚀 |
2 participants
Closes #436
What does this PR do?
Prevents the same tag from being assigned to a lead multiple times.
Changes:
How did you verify your code works?
Logic follows the implementation guide in #436. The constraint enforces uniqueness at the DB level, and the view provides a graceful error response.
Summary by CodeRabbit
Bug Fixes
Data Integrity