Add attributes to existing sample types #1708
Conversation
kdp-cloud
commented
Jan 2, 2024
kdp-cloud
commented
- Add policies and sharing permissions to sample types
- Making existing sample types more editable
- Simplifying assays and studies creation forms.
…oesn' has samples.
…ample_type. sample_type doesn't have any attributes and fails validation at creation time.
kdp-cloud
force-pushed
the
add_attributes_to_existing_sample_types
branch
from
e20ec0d
to
2f34db2
Compare
| @@ -65,6 +73,10 @@ def update | |||
|
|
|||
| private | |||
|
|
|||
| def update_sample_json_metadata | |||
| UpdateSampleMetadataJob.new(@isa_assay.assay.sample_type).perform_now | |||
There was a problem hiding this comment.
This will run immediately and not trigger a job. should be:
UpdateSampleMetadataJob.perform_later(@isa_assay.assay.sample_type)
also it's an expensive job that will be triggered whenever the :update is called, regardless of whether the sample attributes change
| @isa_study.study.sample_types.map do |st| | ||
| UpdateSampleMetadataJob.new(st).perform_now | ||
| end |
There was a problem hiding this comment.
again, should be perform_later and will be triggered for each sample type during the :update regardless of whether they have changed
|
|
||
| private | ||
|
|
||
| def update_sample_json_metadata | ||
| # UpdateSampleMetadataJob.new(@sample_type).queue_job | ||
| UpdateSampleMetadataJob.new(@sample_type).perform_now |
There was a problem hiding this comment.
should be UpdateSampleMetadataJob.perform_later(@sample_type) and only triggered when neccessary
There was a problem hiding this comment.
since this is quite common across several controllers, I'm wondering if it should go in a module and include checks for whether sample type attributes have changed in a particular way
| queue_with_priority 1 | ||
| queue_as QueueNames::SAMPLES | ||
| def perform(sample_type) | ||
| Seek::Samples::MetadataUpdater.new(sample_type).update_metadata |
There was a problem hiding this comment.
this is going to need some profiling and scalability testing with a sample type linked to large set of samples. I'll get some metrics from fairdomhub about what is a our current maximum
| <%= form_submit_buttons(resource, { validate:false, update_sample_types: true }) %> |
There was a problem hiding this comment.
I don't see where update_sample_types param is used, and shouldn't be set to true in all cases
| @@ -18,6 +18,7 @@ | |||
| </div> | |||
|
|
|||
| <%= render partial: "projects/project_selector", locals: { resource: @sample_type } -%> | |||
| <%= render partial: 'assets/manage_specific_attributes', locals:{f:f} if show_form_manage_specific_attributes? %> | |||
There was a problem hiding this comment.
for consistency, should use assets/asset_form_bottom with the relevant options passed
| @@ -51,13 +50,11 @@ | |||
| display_isa_tag: false } %> | |||
| <% end %> | |||
|
|
|||
| <% unless @sample_type.uploaded_template? || !@sample_type.editing_constraints.allow_new_attribute? %> | |||
There was a problem hiding this comment.
check for @sample_type.uploaded_template? should remain for now, but might be revisited as part of #1766
| @@ -1715,7 +1715,9 @@ | |||
| t.text "description" | |||
| t.integer "isa_tag_id" | |||
| t.boolean "allow_cv_free_text", default: false | |||
| t.integer "template_attribute_id" | |||
There was a problem hiding this comment.
not part of the migration, looks like contamination from switching branches
| t.index ["sample_type_id"], name: "index_sample_attributes_on_sample_type_id" | ||
| t.index ["template_attribute_id"], name: "index_sample_attributes_on_template_attribute_id" |
There was a problem hiding this comment.
not part of the migration, looks like contamination from switching branches
| t.integer "parent_attribute_id" | ||
| t.index ["parent_attribute_id"], name: "index_template_attributes_on_parent_attribute_id" | ||
| t.index ["template_id", "title"], name: "index_template_id_asset_id_title" |
There was a problem hiding this comment.
not part of the migration, looks like contamination from switching branches
