-
Notifications
You must be signed in to change notification settings - Fork 2
Add superseded_calibration in Calibration. Modified related functions and tests. #769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-2026.3.0
Are you sure you want to change the base?
Changes from all commits
2d9c43f
8dfd0a2
63bc5b8
a3875f5
1ca15c6
17f920e
d935c59
04fdc59
df53220
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| """add_calibration_superseded_column | ||
|
|
||
| Revision ID: adb481b7c60b | ||
| Revises: 398067c53257 | ||
| Create Date: 2026-06-01 16:45:35.507837 | ||
|
|
||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = 'adb481b7c60b' | ||
| down_revision = 'a7f3c2e9b104' | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.add_column('score_calibrations', sa.Column('replaces_id', sa.Integer(), nullable=True)) | ||
| op.create_index(op.f('ix_score_calibrations_replaces_id'), 'score_calibrations', ['replaces_id'], unique=False) | ||
| op.create_foreign_key(None, 'score_calibrations', 'score_calibrations', ['replaces_id'], ['id']) | ||
| # ### end Alembic commands ### | ||
|
|
||
|
|
||
| def downgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.drop_constraint(None, 'score_calibrations', type_='foreignkey') | ||
| op.drop_index(op.f('ix_score_calibrations_replaces_id'), table_name='score_calibrations') | ||
| op.drop_column('score_calibrations', 'replaces_id') | ||
| # ### end Alembic commands ### |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -604,7 +604,18 @@ async def fetch_score_set_by_urn( | |
| if item.superseding_score_set and not has_permission(user, item.superseding_score_set, Action.READ).permitted: | ||
| item.superseding_score_set = None | ||
|
|
||
| item.score_calibrations = [sc for sc in item.score_calibrations if has_permission(user, sc, Action.READ).permitted] | ||
| visible_calibrations = [sc for sc in item.score_calibrations if has_permission(user, sc, Action.READ).permitted] | ||
|
|
||
| superseded_ids = [sc.superseded_calibration_id for sc in visible_calibrations if sc.superseded_calibration_id is not None] | ||
|
|
||
| available_calibrations = [sc for sc in visible_calibrations if sc.id not in superseded_ids] | ||
|
|
||
| # Solve Pydantic model validation error | ||
| for sc in available_calibrations: | ||
| sc.superseded_calibration = None | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are always null in the response now, for every caller including admins. Same loop in get_score_calibrations_for_score_set, around line 151. They're the two fields #721 adds so consumers can follow the deprecation chain, so that part of the API isn't observable. That is probably why the front end half is editor-only, since there's nothing arriving for a deprecation notice to render from. Assigning to superseding_calibration also isn't a serialization no-op, and this is an idiom that exists in the code base but I'd like to start avoiding going forward. It's the back_populates reverse side of the many-to-one, so None detaches the other calibration and marks that row's replaces_id to be nulled. Essentially if we called What's the Pydantic error the comment refers to? If it's ShorterScoreCalibration failing to validate against the ORM object I'd rather fix it there than drop the data. |
||
| sc.superseding_calibration = None | ||
|
|
||
| item.score_calibrations = available_calibrations | ||
|
|
||
| return item | ||
|
|
||
|
|
@@ -1703,7 +1714,7 @@ async def create_score_set( | |
| ) | ||
|
|
||
| created_calibration_item = await create_score_calibration( | ||
| db, calibration_create, user_data.user, variant_classes=None | ||
| db, calibration_create, user_data, variant_classes=None | ||
| ) | ||
| created_calibration_item.investigator_provided = True # necessarily true on score set creation | ||
| score_calibrations.append(created_calibration_item) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superseding_calibrationis declareduselist=False, butreplaces_idonly gets a non-unique index in the migration, so nothing stops two calibrations pointing at the same original. When that happens SQLAlchemy emitsMultiple rows returned with uselist=Falseand picks one of them arbitrarily, which makes the visibility filter inget_score_calibrations_for_score_setnondeterministic: the superseded calibration appears or disappears between requests depending on which superseding row got loaded.We should use the behavior you implemented for score sets in #706.