diff --git a/app/graphql/irida_schema.rb b/app/graphql/irida_schema.rb index c6be445ea2..f3f38b1adb 100644 --- a/app/graphql/irida_schema.rb +++ b/app/graphql/irida_schema.rb @@ -52,7 +52,9 @@ def self.execute(query_str = nil, **kwargs) # Return a string UUID for `object` def self.id_from_object(object, _type = nil, _ctx = nil) - raise "#{object} does not implement `to_global_id`." unless object.respond_to?(:to_global_id) + unless object.respond_to?(:to_global_id) + raise GraphQL::CoercionError, "#{object} does not implement `to_global_id`." + end object.to_global_id end @@ -69,10 +71,10 @@ def self.parse_gid(global_id, ctx = {}) expected_types = Array(ctx[:expected_type]) gid = GlobalID.parse(global_id) - raise "#{global_id} is not a valid IRIDA Next ID." unless gid + raise GraphQL::CoercionError, "#{global_id} is not a valid IRIDA Next ID." if !gid || gid.app != GlobalID.app if expected_types.any? && expected_types.none? { |type| gid.model_class.ancestors.include?(type) } - raise "#{global_id} is not a valid ID for #{expected_types.join(', ')}" + raise GraphQL::CoercionError, "#{global_id} is not a valid ID for #{expected_types.join(', ')}" end gid @@ -106,4 +108,8 @@ def self.unauthorized_field(error) rescue_from(ActionPolicy::AuthorizationContextMissing) do raise GraphQL::ExecutionError, 'Unable to access object while accessing the API in guest mode' end + + rescue_from(ActiveRecord::RecordNotFound) do |exception| + raise GraphQL::CoercionError, exception + end end diff --git a/app/graphql/mutations/attach_files_to_sample.rb b/app/graphql/mutations/attach_files_to_sample.rb index 3cc24b1ff0..e871b83a29 100644 --- a/app/graphql/mutations/attach_files_to_sample.rb +++ b/app/graphql/mutations/attach_files_to_sample.rb @@ -13,31 +13,52 @@ class AttachFilesToSample < BaseMutation description: 'Persistent Unique Identifier of the sample. For example, `INXT_SAM_AAAAAAAAAA`.' validates required: { one_of: %i[sample_id sample_puid] } - field :errors, GraphQL::Types::JSON, null: false, description: 'Errors that prevented the mutation.' - field :sample, Types::SampleType, null: false, description: 'The updated sample.' - field :status, GraphQL::Types::JSON, null: false, description: 'The status of the mutation.' - - def resolve(args) - sample = if args[:sample_id] - IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample }) - else - Sample.find_by(puid: args[:sample_puid]) - end + field :errors, [Types::UserErrorType], null: false, description: 'A list of errors that prevented the mutation.' + field :sample, Types::SampleType, null: true, description: 'The updated sample.' + field :status, GraphQL::Types::JSON, null: true, description: 'The status of the mutation.' + + def resolve(args) # rubocop:disable Metrics/MethodLength + sample = get_sample_from_id_or_puid_args(args) + + if sample.nil? + return { + sample:, + status: nil, + errors: [{ path: ['sample'], message: 'not found by provided ID or PUID' }] + } + end + files_attached = Attachments::CreateService.new(current_user, sample, { files: args[:files] }).execute - status, errors = attachment_status_and_errors(files_attached) - errors['query'] = sample.errors.full_messages if sample.errors.count.positive? + status, user_errors = attachment_status_and_errors(files_attached:, file_blob_id_list: args[:files]) + + # query level errors + sample.errors.each do |error| + user_errors.append( + { + path: ['sample', error.attribute.to_s.camelize(:lower)], + message: error.message + } + ) + end { sample:, status:, - errors: + errors: user_errors } end - def attachment_status_and_errors(files_attached) - status = {} - errors = {} + def ready?(**_args) + authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] }) + end + + private + + def attachment_status_and_errors(files_attached:, file_blob_id_list:) + # initialize status hash such that all blob ids given by user are included + status = Hash[*file_blob_id_list.collect { |v| [v, nil] }.flatten] + user_errors = [] files_attached.each do |attachment| id = attachment.file.blob.signed_id @@ -45,15 +66,26 @@ def attachment_status_and_errors(files_attached) status[id] = :success else status[id] = :error - errors[id] = attachment.errors.full_messages + attachment.errors.each do |error| + user_errors.append({ path: ['attachment', id], message: error.message }) + end end end - [status, errors] + add_missing_blob_id_error(status:, user_errors:) end - def ready?(**_args) - authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] }) + def add_missing_blob_id_error(status:, user_errors:) + # any nil status is an error + status.each do |id, value| + next unless value.nil? + + status[id] = :error + user_errors.append({ path: ['blob_id', id], + message: 'Blob id could not be processed. Blob id is invalid or file is missing.' }) + end + + [status, user_errors] end end end diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb index 30cc5f7d38..ae5fed1887 100644 --- a/app/graphql/mutations/base_mutation.rb +++ b/app/graphql/mutations/base_mutation.rb @@ -9,5 +9,28 @@ class BaseMutation < GraphQL::Schema::RelayClassicMutation field_class Types::BaseField input_object_class Types::BaseInputObject object_class Types::BaseObject + + protected + + def get_sample_from_id_or_puid_args(args) + if args[:sample_id] + IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample }) + else + Sample.find_by!(puid: args[:sample_puid]) + end + rescue ActiveRecord::RecordNotFound + nil + end + + def get_project_from_id_or_puid_args(args) + if args[:project_id] + IridaSchema.object_from_id(args[:project_id], { expected_type: Project }) + else + project_namespace = Namespaces::ProjectNamespace.find_by!(puid: args[:project_puid]) + project_namespace&.project + end + rescue ActiveRecord::RecordNotFound + nil + end end end diff --git a/app/graphql/mutations/create_direct_upload.rb b/app/graphql/mutations/create_direct_upload.rb index d636965f74..47a31ffefb 100644 --- a/app/graphql/mutations/create_direct_upload.rb +++ b/app/graphql/mutations/create_direct_upload.rb @@ -5,7 +5,8 @@ module Mutations class CreateDirectUpload < BaseMutation description 'Create blob to upload data to.' - argument :byte_size, Int, 'File size (bytes)', required: true + argument :byte_size, Int, 'File size (bytes)', required: true, + validates: { numericality: { greater_than: 0 } } argument :checksum, String, 'MD5 file checksum as base64', required: true argument :content_type, String, 'File content type', required: true # rubocop:disable GraphQL/ExtractInputType argument :filename, String, 'Original file name', required: true # rubocop:disable GraphQL/ExtractInputType diff --git a/app/graphql/mutations/create_sample.rb b/app/graphql/mutations/create_sample.rb index ba9b20c52b..b2f73d430f 100644 --- a/app/graphql/mutations/create_sample.rb +++ b/app/graphql/mutations/create_sample.rb @@ -5,7 +5,7 @@ module Mutations class CreateSample < BaseMutation null true description 'Create a new sample within an existing project.' - argument :description, String, description: 'The description to give the sample.' + argument :description, String, required: false, description: 'The description to give the sample.' argument :name, String, required: true, description: 'The name to give the sample.' argument :project_id, ID, # rubocop:disable GraphQL/ExtractInputType required: false, @@ -15,37 +15,54 @@ class CreateSample < BaseMutation description: 'Persistent Unique Identifier of the project. For example, `INXT_PRJ_AAAAAAAAAA`.' validates required: { one_of: %i[project_id project_puid] } - field :errors, [String], null: false, description: 'A list of errors that prevented the mutation.' + field :errors, [Types::UserErrorType], null: false, description: 'A list of errors that prevented the mutation.' field :sample, Types::SampleType, description: 'The newly created sample.' - def resolve(args) # rubocop:disable Metrics/MethodLength - project = if args[:project_id] - IridaSchema.object_from_id(args[:project_id], { expected_type: Project }) - else - Namespaces::ProjectNamespace.find_by!(puid: args[:project_puid]).project - end - sample = Samples::CreateService.new(current_user, project, - { name: args[:name], description: args[:description] }).execute + def resolve(args) + project = get_project_from_id_or_puid_args(args) + + if project.nil? || !project.persisted? + user_errors = [{ + path: ['project'], + message: 'Project not found by provided ID or PUID' + }] + return { + sample: nil, + errors: user_errors + } + end + + create_sample(project, args) + end + + def ready?(**_args) + authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] }) + end + + private + + def create_sample(project, args) # rubocop:disable Metrics/MethodLength + sample = Samples::CreateService.new( + current_user, project, { name: args[:name], description: args[:description] } + ).execute + if sample.persisted? { sample:, errors: [] } else + user_errors = sample.errors.map do |error| + { + path: ['sample', error.attribute.to_s.camelize(:lower)], + message: error.message + } + end { sample: nil, - errors: sample.errors.full_messages + errors: user_errors } end - rescue ActiveRecord::RecordNotFound - { - sample: nil, - errors: ['Project not found by provided ID or PUID'] - } - end - - def ready?(**_args) - authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] }) end end end diff --git a/app/graphql/mutations/update_sample_metadata.rb b/app/graphql/mutations/update_sample_metadata.rb index 0ff4e89b4c..d60666e95d 100644 --- a/app/graphql/mutations/update_sample_metadata.rb +++ b/app/graphql/mutations/update_sample_metadata.rb @@ -13,22 +13,64 @@ class UpdateSampleMetadata < BaseMutation description: 'Persistent Unique Identifier of the sample. For example, `INXT_SAM_AAAAAAAAAA`.' validates required: { one_of: %i[sample_id sample_puid] } - field :errors, [String], null: false, description: 'A list of errors that prevented the mutation.' - field :sample, Types::SampleType, null: false, description: 'The updated sample.' - field :status, GraphQL::Types::JSON, null: false, description: 'The status of the mutation.' - - def resolve(args) - sample = if args[:sample_id] - IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample }) - else - Sample.find_by(puid: args[:sample_puid]) - end + field :errors, [Types::UserErrorType], description: 'A list of errors that prevented the mutation.' + field :sample, Types::SampleType, null: true, description: 'The updated sample.' + field :status, GraphQL::Types::JSON, null: true, description: 'The status of the mutation.' + + def resolve(args) # rubocop:disable Metrics/MethodLength,Metrics/AbcSize + sample = get_sample_from_id_or_puid_args(args) + + if sample.nil? + user_errors = [{ + path: ['sample'], + message: 'not found by provided ID or PUID' + }] + return { + sample:, + status: nil, + errors: user_errors + } + end + + metadata = args[:metadata] + # convert string to hash if json string as given + metadata = JSON.parse(metadata) if metadata.is_a?(String) + + unless metadata.is_a?(Hash) + user_errors = [{ + path: ['metadata'], + message: 'is not JSON data' + }] + return { + sample:, + status: nil, + errors: user_errors + } + end + metadata_changes = Samples::Metadata::UpdateService.new(sample.project, sample, current_user, - { 'metadata' => args[:metadata] }).execute + { 'metadata' => metadata }).execute + + user_errors = sample.errors.map do |error| + { + path: ['sample', error.attribute.to_s.camelize(:lower)], + message: error.message + } + end { sample:, status: metadata_changes, - errors: sample.errors.full_messages + errors: user_errors + } + rescue JSON::ParserError => e + user_errors = [{ + path: ['metadata'], + message: "JSON data is not formatted correctly. #{e.message}" + }] + { + sample: nil, + status: nil, + errors: user_errors } end diff --git a/app/graphql/schema.graphql b/app/graphql/schema.graphql index 8536ff8c0d..f2df35f363 100644 --- a/app/graphql/schema.graphql +++ b/app/graphql/schema.graphql @@ -33,19 +33,19 @@ type AttachFilesToSamplePayload { clientMutationId: String """ - Errors that prevented the mutation. + A list of errors that prevented the mutation. """ - errors: JSON! + errors: [UserError!]! """ The updated sample. """ - sample: Sample! + sample: Sample """ The status of the mutation. """ - status: JSON! + status: JSON } """ @@ -195,7 +195,7 @@ input CreateSampleInput { """ The description to give the sample. """ - description: String! + description: String """ The name to give the sample. @@ -225,7 +225,7 @@ type CreateSamplePayload { """ A list of errors that prevented the mutation. """ - errors: [String!]! + errors: [UserError!]! """ The newly created sample. @@ -1058,17 +1058,17 @@ type UpdateSampleMetadataPayload { """ A list of errors that prevented the mutation. """ - errors: [String!]! + errors: [UserError!] """ The updated sample. """ - sample: Sample! + sample: Sample """ The status of the mutation. """ - status: JSON! + status: JSON } """ @@ -1095,3 +1095,18 @@ type User implements Node { """ updatedAt: ISO8601DateTime! } + +""" +A user-readable error +""" +type UserError { + """ + A description of the error + """ + message: String! + + """ + Which input value this error came from + """ + path: [String!] +} diff --git a/app/graphql/types/user_error_type.rb b/app/graphql/types/user_error_type.rb new file mode 100644 index 0000000000..827deb4465 --- /dev/null +++ b/app/graphql/types/user_error_type.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Types + # User Type + class UserErrorType < Types::BaseObject + description 'A user-readable error' + + field :message, String, null: false, description: 'A description of the error' + field :path, [String], description: 'Which input value this error came from' + end +end diff --git a/app/services/attachments/create_service.rb b/app/services/attachments/create_service.rb index 86f143bd1c..7ada53c897 100644 --- a/app/services/attachments/create_service.rb +++ b/app/services/attachments/create_service.rb @@ -2,7 +2,7 @@ module Attachments # Service used to Create Attachments - class CreateService < BaseService + class CreateService < BaseService # rubocop:disable Metrics/ClassLength attr_accessor :attachable, :attachments, :pe_attachments def initialize(user = nil, attachable = nil, params = {}) @@ -20,6 +20,9 @@ def initialize(user = nil, attachable = nil, params = {}) rescue ActiveSupport::MessageVerifier::InvalidSignature => e @attachable.errors.add(:base, "#{e.message}: Invalid blob id") @attachments + rescue ActiveStorage::FileNotFoundError => e + @attachable.errors.add(:base, "#{e.message}: Blob is empty, no file found.") + @attachments end def execute # rubocop:disable Metrics/CyclomaticComplexity, Metrics/AbcSize, Metrics/PerceivedComplexity diff --git a/app/services/samples/metadata/update_service.rb b/app/services/samples/metadata/update_service.rb index db5d3fb459..b73d3ec336 100644 --- a/app/services/samples/metadata/update_service.rb +++ b/app/services/samples/metadata/update_service.rb @@ -13,7 +13,7 @@ def initialize(project, sample, user = nil, params = {}) @sample = sample @metadata = params['metadata'] @analysis_id = params['analysis_id'] - @metadata_changes = { added: [], updated: [], deleted: [], not_updated: [] } + @metadata_changes = { added: [], updated: [], deleted: [], not_updated: [], unchanged: [] } end def execute @@ -57,8 +57,9 @@ def validate_metadata_param end def transform_metadata_keys - # Without transforming keys, issues with overwritting can occur and multiples of the same key can appear - @metadata = @metadata.transform_keys(&:to_s) + # Without transforming and downcasing keys, + # issues with overwritting can occur and multiples of the same key can appear + @metadata = @metadata.transform_keys { |key| key.to_s.downcase } end def perform_metadata_update @@ -75,12 +76,15 @@ def perform_metadata_update end end - def assign_metadata_to_sample(key, value) # rubocop:disable Metrics/AbcSize + def assign_metadata_to_sample(key, value) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity # We don't overwrite existing @sample.metadata_provenance or @sample.metadata # that has a {source: 'analysis'} with a user if @sample.metadata_provenance.key?(key) && @analysis_id.nil? && @sample.metadata_provenance[key]['source'] == 'analysis' @metadata_changes[:not_updated] << key + elsif @sample.metadata.key?(key) && @sample.metadata[key] == value + # Do not update if the current value matches the given value + @metadata_changes[:unchanged] << key else @sample.metadata.key?(key) ? @metadata_changes[:updated] << key : @metadata_changes[:added] << key @sample.metadata_provenance[key] = diff --git a/config/routes.rb b/config/routes.rb index c815e24511..fc8b24b3a7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,8 +1,15 @@ # frozen_string_literal: true Rails.application.routes.draw do - default_url_options protocol: ENV.fetch('RAILS_PROTOCOL', 'http'), - host: ENV.fetch('RAILS_HOST', 'localhost') + if ENV.fetch('RAILS_ENV', 'development') == 'development' + default_url_options protocol: ENV.fetch('RAILS_PROTOCOL', 'http'), + host: ENV.fetch('RAILS_HOST', 'localhost'), + port: ENV.fetch('RAILS_PORT', 3000) + else + default_url_options protocol: ENV.fetch('RAILS_PROTOCOL', 'http'), + host: ENV.fetch('RAILS_HOST', 'localhost') + end + # Define your application routes per the DSL in https://guides.rubyonrails.org/routing.html # Defines the root path route ("/") diff --git a/db/migrate/20240730161412_metadata_keys_downcase.rb b/db/migrate/20240730161412_metadata_keys_downcase.rb new file mode 100644 index 0000000000..4aa520a26a --- /dev/null +++ b/db/migrate/20240730161412_metadata_keys_downcase.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +# migration to update all metadata keys to be downcased +# outputs a list of sample puid's which could not be downcased because of duplicate keys +class MetadataKeysDowncase < ActiveRecord::Migration[7.1] + def up + @fail_list = [] + migrate_sample_metadata + put_failed_migration_list + end + + def down + raise ActiveRecord::IrreversibleMigration + end + + def migrate_sample_metadata + Sample.all.each do |sample| + next if duplicate_key?(sample.metadata, sample.puid) || + duplicate_key?(sample.metadata_provenance, sample.puid) + + update_metadata(sample) + end + end + + def duplicate_key?(metadata_hash, puid) + keys = metadata_hash.map { |key, _| key.to_s.downcase } + + res = keys.uniq.length != keys.length + + @fail_list.append(puid) if res + + res + end + + def update_metadata(sample) + sample.metadata = sample.metadata.transform_keys { |key| key.to_s.downcase } + sample.metadata_provenance = sample.metadata_provenance.transform_keys { |key| key.to_s.downcase } + sample.save! + end + + def put_failed_migration_list + @fail_list.each do |puid| + puts "Metadata Downcase Migration skipped Sample #{puid}" + end + end +end diff --git a/docs-site/docs/configuration/options.md b/docs-site/docs/configuration/options.md index e36740ebfd..71e426bb5d 100644 --- a/docs-site/docs/configuration/options.md +++ b/docs-site/docs/configuration/options.md @@ -20,6 +20,7 @@ Primary ENV Variable which sets what type of environment to run | ---------------------------- | -------------------------------------------------------------------------------------------------------- | -------------------------- | | `RAILS_MAX_THREADS` | Number of threads in the thread pool | `5` | | `RAILS_HOST` | URL host for the application | `example.com` | +| `RAILS_PORT` | Port that the application runs on | `3000` *when `RAILS_ENV` is `development`* | | `RAILS_PROTOCOL` | Protocol the application uses | `http` | | `RAILS_DAILY_LOG_ROTATION` | Rotate the logs daily. For production environment only. | `false` | | `RAILS_LOG_TO_STDOUT` | Enable logging to stdout | `false` | diff --git a/test/graphql/attach_files_to_sample_test.rb b/test/graphql/attach_files_to_sample_test.rb index 19ecf05fd5..3b3cd1ed86 100644 --- a/test/graphql/attach_files_to_sample_test.rb +++ b/test/graphql/attach_files_to_sample_test.rb @@ -10,7 +10,10 @@ class AttachFilesToSampleTest < ActiveSupport::TestCase { sample{id}, status, - errors + errors{ + path + message + } } } GRAPHQL @@ -22,7 +25,10 @@ class AttachFilesToSampleTest < ActiveSupport::TestCase { sample{id}, status, - errors + errors{ + path + message + } } } GRAPHQL @@ -160,7 +166,10 @@ def setup expected_status = { blob_file.signed_id => :error } assert_equal expected_status, data['status'] assert_equal 1, data['errors'].count, 'shouldn\'t work and have errors.' - expected_error = { blob_file.signed_id => ['File checksum matches existing file'] } + expected_error = [ + { 'path' => ['attachment', blob_file.signed_id], + 'message' => 'checksum matches existing file' } + ] assert_equal expected_error, data['errors'] assert_equal :error, data['status'][blob_file.signed_id] end @@ -244,7 +253,10 @@ def setup expected_status = { blob_file_a2.signed_id => :error } assert_equal expected_status, data['status'] assert_not_empty data['sample'] - expected_error = { blob_file_a2.signed_id => ['File checksum matches existing file'] } + expected_error = [ + { 'path' => ['attachment', blob_file_a2.signed_id], + 'message' => 'checksum matches existing file' } + ] assert_equal expected_error, data['errors'] assert_equal 1, sample.attachments.count end @@ -340,20 +352,67 @@ def setup assert_equal 0, sample.attachments.count - expected_error = { 'query' => ['mismatched digest: Invalid blob id'] } + expected_error = [ + { 'path' => %w[blob_id NAN], + 'message' => 'Blob id could not be processed. Blob id is invalid or file is missing.' }, + { 'path' => %w[sample base], + 'message' => 'mismatched digest: Invalid blob id' } + ] actual_error = result['data']['attachFilesToSample']['errors'] - assert_equal actual_error, expected_error + assert_equal expected_error, actual_error + end + + test 'attachFilesToSample mutation should not work with blob missing file' do + sample = samples(:sampleJeff) + # blob_file_missing = active_storage_blobs(:attachment_attach_files_to_sample_test_blob) + blob_file_missing = ActiveStorage::Blob.create_before_direct_upload!( + filename: 'missing.file', byte_size: 404, checksum: 'Y33CgI35hFoI6p+WBXYl+A==' + ) + + assert_equal 0, sample.attachments.count + + result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { files: [blob_file_missing.signed_id], + sampleId: sample.to_global_id.to_s }) + + assert_not_nil result['data']['attachFilesToSample']['errors'], 'shouldn\'t work and have errors.' + + assert_equal 0, sample.attachments.count + + expected_error = [ + { 'path' => ['blob_id', blob_file_missing.signed_id], + 'message' => 'Blob id could not be processed. Blob id is invalid or file is missing.' }, + { 'path' => %w[sample base], + 'message' => 'ActiveStorage::FileNotFoundError: Blob is empty, no file found.' } + ] + + actual_error = result['data']['attachFilesToSample']['errors'] + + assert_equal expected_error, actual_error end test 'attachFilesToSample mutation should not work with invalid sample id' do blob_file = active_storage_blobs(:attachment_attach_files_to_sample_test_blob) - assert_raises RuntimeError do - IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION, - context: { current_user: @user, token: @api_scope_token }, - variables: { files: [blob_file.signed_id], - sampleId: 'this is not a valid sample id' }) - end + sample = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { files: [blob_file.signed_id], + sampleId: 'this is not a valid sample id' }) + expected_error = { 'message' => 'this is not a valid sample id is not a valid IRIDA Next ID.', + 'locations' => [{ 'line' => 2, 'column' => 3 }], 'path' => ['attachFilesToSample'] } + assert_equal expected_error, sample['errors'][0] + end + + test 'attachFilesToSample mutation should not work with invalid sample gid' do + blob_file = active_storage_blobs(:attachment_attach_files_to_sample_test_blob) + + result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { files: [blob_file.signed_id], + sampleId: 'gid://irida/Sample/doesnotexist' }) + expected_error = { 'message' => 'not found by provided ID or PUID', 'path' => ['sample'] } + assert_equal expected_error, result['data']['attachFilesToSample']['errors'][0] end end diff --git a/test/graphql/create_direct_upload_test.rb b/test/graphql/create_direct_upload_test.rb index 9b5e18735a..7639a2edfd 100644 --- a/test/graphql/create_direct_upload_test.rb +++ b/test/graphql/create_direct_upload_test.rb @@ -85,4 +85,34 @@ def setup assert_equal 'You are not authorized to perform this action', error_message end + + test 'createDirectUpload mutation should not work with negative bytesize' do + result = IridaSchema.execute(CREATE_DIRECT_UPLOAD_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { filename: 'dev.to', + contentType: 'image/jpeg', + checksum: 'asZ3Yzc2Q5iA5eXIgeTJndf', + byteSize: -123 }) + + assert_not_nil result['errors'], 'shouldn\'t work and have errors.' + + error_message = result['errors'][0]['message'] + + assert_equal 'byteSize must be greater than 0', error_message + end + + test 'createDirectUpload mutation should not work with 0 bytesize' do + result = IridaSchema.execute(CREATE_DIRECT_UPLOAD_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { filename: 'dev.to', + contentType: 'image/jpeg', + checksum: 'asZ3Yzc2Q5iA5eXIgeTJndf', + byteSize: 0 }) + + assert_not_nil result['errors'], 'shouldn\'t work and have errors.' + + error_message = result['errors'][0]['message'] + + assert_equal 'byteSize must be greater than 0', error_message + end end diff --git a/test/graphql/create_sample_mutation_test.rb b/test/graphql/create_sample_mutation_test.rb index 92fc7cd4cd..1a1413b2c7 100644 --- a/test/graphql/create_sample_mutation_test.rb +++ b/test/graphql/create_sample_mutation_test.rb @@ -6,7 +6,10 @@ class CreateSampleMutationTest < ActiveSupport::TestCase CREATE_SAMPLE_USING_PROJECT_ID_MUTATION = <<~GRAPHQL mutation($projectId: ID!, $name: String!, $description: String!) { createSample(input: { projectId: $projectId, name: $name, description: $description }) { - errors + errors { + path + message + } sample { id name @@ -19,7 +22,10 @@ class CreateSampleMutationTest < ActiveSupport::TestCase CREATE_SAMPLE_USING_PROJECT_PUID_MUTATION = <<~GRAPHQL mutation($projectPuid: ID!, $name: String!, $description: String!) { createSample(input: { projectPuid: $projectPuid, name: $name, description: $description }) { - errors + errors { + path + message + } sample { id name @@ -118,7 +124,8 @@ def setup assert_not_empty data['errors'] assert_nil data['sample'], 'sample should not be populated as one was not created.' - assert_equal 'Name has already been taken', data['errors'][0] + assert_equal %w[sample name], data['errors'][0]['path'] + assert_equal 'has already been taken', data['errors'][0]['message'] end test 'createSample mutation should not work with valid params and read api scope token' do @@ -187,7 +194,7 @@ def setup errors = result['data']['createSample']['errors'] - assert_equal 'Project not found by provided ID or PUID', errors[0] + assert_equal 'Project not found by provided ID or PUID', errors[0]['message'] end test 'createSample mutation should not work with invalid project id and valid api scope token' do @@ -200,10 +207,24 @@ def setup name: sample1.name, description: sample1.description }) - assert_not_nil result['data']['createSample']['errors'], 'shouldn\'t work and have errors.' + expected_error = { 'message' => 'Project not found by provided ID or PUID', 'path' => ['project'] } - errors = result['data']['createSample']['errors'] + assert_equal expected_error, result['data']['createSample']['errors'][0] + end + + test 'createSample mutation should not work with incorrectly formatted project id and valid api scope token' do + sample1 = samples(:sample1) + + result = IridaSchema.execute(CREATE_SAMPLE_USING_PROJECT_ID_MUTATION, + context: { current_user: @user, + token: @api_scope_token }, + variables: { projectId: 'project_ids_dont_look_like_this', + name: sample1.name, + description: sample1.description }) + + expected_error = { 'message' => 'project_ids_dont_look_like_this is not a valid IRIDA Next ID.', + 'locations' => [{ 'line' => 2, 'column' => 3 }], 'path' => ['createSample'] } - assert_equal 'Project not found by provided ID or PUID', errors[0] + assert_equal expected_error, result['errors'][0] end end diff --git a/test/graphql/irida_schema_test.rb b/test/graphql/irida_schema_test.rb index ecd6e74cf5..ac309b2e6d 100644 --- a/test/graphql/irida_schema_test.rb +++ b/test/graphql/irida_schema_test.rb @@ -34,7 +34,7 @@ class IridaSchemaTest < ActiveSupport::TestCase end test '#object_from_id fails if the type does not match' do - assert_raises RuntimeError do + assert_raises GraphQL::CoercionError do IridaSchema.object_from_id(groups(:group_one).to_global_id.to_s, expected_type: Project) end end @@ -50,7 +50,15 @@ class IridaSchemaTest < ActiveSupport::TestCase test '#parse_gid when gid is malformed raises an error' do global_id = 'malformed://irida/Group/12345' - assert_raises RuntimeError do + assert_raises GraphQL::CoercionError do + IridaSchema.parse_gid(global_id) + end + end + + test '#parse_gid when gid is wrongapp raises an error' do + global_id = 'gid://wrongapp/Sample/asdfqwerty' + + assert_raises GraphQL::CoercionError do IridaSchema.parse_gid(global_id) end end @@ -72,7 +80,7 @@ class IridaSchemaTest < ActiveSupport::TestCase test '#parse_gid when using expected_type rejects an unknown type' do global_id = 'gid://irida/Group/12345' - assert_raises RuntimeError do + assert_raises GraphQL::CoercionError do IridaSchema.parse_gid(global_id, expected_type: Project) end end @@ -87,7 +95,7 @@ class IridaSchemaTest < ActiveSupport::TestCase test '#parse_gid when using expected_type rejects an unknown type not present in an array of types' do global_id = 'gid://irida/Group/12345' - assert_raises RuntimeError do + assert_raises GraphQL::CoercionError do IridaSchema.parse_gid(global_id, expected_type: [User, Project]) end end diff --git a/test/graphql/update_sample_metadata_test.rb b/test/graphql/update_sample_metadata_test.rb index 84a978ca17..439eca9d4f 100644 --- a/test/graphql/update_sample_metadata_test.rb +++ b/test/graphql/update_sample_metadata_test.rb @@ -13,7 +13,10 @@ class UpdateSampleMetadataMutationTest < ActiveSupport::TestCase metadata }, status, - errors + errors { + path + message + } } } GRAPHQL @@ -28,7 +31,10 @@ class UpdateSampleMetadataMutationTest < ActiveSupport::TestCase metadata }, status, - errors + errors { + path + message + } } } GRAPHQL @@ -82,6 +88,27 @@ def setup assert_equal 'value1', data['sample']['metadata']['key1'] end + test 'updateSampleMetadata mutation should work with valid JSON String params, global id, and api scope token' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { sampleId: @sample.to_global_id.to_s, + metadata: "{ \"key1\": \"value1\" }" }) # rubocop:disable Style/StringLiterals + + assert_nil result['errors'], 'should work and have no errors.' + + data = result['data']['updateSampleMetadata'] + + assert_not_empty data, 'updateSampleMetadata should be populated when no authorization errors' + assert_empty data['errors'] + assert_not_empty data['status'] + assert_not_empty data['status'][:added] + assert_equal 'key1', data['status'][:added].first + assert_not_empty data['sample'] + assert_not_empty data['sample']['metadata'] + assert_not_empty data['sample']['metadata']['key1'] + assert_equal 'value1', data['sample']['metadata']['key1'] + end + test 'updateSampleMetadata mutation should work with valid params, puid, and api scope token with uploader access level' do # rubocop:disable Layout/LineLength user = users(:user_bot_account0) token = personal_access_tokens(:user_bot_account0_valid_pat) @@ -122,7 +149,11 @@ def setup assert_empty data['status'][:not_updated], 'status not_updated changes should be empty as the sample was not updated.' assert_not_empty data['errors'] - assert_equal I18n.t('services.samples.metadata.empty_metadata', sample_name: @sample.name), data['errors'][0] + expected_error = { + 'path' => %w[sample base], + 'message' => I18n.t('services.samples.metadata.empty_metadata', sample_name: @sample.name) + } + assert_equal expected_error, data['errors'][0] end test 'updateSampleMetadata mutation should not work with valid params and read api scope token' do @@ -169,4 +200,87 @@ def setup assert_equal 'You are not authorized to update samples for project Project 1 on this server.', error_message end + + test 'updateSampleMetadata mutation should not work with invalid sample id' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { sampleId: 'not a sample id', + metadata: { key1: 'value1' } }) + + expected_error = [ + { 'message' => 'not a sample id is not a valid IRIDA Next ID.', 'locations' => [{ 'line' => 2, 'column' => 3 }], + 'path' => ['updateSampleMetadata'] } + ] + assert_equal expected_error, result['errors'] + end + + test 'updateSampleMetadata mutation should not work with non existing sample id' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { sampleId: 'gid://irida/Sample/doesnotexist', + metadata: { key1: 'value1' } }) + + expected_error = [{ 'message' => 'not found by provided ID or PUID', 'path' => ['sample'] }] + assert_equal expected_error, result['data']['updateSampleMetadata']['errors'] + end + + test 'updateSampleMetadata mutation should not work with invalid sample puid' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_PUID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { samplePuid: 'not a sample puid', + metadata: { key1: 'value1' } }) + + assert_nil result['errors'], 'should work and have no errors.' + + data = result['data']['updateSampleMetadata'] + + assert_not_empty data, 'updateSampleMetadata should be populated when no authorization errors' + assert_not_empty data['errors'] + + expected_error = [ + 'path' => ['sample'], + 'message' => 'not found by provided ID or PUID' + ] + assert_equal expected_error, data['errors'] + end + + test 'updateSampleMetadata mutation should not work with invalid JSON formatting' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { sampleId: @sample.to_global_id.to_s, + metadata: "bad formatting" }) # rubocop:disable Style/StringLiterals,Lint/RedundantCopDisableDirective + + assert_nil result['errors'], 'should work and have no errors.' + + data = result['data']['updateSampleMetadata'] + + assert_not_empty data + assert_not_empty data['errors'] + + expected_error = [ + 'path' => ['metadata'], + 'message' => "JSON data is not formatted correctly. unexpected token at 'bad formatting'" + ] + assert_equal expected_error, data['errors'] + end + + test 'updateSampleMetadata mutation should not work with non JSON data' do + result = IridaSchema.execute(UPDATE_SAMPLE_METADATA_BY_SAMPLE_ID_MUTATION, + context: { current_user: @user, token: @api_scope_token }, + variables: { sampleId: @sample.to_global_id.to_s, + metadata: 1234 }) + + assert_nil result['errors'], 'should work and have no errors.' + + data = result['data']['updateSampleMetadata'] + + assert_not_empty data + assert_not_empty data['errors'] + + expected_error = [ + 'path' => ['metadata'], + 'message' => 'is not JSON data' + ] + assert_equal expected_error, data['errors'] + end end diff --git a/test/services/samples/metadata/fields/update_service_test.rb b/test/services/samples/metadata/fields/update_service_test.rb index a9356ec7e1..6612d4f9b8 100644 --- a/test/services/samples/metadata/fields/update_service_test.rb +++ b/test/services/samples/metadata/fields/update_service_test.rb @@ -33,7 +33,7 @@ def setup 'metadatafield3' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, @sample32.metadata_provenance) assert_equal({ added: %w[metadatafield3], updated: [], deleted: %w[metadatafield1], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield2' => 1, 'metadatafield3' => 1 }, @project29.namespace.reload.metadata_summary) assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 2, 'metadatafield3' => 1 }, @@ -59,7 +59,8 @@ def setup 'metadatafield2' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => '2000-01-01T00:00:00.000+00:00' } }, @sample32.metadata_provenance) - assert_equal({ added: [], updated: %w[metadatafield1], deleted: [], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: %w[metadatafield1], deleted: [], not_updated: [], unchanged: [] }, + metadata_changes) assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1 }, @project29.namespace.metadata_summary) assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12a.metadata_summary) assert_equal({ 'metadatafield1' => 3, 'metadatafield2' => 3 }, @group12.metadata_summary) @@ -83,7 +84,7 @@ def setup 'metadatafield3' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, @sample32.metadata_provenance) assert_equal({ added: %w[metadatafield3], updated: [], deleted: %w[metadatafield1], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield2' => 1, 'metadatafield3' => 1 }, @project29.namespace.reload.metadata_summary) assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 2, 'metadatafield3' => 1 }, diff --git a/test/services/samples/metadata/file_import_service_test.rb b/test/services/samples/metadata/file_import_service_test.rb index 185c024222..ccdee85e63 100644 --- a/test/services/samples/metadata/file_import_service_test.rb +++ b/test/services/samples/metadata/file_import_service_test.rb @@ -50,9 +50,9 @@ def setup response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] }, + updated: [], deleted: [], not_updated: [], unchanged: [] }, @sample2.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => '10', 'metadatafield2' => '20', 'metadatafield3' => '30' }, @sample1.reload.metadata) assert_equal({ 'metadatafield1' => '15', 'metadatafield2' => '25', 'metadatafield3' => '35' }, @@ -67,9 +67,9 @@ def setup response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.puid => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] }, + updated: [], deleted: [], not_updated: [], unchanged: [] }, @sample2.puid => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => '10', 'metadatafield2' => '20', 'metadatafield3' => '30' }, @sample1.reload.metadata) assert_equal({ 'metadatafield1' => '15', 'metadatafield2' => '25', 'metadatafield3' => '35' }, @@ -83,9 +83,9 @@ def setup params = { file: xls, sample_id_column: 'sample_name' } response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] }, + updated: [], deleted: [], not_updated: [], unchanged: [] }, @sample2.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => 10, 'metadatafield2' => 20, 'metadatafield3' => 30 }, @sample1.reload.metadata) assert_equal({ 'metadatafield1' => 15, 'metadatafield2' => 25, 'metadatafield3' => 35 }, @@ -99,9 +99,9 @@ def setup params = { file: xlsx, sample_id_column: 'sample_name' } response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] }, + updated: [], deleted: [], not_updated: [], unchanged: [] }, @sample2.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => 10, 'metadatafield2' => 20, 'metadatafield3' => 30 }, @sample1.reload.metadata) assert_equal({ 'metadatafield1' => 15, 'metadatafield2' => 25, 'metadatafield3' => 35 }, @@ -116,9 +116,9 @@ def setup response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] }, + updated: [], deleted: [], not_updated: [], unchanged: [] }, @sample2.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => '10', 'metadatafield2' => '20', 'metadatafield3' => '30' }, @sample1.reload.metadata) assert_equal({ 'metadatafield1' => '15', 'metadatafield2' => '25', 'metadatafield3' => '35' }, @@ -173,7 +173,7 @@ def setup params = { file: csv, sample_id_column: 'sample_name', ignore_empty_values: true } response = Samples::Metadata::FileImportService.new(project29, @john_doe, params).execute assert_equal({ sample32.name => { added: ['metadatafield3'], updated: ['metadatafield2'], - deleted: [], not_updated: [] } }, response) + deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield1' => 'value1', 'metadatafield2' => '20', 'metadatafield3' => '30' }, sample32.reload.metadata) end @@ -205,7 +205,7 @@ def setup params = { file: csv, sample_id_column: 'sample_name', ignore_empty_values: false } response = Samples::Metadata::FileImportService.new(project29, @john_doe, params).execute assert_equal({ sample32.name => { added: ['metadatafield3'], updated: ['metadatafield2'], - deleted: ['metadatafield1'], not_updated: [] } }, response) + deleted: ['metadatafield1'], not_updated: [], unchanged: [] } }, response) assert_equal({ 'metadatafield2' => '20', 'metadatafield3' => '30' }, sample32.reload.metadata) end @@ -216,7 +216,7 @@ def setup params = { file: csv, sample_id_column: 'sample_name' } response = Samples::Metadata::FileImportService.new(@project, @john_doe, params).execute assert_equal({ @sample1.name => { added: %w[metadatafield1 metadatafield2 metadatafield3], - updated: [], deleted: [], not_updated: [] } }, response) + updated: [], deleted: [], not_updated: [], unchanged: [] } }, response) assert_equal("Sample 'Project 2 Sample 1' is not found within this project", @project.errors.messages_for(:sample).first) assert_equal({ 'metadatafield1' => '10', 'metadatafield2' => '20', 'metadatafield3' => '30' }, diff --git a/test/services/samples/metadata/update_service_test.rb b/test/services/samples/metadata/update_service_test.rb index 24998c265a..2b84e64016 100644 --- a/test/services/samples/metadata/update_service_test.rb +++ b/test/services/samples/metadata/update_service_test.rb @@ -35,7 +35,7 @@ def setup 'metadatafield2' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, @sample35.metadata_provenance) assert_equal({ added: %w[metadatafield1 metadatafield2], updated: [], deleted: [], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @project31.namespace.reload.metadata_summary) assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12aa.reload.metadata_summary) @@ -59,7 +59,30 @@ def setup 'metadatafield2' => { 'id' => 2, 'source' => 'analysis', 'updated_at' => Time.current } }, @sample35.metadata_provenance) assert_equal({ added: %w[metadatafield1 metadatafield2], updated: [], deleted: [], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) + + assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @project31.namespace.reload.metadata_summary) + assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12aa.reload.metadata_summary) + assert_equal({ 'metadatafield1' => 3, 'metadatafield2' => 3 }, @subgroup12a.reload.metadata_summary) + assert_equal({ 'metadatafield1' => 4, 'metadatafield2' => 4 }, @group12.reload.metadata_summary) + end + + test 'add metadata and verify to_s and downcase' do + freeze_time + params = { 'metadata' => { 'MetaDATAfield1' => 'value1', MetaDATAfield2: 'value2' } } + + assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1 }, @project31.namespace.metadata_summary) + assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1 }, @subgroup12aa.metadata_summary) + assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12a.metadata_summary) + assert_equal({ 'metadatafield1' => 3, 'metadatafield2' => 3 }, @group12.metadata_summary) + + metadata_changes = Samples::Metadata::UpdateService.new(@project31, @sample35, @user, params).execute + assert_equal({ 'metadatafield1' => 'value1', 'metadatafield2' => 'value2' }, @sample35.metadata) + assert_equal({ 'metadatafield1' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current }, + 'metadatafield2' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, + @sample35.metadata_provenance) + assert_equal({ added: %w[metadatafield1 metadatafield2], updated: [], deleted: [], + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @project31.namespace.reload.metadata_summary) assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12aa.reload.metadata_summary) @@ -88,7 +111,7 @@ def setup 'metadatafield3' => { 'id' => 10, 'source' => 'analysis', 'updated_at' => Time.current } }, @sample33.metadata_provenance) assert_equal({ added: %w[metadatafield3], updated: %w[metadatafield1], deleted: [], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1, 'metadatafield3' => 1 }, @project30.namespace.reload.metadata_summary) @@ -116,7 +139,7 @@ def setup 'metadatafield3' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, @sample33.metadata_provenance) assert_equal({ added: %w[metadatafield3], updated: %w[metadatafield1], deleted: [], - not_updated: [] }, metadata_changes) + not_updated: [], unchanged: [] }, metadata_changes) assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1, 'metadatafield3' => 1 }, @project30.namespace.reload.metadata_summary) @@ -146,7 +169,7 @@ def setup 'metadatafield3' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => Time.current } }, @sample34.metadata_provenance) assert_equal({ added: %w[metadatafield3], updated: [], deleted: [], - not_updated: %w[metadatafield1] }, metadata_changes) + not_updated: %w[metadatafield1], unchanged: [] }, metadata_changes) assert @sample34.errors.full_messages.include?( I18n.t('services.samples.metadata.user_cannot_update_metadata', sample_name: @sample34.name, @@ -178,7 +201,8 @@ def setup { 'metadatafield1' => { 'id' => 1, 'source' => 'analysis', 'updated_at' => '2000-01-01T00:00:00.000+00:00' } }, @sample34.metadata_provenance ) - assert_equal({ added: [], updated: [], deleted: %w[metadatafield2], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: [], deleted: %w[metadatafield2], not_updated: [], unchanged: [] }, + metadata_changes) assert_equal({ 'metadatafield1' => 1 }, @project31.namespace.reload.metadata_summary) assert_equal({ 'metadatafield1' => 1 }, @subgroup12aa.reload.metadata_summary) @@ -200,7 +224,8 @@ def setup { 'metadatafield2' => { 'id' => @user.id, 'source' => 'user', 'updated_at' => '2000-01-01T00:00:00.000+00:00' } }, @sample33.metadata_provenance ) - assert_equal({ added: [], updated: [], deleted: %w[metadatafield1], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: [], deleted: %w[metadatafield1], not_updated: [], unchanged: [] }, + metadata_changes) assert_equal({ 'metadatafield2' => 1 }, @project30.namespace.reload.metadata_summary) assert_equal({ 'metadatafield2' => 1 }, @subgroup12b.reload.metadata_summary) @@ -226,7 +251,7 @@ def setup ) assert_equal( { added: %w[metadatafield3], updated: %w[metadatafield2], deleted: %w[metadatafield1], - not_updated: [] }, metadata_changes + not_updated: [], unchanged: [] }, metadata_changes ) assert_equal({ 'metadatafield2' => 1, 'metadatafield3' => 1 }, @project30.namespace.reload.metadata_summary) @@ -254,7 +279,7 @@ def setup ) assert_equal( { added: %w[metadatafield3], updated: %w[metadatafield2], deleted: %w[metadatafield1], - not_updated: [] }, metadata_changes + not_updated: [], unchanged: [] }, metadata_changes ) assert_equal({ 'metadatafield2' => 1, 'metadatafield3' => 1 }, @@ -289,12 +314,37 @@ def setup exception.result.message end + test 'unchanged metadata value when updating to current value' do + freeze_time + params = { 'metadata' => { 'metadatafield1' => 'value1' } } + + assert_equal({ 'metadatafield1' => 'value1', 'metadatafield2' => 'value2' }, @sample32.metadata) + assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1 }, @project29.namespace.metadata_summary) + assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12a.metadata_summary) + assert_equal({ 'metadatafield1' => 3, 'metadatafield2' => 3 }, @group12.metadata_summary) + + metadata_changes = Samples::Metadata::UpdateService.new(@project29, @sample32, @user, params).execute + + assert_equal({ 'metadatafield1' => 'value1', 'metadatafield2' => 'value2' }, @sample32.metadata) + # timestamps should not update as the fields are unchanged + assert_equal({ 'metadatafield1' => { 'id' => @user.id, 'source' => 'user', + 'updated_at' => '2000-01-01T00:00:00.000+00:00' }, + 'metadatafield2' => { 'id' => @user.id, 'source' => 'user', + 'updated_at' => '2000-01-01T00:00:00.000+00:00' } }, + @sample32.metadata_provenance) + assert_equal({ added: [], updated: [], deleted: [], not_updated: [], unchanged: %w[metadatafield1] }, + metadata_changes) + assert_equal({ 'metadatafield1' => 1, 'metadatafield2' => 1 }, @project29.namespace.metadata_summary) + assert_equal({ 'metadatafield1' => 2, 'metadatafield2' => 2 }, @subgroup12a.metadata_summary) + assert_equal({ 'metadatafield1' => 3, 'metadatafield2' => 3 }, @group12.metadata_summary) + end + test 'sample does not belong to project' do params = { 'metadata' => { 'metadatafield1' => 'value1', 'metadatafield2' => 'value2' } } project = projects(:projectA) metadata_changes = Samples::Metadata::UpdateService.new(project, @sample33, @user, params).execute - assert_equal({ added: [], updated: [], deleted: [], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: [], deleted: [], not_updated: [], unchanged: [] }, metadata_changes) assert @sample33.errors.full_messages.include?( I18n.t('services.samples.metadata.sample_does_not_belong_to_project', sample_name: @sample33.name, project_name: project.name) @@ -304,7 +354,7 @@ def setup test 'metadata is nil' do metadata_changes = Samples::Metadata::UpdateService.new(@project30, @sample33, @user, {}).execute - assert_equal({ added: [], updated: [], deleted: [], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: [], deleted: [], not_updated: [], unchanged: [] }, metadata_changes) assert @sample33.errors.full_messages.include?( I18n.t('services.samples.metadata.empty_metadata', sample_name: @sample33.name) ) @@ -314,7 +364,7 @@ def setup params = { 'metadata' => {} } metadata_changes = Samples::Metadata::UpdateService.new(@project30, @sample33, @user, params).execute - assert_equal({ added: [], updated: [], deleted: [], not_updated: [] }, metadata_changes) + assert_equal({ added: [], updated: [], deleted: [], not_updated: [], unchanged: [] }, metadata_changes) assert @sample33.errors.full_messages.include?( I18n.t('services.samples.metadata.empty_metadata', sample_name: @sample33.name) ) diff --git a/test/system/projects/samples_test.rb b/test/system/projects/samples_test.rb index 10ac810dbd..1b9b6e0673 100644 --- a/test/system/projects/samples_test.rb +++ b/test/system/projects/samples_test.rb @@ -1146,7 +1146,7 @@ class SamplesTest < ApplicationSystemTestCase assert_text I18n.t('projects.samples.metadata.fields.update.success') assert_no_text 'metadatafield1' - assert_text 'newMetadataKey' + assert_text 'newmetadatakey' # NOTE: downcase assert_text 'value1' end @@ -1202,7 +1202,7 @@ class SamplesTest < ApplicationSystemTestCase assert_text I18n.t('projects.samples.metadata.fields.update.success') assert_no_text 'metadatafield1' assert_no_text 'value1' - assert_text 'newMetadataKey' + assert_text 'newmetadatakey' # NOTE: downcase assert_text 'newMetadataValue' end