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

Merging from FAIR Data Station #2017

Merged
merged 32 commits into from
Oct 18, 2024
Merged

Merging from FAIR Data Station #2017

merged 32 commits into from
Oct 18, 2024

Conversation

stuzart
Copy link
Member

@stuzart stuzart commented Oct 14, 2024

Supports updating Investigation->Study->ObsUnit->Sample->Assays with new information from FAIR Data Station.

Supports

  • Changing existing metadata
  • Moving items, e.g an Observation Unit to a different Study
  • Creating new items

It doesn't currently support deleting items, as there may be cases where things are being updated from a new template with partial information (like a PATCH). An option to delete may be added in the future.

There is some future work needed to improve error handling and reporting, to be treated separately

…tries #1981

an additional one to cover things moving is likely to be needed
currently an issue with one being saved unexpectedly though
…associations #1981

setting to true has a slightly different meaning to it being undeclared
@stuzart stuzart added this to the 1.16.0 milestone Oct 14, 2024
@stuzart stuzart requested a review from whomingbird October 15, 2024 10:33
@@ -0,0 +1,14 @@
<%= render :partial => "general/page_title",:locals=>{:title=>"Update from FAIR Data Station for #{ t('investigation') } #{link_to(@investigation.title, @investigation)}".html_safe} %>
Copy link
Contributor

Choose a reason for hiding this comment

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

use new style, not :x =>

@@ -1,4 +1,24 @@
<%= render :partial => "general/page_title",:locals=>{:title=>"Import ISA from FAIRData Station for #{ t('Project') } #{link_to(@project.title, @project)}".html_safe} %>
<%= render :partial => "general/page_title",:locals=>{:title=>"Import ISA from FAIR Data Station for #{ t('Project') } #{link_to(@project.title, @project)}".html_safe} %>
Copy link
Contributor

Choose a reason for hiding this comment

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

still the style

@@ -33,7 +53,14 @@ def reset_data_file_cache
@data_file_cache = {}
end

def build_assay(datastation_assay, contributor, policy, projects, sample, study)
def preload_data_file_cache(data_files)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why do you need to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

data files can be connected in multiple places, so may be encountered more than once. The cache is used to preven't multiple duplicates of the data files being created. For the import this is built up whilst as they are created, but for the update the cache needs preloading, otherwise new ones will start being created again.

investigation
end

def update_isa(investigation, datastation_inv, contributor, projects, policy)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the current update_isa method only supports fair_data_station resource types derived from SEEK resources. In the future, could this method be adapted for use with SEEK ISA elements as well? A good use case could be creating and updating a whole ISA through the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that once SEEK resources have an external_identifier, we’ll be one step closer to enabling resource migration between different SEEK instance

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, although thinking more in terms of supporting RO-Crates. The FAIR data station TTL is very similar to what an RO-Crate may look like.
For SEEK to SEEK migration the UUID's may be safer to use than the external identifiers, it's what they were originally added for.

Copy link
Contributor

@whomingbird whomingbird left a comment

Choose a reason for hiding this comment

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

The changes look solid, and I’ve tested them through the user interface as well. One suggestion for future work: it would be beneficial to enhance the response after 'update' or 'move' actions, so users can have a clear overview of what has been changed.

@stuzart
Copy link
Member Author

stuzart commented Oct 18, 2024

The changes look solid, and I’ve tested them through the user interface as well. One suggestion for future work: it would be beneficial to enhance the response after 'update' or 'move' actions, so users can have a clear overview of what has been changed.

thanks. Yes, the UI of the results is something I also think will be needed, but as a later task. I'm also not sure how much this will be used by a human rather than via an API.
I think also it will eventually needed to be turned into background job, but waiting to get a better idea of the likely size of the imports and updates

@stuzart stuzart merged commit afc5c7a into main Oct 18, 2024
13 checks passed
@stuzart stuzart deleted the fds-import-and-merge-1981 branch October 18, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants