Y26-229 - Refactor externally managed study validations - #6007
Conversation
…pend on being externally_managed
…edundant data release study type associations modules
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6007 +/- ##
===========================================
- Coverage 83.79% 83.70% -0.10%
===========================================
Files 1297 1297
Lines 31804 31792 -12
Branches 3516 3516
===========================================
- Hits 26651 26612 -39
- Misses 4328 4362 +34
+ Partials 825 818 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # When a study is mastered in Sapio (externally_managed), all required-field | ||
| # validation errors are suppressed after validation. Sapio is the source of | ||
| # truth for these studies and will provide field values over time via updates. | ||
| after_validation :clear_externally_managed_errors, if: -> { externally_managed? } |
There was a problem hiding this comment.
I removed this after cleaning up the abstracted validations.
| context 'when a study is externally managed' do | ||
| describe '#validation' do | ||
| it 'is valid with only the study name' do | ||
| study = described_class.new(name: 'Externally Managed Study', externally_managed: true) |
There was a problem hiding this comment.
This should be sufficient to catch future regressions, e.g. someone tries to adds a validation that doesn't filter out externally_managed?
| # TODO: This is stored at study and study_metadata level, we should remove it from one of them | ||
| belongs_to :reference_genome |
There was a problem hiding this comment.
There's a note in the JSONAPI study resource that the metadata version is preferred. Probably depends on the underlying data in the DB too.
There was a problem hiding this comment.
I added the note at https://github.com/sanger/sequencescape/blob/develop/app/resources/api/v2/sapio/study_resource.rb#L20
because there are more reference genomes set in study_metadata than study.
with rgs as (
select s.name as study_name, sr.name as study_reference_genome, smr.name as metadata_reference_genome
from studies s join study_metadata sm on sm.study_id = s.id
join reference_genomes sr on s.reference_genome_id = sr.id
join reference_genomes smr on sm.reference_genome_id = smr.id
)
select 'studies' as thing, count(*) as count from rgs where coalesce(study_name, '') != ''
union select 'study_reference_genomes', count(*) from rgs where coalesce(study_reference_genome, '') != ''
union select 'metadata_reference_genomes', count(*) from rgs where coalesce(metadata_reference_genome, '') != '';thing,count
studies,8264
study_reference_genomes,410
metadata_reference_genomes,5773
There was a problem hiding this comment.
Isn't reference genome a required field? From those numbers it looks like 2000+ studies don't have one?
Maybe it's not known in advance for some projects? (ToL? Bioscan?)
…n_externally_managed is called
…_integration_hub validation
…o Y26-229-refactor-externally-managedstudy-validations
4 participants
Closes #5980
Changes proposed in this pull request
Additional context
I preferred in-class validations instead of adding a separate validator module for only externally_managed false studies as it keeps the code in one place. There are quite a few custom_attribute's that have validation that couldn't be extracted to a separate module.