Lock Sapio Study in SS by wendyyang · Pull Request #5870 · sanger/sequencescape

wendyyang · 2026-06-18T12:23:17Z

Closes #

Changes proposed in this pull request

add a new field in Study table indicate mastered_in_sapio
block editing from SS UI
add condition in study controller
add update control in study model, only integration hub or console can update sapio study
feature flag with Flipper.

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
- Check story numbers included
- Check for debug code
- Check version

codecov · 2026-06-18T12:36:24Z

Codecov Report

❌ Patch coverage is 78.84615% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.83%. Comparing base (1724e6a) to head (690cff9).
⚠️ Report is 38 commits behind head on develop.

Files with missing lines	Patch %	Lines
app/controllers/sdb/sample_manifests_controller.rb	20.00%	3 Missing and 1 partial ⚠️
app/models/study.rb	81.81%	2 Missing and 2 partials ⚠️
app/controllers/admin/studies_controller.rb	83.33%	0 Missing and 2 partials ⚠️
app/models/api_application.rb	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5870      +/-   ##
===========================================
- Coverage    84.84%   84.83%   -0.02%     
===========================================
  Files         1494     1494              
  Lines        33954    34004      +50     
  Branches      3615     3630      +15     
===========================================
+ Hits         28809    28847      +38     
- Misses        4297     4302       +5     
- Partials       848      855       +7

Flag	Coverage Δ
javascript	`76.31% <ø> (ø)`
ruby	`84.72% <78.84%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

StephenHulme · 2026-06-23T15:28:50Z

I have a few general thoughts and suggestions, rather than comments about individual lines of code:

Remove references to Sapio from the code. If these are required - like in the banner message - put them in the config file instead. This prevents our code from going out of date if Sapio (or Integration Hub) is renamed.
Rename the mastered_in_sapio field to locked (or similar). This is shorter, and more descriptive while avoiding potentially-biased language.
There is a movement in software to move away from historically problematic and non-inclusive language. Common examples of this are master -> main and black-list -> deny-list.
Focus on preventing users from altering the study in the UI, instead of putting validations in the study model. Preventing API alterations is not in the story requirements. The workarounds for the Rails Console and Integration Hub requests can then also be removed.

I hope that all makes sense, please let me know if you have any questions or would like to chat on Zoom.

wendyyang · 2026-06-24T08:20:23Z

I have a few general thoughts and suggestions, rather than comments about individual lines of code:

Remove references to Sapio from the code. If these are required - like in the banner message - put them in the config file instead. This prevents our code from going out of date if Sapio (or Integration Hub) is renamed.

good points!

Rename the mastered_in_sapio field to locked (or similar). This is shorter, and more descriptive while avoiding potentially-biased language.

May ask the team for suggestions.

There is a movement in software to move away from historically problematic and non-inclusive language. Common examples of this are master -> main and black-list -> deny-list.
3. Focus on preventing users from altering the study in the UI, instead of putting validations in the study model. Preventing API alterations is not in the story requirements. The workarounds for the Rails Console and Integration Hub requests can then also be removed.

This worths discussions.

The Acceptance criteria state:
SequenceScape users will not be able to edit ANY fields of Sapio study.
SequenceScape users will not be able to change a Sapio study's state e.g. activate it or deactivate it.
SequenceScape users cannot create a "Sapio study" in SequenceScape.

There are multiple entries to do the above actions. Implementing restrictions on controller/UI level may not be sufficient. It is easy to miss some of them. Future development may add new controllers to update study without realise the need of adding the restrictions. The consequence of that could cause the studies in Sapio and SS are not consistent. I think Implementing validation on model level provide a single point of enforcement and reduces the risk of missing an update path.

Any harm of doing model level validations? One thing I can think of is the validations may only apply for certain fields, not block all fields for update.

I hope that all makes sense, please let me know if you have any questions or would like to chat on Zoom.

…n sapio

yoldas

I have added some comments but avoided repeating the same comment everywhere.

About "SequenceScape will not send or create Study data to the MLWH database for Sapio studies.": broadcast_with_warren macro will send the study to MLWH independent of how it is updated. Something like this might cover after_commit case: Register a handler instead of using the default:

broadcast_with_warren(handler: SapioAwareStudyHandler.new)

its def <<(message) method then either ignores the message or routes it to Warren.handler << message

where message.record would be a Study.

Also to cover after_touch case, override the rebroadcast method of Study.
I can't see how the story criteria item "not accession Sapio Study data to the EBI." is prevented by this PR.

yoldas · 2026-07-03T15:22:44Z

        def validate_api_key(api_key)
-          ApiApplication.find_by!(key: api_key)
+          api_application = ApiApplication.find_by!(key: api_key)
+          Current.api_application = api_application


praise: this is very useful for the request scope.

note: We might have add an ApiApplication.integration_hub? to avoid checking against strings everywhere: Current.api_application.integration_hub?

StephenHulme

Lots of work's gone into this and it's looking good overall : )
I've added some suggestions around functions and accessioning.

StephenHulme · 2026-07-06T10:43:07Z

+# so that it can be accessed in models and other places where the controller context is not available.
+
+class Current < ActiveSupport::CurrentAttributes
+  attribute :api_application


I would very much like to remove the reliance on CurrentAttributes if at all possible. This is because it blurs the line between controller and model making maintenance much harder in the future.

StephenHulme · 2026-07-06T10:45:19Z

+
+  # This validation prevents any updates to a study that is managed in SAPIO
+  # unless the request is coming from Integration Hub
+  def prevent_updates_when_mastered_in_sapio


Could we change this to allow updates from the api? This solves several naming concerns and also removes the need to check against the request coming from the integration hub itself.

Will Integration hub be the only user to use SS study API? What about other applications/users trying to call SS API, to update the study? How to prevent those cases if not checking if the request from Integration hub?

Most likely, I think. Although there is a case to be made for principle of least priviledge, but Sequencescape doesn't currently support authorisation, compared to authentication.

StephenHulme · 2026-07-06T10:47:33Z

 <% add :menu, "Study QC Reports" => study_reports_study_path(@study) -%>

-<% if permitted_to_accession?(@study) & @study.samples_accessionable? %>
+<% if permitted_to_accession?(@study) & @study.samples_accessionable? && !@study.mastered_in_sapio %>


I think the feature-flag check could be added to def samples_accessionable?, simplifying the code here and propagating to the rest of the accessioning functionality?

andrewsparkes

Looks really good!

Suggestion: For UAT ask the SSRs to test it, see if they can find any way to update the study or the samples in it that you might have missed!

andrewsparkes · 2026-07-23T10:24:13Z

  def new
+    @study = Study.find_by(id: params[:study_id])
+
+    if @study&.ui_locked?


So to be clear, if the Study is now in Sapio the assumption is all Samples will be entered via Sapio too?

And edited there, instead of via sample manifest re-uploads in SS?

add banner for sapio study

888659a

wendyyang added 13 commits June 22, 2026 12:28

add model level control study update

49ba606

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

dd25b9f

add tests

6a65664

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

19fd034

add more tests

09d501d

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

a82066a

add feature flag

cf1bbb6

fix linting

3eb0f18

hide the update button for sapio study

2247cea

add more tests

2bfe363

add only integration hub can update mastered_in_sapio field

de3439e

add more tests

789532a

fix linting

132c118

wendyyang marked this pull request as ready for review June 22, 2026 22:03

wendyyang requested review from KatyTaylor and StephenHulme June 23, 2026 08:00

wendyyang added 9 commits June 24, 2026 16:30

minor changes to the banner and add comments

c3920f1

refactor and move the Sapio warning message to config file

a6f15e5

fix the test

795631b

fix the test

a4044e0

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

b73d5cc

add restriction on sample manifest creation if the study is managed i…

0d45dd5

…n sapio

fix test

b40b595

fix test

4e9e0d3

fix the tests

01b80ba

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

9def8ce

yoldas reviewed Jul 3, 2026

View reviewed changes

StephenHulme reviewed Jul 6, 2026

View reviewed changes

wendyyang added 16 commits July 6, 2026 13:20

update the error/wanrings key names

7af03c7

fix the test

fe53f9c

extract sapio_restrictions_enabled? method to avoid repetion

aa61856

fix tests

aeb35c7

refactor study controller

9c515ea

refactor sample manifest validation

d493e5b

fix the tests

d871d3a

fix test

6357ec3

update the UI locked condition in view

4ab6361

update the view to include feature flag enable condition

7958e66

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

6e50e72

add restriction to sample accessioning

5d39bae

add accessioning restriction for lock study

a13c215

remove restriction on view as it is done in model

6ea6539

merge develop in

aa7aa9a

fix linting

d02e6eb

wendyyang mentioned this pull request Jul 7, 2026

Y26-209 - Disable MLWH Broadcasts for Sapio-managed Studies #5903

Closed

1 task

wendyyang added 6 commits July 8, 2026 08:53

remove the current attribute

9faf1fa

fix linting

c383b04

fix the test

589acbf

prevent create sample manifest from sapio study

fea921b

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

473968c

fix test

e181ec8

wendyyang requested review from StephenHulme and yoldas July 9, 2026 19:52

Merge branch 'develop' into 5824-y26-172---lock-sapio-mastered-studies

690cff9

andrewsparkes approved these changes Jul 23, 2026

View reviewed changes

Uh oh!

Conversation

wendyyang commented Jun 18, 2026 • edited by StephenHulme Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Instructions for Reviewers

Uh oh!

codecov Bot commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

StephenHulme commented Jun 23, 2026

Uh oh!

wendyyang commented Jun 24, 2026

Uh oh!

yoldas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephenHulme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewsparkes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wendyyang commented Jun 18, 2026 •

edited by StephenHulme

Loading

codecov Bot commented Jun 18, 2026 •

edited

Loading

yoldas left a comment •

edited

Loading