feat: Inline tag creation in /inc lifecycle modals#244
Conversation
| affected_service_tags = _resolve_tag_values( | ||
| [opt["value"] for opt in affected_service_selections], | ||
| TagType.AFFECTED_SERVICE, | ||
| resolve_tags=resolve_tags, | ||
| ) | ||
|
|
||
| affected_region_selections = ( | ||
| values.get("affected_region_block", {}) | ||
| .get("affected_region_tags", {}) | ||
| .get("selected_options") | ||
| or [] | ||
| ) | ||
| affected_region_tags = [opt["value"] for opt in affected_region_selections] | ||
| affected_region_tags = _resolve_tag_values( | ||
| [opt["value"] for opt in affected_region_selections], | ||
| TagType.AFFECTED_REGION, | ||
| resolve_tags=resolve_tags, |
There was a problem hiding this comment.
Inline tag creation runs before form validation, persisting orphaned tags on rejected submissions
Because _resolve_tag_values (called at this line) issues Tag.objects.create() inside parse_incident_form_values, tags are written to the DB before validate_lifecycle_form is checked in resolved.py and mitigated.py — so if the submission is rejected (e.g. missing captain), newly created tags persist in the DB without being associated with any incident. Consider passing resolve_tags=False for the initial parse, validating, then resolving tags only on a clean form.
Evidence
resolved.py:50callsparse_incident_form_values(view)(defaultresolve_tags=True), which triggers_resolve_tag_valuesatutils.py:214–229and may issueTag.objects.create()for any__create__:-prefixed values.validate_lifecycle_form(form)is called atresolved.py:53, after the tag writes; if it returns errors the handler callsack(response_action="errors", ...)and returns, leaving created tags in the DB with no incident association.mitigated.py:52–55has the identical call ordering.update_incident.py:235callsparse_incident_form_valuesthen performs its own title check at line 244, with the same pre-write pattern.test_missing_captain_returns_errorintest_resolved.pyconfirms the rejection path exists but does not assert that no tags were written, so this case is untested.
Identified by Warden code-review · W2L-P8N
| resolved = _resolve_tag_values( | ||
| ["__create__: ", "__create__:payments"], | ||
| TagType.AFFECTED_SERVICE, | ||
| ) |
There was a problem hiding this comment.
test_recovers_existing_tag_when_create_races never exercises the race-condition recovery path
The test pre-creates "Payments" in the DB before calling _resolve_tag_values(["__create__:payments"], ...). Because name__iexact="payments" matches the pre-created "Payments", the initial Tag.objects.filter(...).first() returns the tag and name = existing.name is set, so the else branch containing Tag.objects.create is never reached. The patched IntegrityError side-effect is therefore dead code, and the assertion resolved == ["Payments"] passes via the pre-creation read path, not the race-recovery path. The intended scenario (first filter returns None, create raises IntegrityError, second filter recovers the concurrently-inserted tag) has no effective coverage.
Evidence
_resolve_tag_values(utils.py:47) callsexisting = Tag.objects.filter(type=tag_type, name__iexact=name).first()before thetry/createblock.- The test (test_new_incident.py:749) pre-creates
Tag(name="Payments"), so for input__create__:paymentsthename__iexactfilter matches immediately andname = existing.nameis set (utils.py:49), skipping theelsebranch entirely. - The
patchonTag.objects.create(test_new_incident.py:751-754) is consequently never invoked, so the patchedIntegrityErrorand the second recoveryfilterare never executed. - The success branch of the recovery (
existingfound afterIntegrityError, utils.py:62-67) thus remains untested; to exercise it the tag must be absent at first read and only inserted concurrently beforecreate.
Also found at 1 additional location
src/firetower/slack_app/handlers/utils.py:57
Identified by Warden code-review · NZW-FF6
| try: | ||
| form = parse_incident_form_values(view) |
There was a problem hiding this comment.
Tag creation runs before ack() in resolved and mitigated submission handlers
This PR adds DB writes to parse_incident_form_values via _resolve_tag_values, but handle_resolved_submission and handle_mitigated_submission still call parse_incident_form_values(view) before ack() — the same pre-ack pattern explicitly fixed here in new_incident.py. An OperationalError during tag creation (or any DB call exceeding Slack's 3-second window) will prevent ack() from ever being called, causing Slack to show a timeout error. Additionally, tags are written to the database even when validate_lifecycle_form subsequently finds errors and returns them to the user via ack(response_action="errors"), leaving orphaned approved=False tags on every failed submission.
Evidence
resolved.py:50:form = parse_incident_form_values(view)is called beforeack()at line 58; same pattern inmitigated.py:54before itsack()at line 62.utils.py_resolve_tag_values(added in this PR) callsTag.objects.filter(...).first()andTag.objects.create(...)when values carry the__create__:prefix, makingparse_incident_form_valuesa DB-writing call for the first time.new_incident.pyhunk (lines 303–304) demonstrates the correct fix:parse_incident_form_values(view)is placed inside thetryblock afterack(), so a DB failure is caught and handled rather than silently dropping the ack.validate_lifecycle_forminresolved.pyis invoked afterparse_incident_form_values, meaning tags are already written before form errors are returned to the user; no rollback occurs.
Also found at 2 additional locations
src/firetower/slack_app/tests/handlers/test_new_incident.py:19src/firetower/slack_app/handlers/utils.py:46-62
Identified by Warden find-bugs · LBG-LVU
| affected_service_tags = _resolve_tag_values( | ||
| [opt["value"] for opt in affected_service_selections], | ||
| TagType.AFFECTED_SERVICE, | ||
| resolve_tags=resolve_tags, | ||
| ) | ||
|
|
||
| affected_region_selections = ( | ||
| values.get("affected_region_block", {}) | ||
| .get("affected_region_tags", {}) | ||
| .get("selected_options") | ||
| or [] | ||
| ) | ||
| affected_region_tags = [opt["value"] for opt in affected_region_selections] | ||
| affected_region_tags = _resolve_tag_values( | ||
| [opt["value"] for opt in affected_region_selections], | ||
| TagType.AFFECTED_REGION, |
There was a problem hiding this comment.
Concurrent inline tag creation with different letter cases bypasses uniqueness guard
When two concurrent requests each create the same tag name with different casing (e.g. foo vs FOO), both can pass Tag.clean()'s case-insensitive check before either commits, and the DB-level unique_together = [("name", "type")] is case-sensitive so it won't fire — leaving two duplicate case-variant tags in the database. Use a UniqueConstraint with Upper("name") (or a CITextField) to make the uniqueness guarantee atomic at the DB level.
Evidence
Tag.unique_together = [("name", "type")]is a case-sensitive DB constraint (models.py:110).- Case-insensitive uniqueness is enforced only inside
Tag.clean(), which runs app-side before the INSERT (models.py:116-127). _resolve_tag_valuespre-checks withfilter(name__iexact=name).first(), but if two threads both getNoneand both passclean()before either commits, both INSERTs succeed because'foo'≠'FOO'for the DB constraint.- The
except (ValidationError, IntegrityError)recovery block at lines 58-63 of utils.py handlesValidationError(same-case duplicate caught byclean()) andIntegrityError(exact-case DB collision), but neither fires for different-case concurrent inserts. - Result:
Tagtable accumulates bothfooandFOOwith the sametype, breaking the case-insensitive deduplication assumption the rest of the code relies on.
Identified by Warden find-bugs · Q3L-9LV
Adds inline affected-service/region tag creation from the Slack lifecycle modals. The tag autocomplete offers a "+ Create " option (service/region only; impact_type stays curated) that get_or_creates the tag with the right type on submission, dedupes case-insensitively, handles uniqueness races, and degrades gracefully when the DB is unreachable. Closes RELENG-768.