Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Y24-398 - scRNA Pooling scenario restriction to study-project level #4437

Merged
merged 3 commits into from
Oct 23, 2024
Merged

Y24-398 - scRNA Pooling scenario restriction to study-project level #4437

merged 3 commits into from
Oct 23, 2024

Conversation

BenTopping
Copy link
Contributor

@BenTopping BenTopping commented Oct 21, 2024 •
edited
Loading

Closes #4422

Changes proposed in this pull request

  • Updates scRNA submission templates validation for pooling restrictions to only reject on same study and project combination
  • Adds 'scrna core cells per chip well' validation to ensure they are consistent in study/project combinations.

@codecov Codecov
Copy link

codecov bot commented Oct 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (86c3df0) to head (87e277e).
Report is 14 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4437      +/-   ##
===========================================
- Coverage    86.48%   86.40%   -0.09%     
===========================================
  Files         1390     1390              
  Lines        29770    29774       +4     
===========================================
- Hits         25747    25725      -22     
- Misses        4023     4049      +26

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

@BenTopping BenTopping marked this pull request as ready for review October 21, 2024 12:09
KatyTaylor
KatyTaylor reviewed Oct 22, 2024
View reviewed changes
app/models/submission/validations_by_template_name.rb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was nice and neat to adapt :) I just spotted a problem with the original implementation... would you be up for fixing it as part of this story? It's meant to do the validation for 2 fields, 'number of samples/pool' and 'number of cells /chip well', but it only seems to do it for one of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(link to original story - #4337)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 87e277e

KatyTaylor
KatyTaylor reviewed Oct 22, 2024
View reviewed changes
spec/models/bulk_submission_spec.rb
"Inconsistent values for column 'scRNA Core Number of Samples per Pool' for " \
"Study name 'abc123_study', all rows for a specific study must have the same value"
"Inconsistent values for column 'scRNA Core Number of Samples per Pool' for Study name 'abc123_study' " \
"and Project name 'Test project', all rows for a specific study and project must have the same value"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a new test to check the new behaviour - that it DOES allow different values for different projects within the same study.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I manually tested it but will add a unit test. Ill try see if there is a nicer way to handle the CSV differences instead of making a separate file for each test.

Copy link
Contributor Author

@BenTopping BenTopping Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a line in the valid sample sheet to cover this case. Same study, different project, different samples per pool, different cells per chip well.

KatyTaylor
KatyTaylor requested changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@KatyTaylor KatyTaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. Requested a couple of changes.

KatyTaylor
KatyTaylor approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@KatyTaylor KatyTaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes.

@BenTopping BenTopping merged commit 74da0cb into develop Oct 23, 2024
22 of 23 checks passed
@BenTopping BenTopping deleted the y24-398-scrna-pooling-scenario-restriction branch October 23, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KatyTaylor KatyTaylor KatyTaylor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Y24-398 - Change pooling scenario restriction to study-project level
2 participants
@BenTopping @KatyTaylor