Fix concept reassignment discarding and recreating unchanged BC entities - #323
Merged
Merged
Conversation
Reassigning the same concept code(s) to an activity deleted every activity_concept row and recreated it with a fresh UID, even when nothing actually changed. Because the next UID was computed as MAX(live rows)+1 immediately after the activity's own row was deleted, this could reissue the exact number just freed, and the subsequent orphan cleanup then deleted the freshly recreated biomedical_concept row (and its properties), leaving the activity's concept assignment pointing at nothing. - set_activity_concepts now diffs old vs. new codes and only touches what actually changed; unchanged codes keep their row and UID untouched. - Added a durable uid_counter table and moved all 11 get_next_*_uid generators onto it so a freed number is never reissued, even when rows are genuinely removed and re-added within one request. - Guarded _cleanup_orphaned_concept_rows against deleting a UID that was reissued to a live row. - Removed a dead, unreachable duplicate of set_activity_concepts in app.py (shadowed by the router version) that carried the same bug and never scheduled the biomedical-concept-property background population task at all; the UI form handler now calls the fixed router implementation directly.
extra_skip stripped freeze_id out of the INSERT column list before col_overrides could apply its sentinel value, so the column fell back to SQLite's NULL default instead of the intended 0.
The background enrichment task only wrote the fetched name back to biomedical_concept.name/label, never to the denormalized activity_concept.concept_title snapshot shown in the UI. When the initial assignment-time lookup missed, that snapshot stayed stuck on the raw concept code even after enrichment succeeded.
Owner
Author
|
@claude tests failed on the latest push |
The action was defaulting to claude-opus-5[1m], a model/beta our ANTHROPIC_API_KEY isn't entitled to, causing every Claude-triggered workflow to fail instantly (is_error:true, 0 cost, 1 turn).
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
Reassigning the same Biomedical Concept code(s) to an activity via
POST /soa/{soa_id}/activities/{activity_id}/conceptsdeleted everyactivity_conceptrow for that activity and recreated it with a fresh UID — even when nothing had actually changed. Because the next UID was computed asMAX(live rows)+1immediately after the activity's own row was deleted, this could reissue the exact number that was just freed. The subsequent orphan-cleanup step then unconditionally deleted thebiomedical_conceptrow for that UID, wiping out the freshly recreated entity (and its properties) moments after creation, and leaving the activity's concept assignment pointing at nothing.This surfaced while investigating a real report: after reassigning concept
C114209to a study's RANDOMIZATION activity, its USDM export showed the Biomedical Concept with no properties. Root-caused, reproduced, and fixed against the live data — confirmed the fix makes an unchanged reassignment a byte-for-byte no-op (sameactivity_concept.idandbiomedical_concept.idbefore/after).Changes
set_activity_conceptsnow reconciles instead of replacing (src/soa_builder/web/routers/activities.py): diffs the requested codes against the activity's current assignment. Codes that are unchanged keep their row and UID completely untouched — no delete, no reinsert, no new UID, no re-triggered CDISC Library enrichment/property-population background tasks. Only genuinely added/removed codes are touched.uid_countertable (new migration inmigrate_database.py): all 11get_next_*_uidgenerators inutils.pynow draw from a persistent per-(soa_id, prefix)counter instead of recomputingMAX(live rows)+1on every call, so a freed number is never reissued — even when rows are genuinely removed and re-added within the same request. Each generator lazily seeds the counter once from its original max-scan logic, so existing SoAs pick up exactly where their current numbering left off._cleanup_orphaned_concept_rows(app.py): won't delete abiomedical_conceptrow that's still referenced by a liveactivity_conceptrow — defense-in-depth alongside the two fixes above.set_activity_conceptsinapp.py(shadowed at the HTTP layer by the router version, confirmed via routing order + live testing). It carried the same bug and never scheduled the biomedical-concept-property background population task at all — so concepts assigned via the HTML form UI (ui_set_activity_concepts, which called this dead copy directly) never got their properties populated at all. The UI form handler now calls the fixed router implementation directly.Test plan
pytest— full suite passes (668 tests), including two new regression tests: reassigning an unchanged code is a true no-op, and adding+removing codes in one call preserves the kept code's UID.status_code in (200, 422)(i.e. never actually verified success).flake8clean on all changed files.uid_countermigration is idempotent (created once, no errors, no duplicate rows on a second run against a DB copy).C114209to the RANDOMIZATION activity is now a no-op (activity_concept.idandbiomedical_concept.ididentical before/after), with its 3 properties intact.🤖 Generated with Claude Code