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.

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

4409 y24 379 study setup fields to align to the ena database fields #4542

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

wendyyang
Copy link
Contributor

@wendyyang wendyyang commented Dec 3, 2024

Closes #

Changes proposed in this pull request

Add EBI:Library Strategy as field with a combobox of options from here to the Study Setup page. This must be selected and cannot be left empty.

Add EBI: Library Source, as field with a combobox of options from here to the Study Setup page. This must be selected and cannot be left empty.

Add EBI:Library Selection, as field with a combobox of options from here to the Study Setup page. This must be selected and cannot be left empty.

Export these additional fields to the MLWH.

Remove the "ENA requirement" text from the Study Type field on Study Setup page.

Add the "ENA requirement" text to the EBI:Library Strategy, EBI: Library Source and EBI:Library Selection fields.

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

@wendyyang wendyyang marked this pull request as ready for review January 6, 2025 13:31
@wendyyang wendyyang requested a review from BenTopping January 6, 2025 13:31
Copy link
Contributor

@BenTopping BenTopping left a comment

Choose a reason for hiding this comment

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

Good start, couple suggestions about data validation and creation.

.rubocop_todo.yml Outdated Show resolved Hide resolved
@@ -286,6 +295,10 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength
allow_blank: true
}

validates :ebi_library_strategy, inclusion: { in: EBI_LIBRARY_STRATEGY_OPTIONS }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be nil or are these required for all studies? Do we need to add a conditional to these so they are just checked for ENA studies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the story states cannot be empty.

@@ -31,7 +31,10 @@ def create_study
contaminated_human_dna: 'No',
contains_human_dna: 'No',
commercially_available: 'No',
program: UatActions::StaticRecords.program
program: UatActions::StaticRecords.program,
ebi_library_strategy: 'WGS',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be here, are they mandatory for all studies?

@@ -14,6 +14,10 @@
<%= group.plain_value(:study_sra_hold) %>
<% end %>

<%= metadata_fields.plain_value(:ebi_library_strategy) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be part of the above ENA requirement group?

app/api/io/study.rb Show resolved Hide resolved
@@ -0,0 +1,9 @@
# frozen_string_literal: true

file_path = Rails.root.join('config/ena_requirement_fields.yml')
Copy link
Contributor

Choose a reason for hiding this comment

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

It is looking for ena_requirement_fields but the file name is ENA_requirement_fields.

It is probably better to use config_for rather than an initializer.

@@ -90,6 +90,10 @@ class Study < ApplicationRecord # rubocop:todo Metrics/ClassLength

DATA_RELEASE_DELAY_PERIODS = ['3 months', '6 months', '9 months', '12 months', '18 months'].freeze

EBI_LIBRARY_STRATEGY_OPTIONS = EBI_REQUIREMENT_FIELDS['EBI_Library_strategy']
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use config_for then you can use the options directly in study without having to redefine them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants