GraphQL: BulkUpdateSampleMetadata

[ChrisHuynh333]

What does this PR do and why?

This PR is part 2 of 2 for STRY0019725 and creates the new BulkUpdateSampleMetadata mutation that now accepts multiple sample inputs and uses the new metadata/bulk_update_service. This will allow users to update multiple samples within a namespace at once.

Mutation example:

mutation updateMetadata {
  bulkUpdateSampleMetadata(input: { 
    metadata: {
      INXT_SAM_A2EBWJDFW6: {new_metadata_1:"value1", new_metadata_2:"value2"},
      sample_name_1: {new_metadata_3:"value3", new_metadata_4:"value4"}
    },
    projectPuid:"INXT_PRJ_A2EBWJDDRH"}) {
    	overallStatus,
        status,
        errors {
          path
          message
        }
      }
}

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other pull requests.

How to set up and validate locally

Access the API through http://localhost:3000/graphiql and test the following cases using multiple samples at once:

1. Test updating metadata using project_puid, project_gid, group_puid, and group_gid.
2. Test updating metadata using sample name, gid, and puid as identifiers. Note: use stringified JSON if the identifier has special characters that need to be escaped
3. Test with invalid sample identifiers and verify appropriate errors occur
4. Test where all, partial, and none of the metadata updates will work, and verify the appropriate errors occur
5. Test overwriting metadata and verify the metadata updates and the old value is overwritten
6. Test updating metadata with the same existing value, and verify an error stating metadata is unchanged occurs.

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the PR acceptance checklist for this PR.

[deepsidhu85]

Still need to test this out but just have a few comments to start

[ericenns]

Just leaving my initial review here as it suggests some drastic changes, basically I think the status field should be more like the status from updateSampleMetadata e.g.

updateSampleMetadata:

"status": {
  "added": [],
  "updated": [],
  "deleted": [],
  "not_updated": [],
  "unchanged": [
    "new_metadata_1",
    "new_metadata_2"
  ]
},

suggested bulkUpdateSampleMedata:

"status": {
  "INXT_SAM_A2ENQXTKLT": {
    "added": [],
    "updated": [],
    "deleted": [],
    "not_updated": [],
    "unchanged": [
      "new_metadata_1",
      "new_metadata_2"
    ]
  },
  ...
},

This way a tool using the API has a clearer idea of what changes have been persisted, which is easier then parsing a status message string. This would also mean that it should not have any errors present, errors should only occur when sample could not be located, or permissions prevent update.

[ChrisHuynh333]

Just leaving my initial review here as it suggests some drastic changes, basically I think the status field should be more like the status from updateSampleMetadata e.g.

updateSampleMetadata:

"status": {
  "added": [],
  "updated": [],
  "deleted": [],
  "not_updated": [],
  "unchanged": [
    "new_metadata_1",
    "new_metadata_2"
  ]
},

suggested bulkUpdateSampleMedata:

"status": {
  "INXT_SAM_A2ENQXTKLT": {
    "added": [],
    "updated": [],
    "deleted": [],
    "not_updated": [],
    "unchanged": [
      "new_metadata_1",
      "new_metadata_2"
    ]
  },
  ...
},

This way a tool using the API has a clearer idea of what changes have been persisted, which is easier then parsing a status message string. This would also mean that it should not have any errors present, errors should only occur when sample could not be located, or permissions prevent update.

The mutation has been updated to return the full list of metadata_changes as status. Samples field has been removed as the status will now list all the samples with changes (I can put this back in if desired). As discussed, it will also return an overallStatus stating successful, successful with errors, and unsuccessful

[deepsidhu85]

Just a few more comments below

[deepsidhu85]

Lets update this description to The status of the mutation which lists the fields which were added, removed, updated, and unchanged

[ChrisHuynh333]

Updated in 5a92e1c
added not_found as that will be added in a follow up commit as discussed

[deepsidhu85]

Move the code in this block into it's own method and then here we can just do a return METHOD_NAME. The 2 blocks of code below it which also return the user errors, can be updated in a similar fashion

[ChrisHuynh333]

Thanks, I cleaned up basically anywhere that called the return hash, and made that into its own method with kwargs where we can default to nil values since most return values will be nil
5a92e1c

[JeffreyThiessen]

Looks good overall. Just one small nitpick on making the code slightly more readable.

[JeffreyThiessen]

Please update this error message to something a bit more descriptive of the error that is occurring, such as: "An error occurred while attempting to parse Sample gid with following message: #{e.message]"

[JeffreyThiessen]

Also, I think we should explicitly return nil in the error case (or its expected that nil is returned from the Rails.logger.error return, add a comment specifying that). When trying to trace the error case through the code I was unsure of how this would behave if this occurred from the original find_sample call.