From 059eb3ddb3bfb7700e95e77eed3b9712ec658746 Mon Sep 17 00:00:00 2001 From: Josh Adam Date: Wed, 8 Jan 2025 07:55:16 -0600 Subject: [PATCH] Enhancement: Edit line list metadata in place (#864) * feat: :sparkles: Initial work on editing line list * feat: Add check_editable route to project for enhanced editing capabilities * feat: Enhance sample editing functionality with update_field action and form improvements - Added a new `update_field` action in the SamplesController to handle field updates. - Updated the `check_editable` method to pass the current value to the edit form. - Modified the `_edit_field_form` partial to include hidden fields for original value and format. - Updated routes to include the new `update_field` patch route for samples. - Improved the HTML structure of the edit field form for better integration with Turbo Streams. * feat: Implement editable cell component for enhanced inline editing of sample fields - Added `EditableCell` component to streamline the rendering of editable table cells. - Refactored `table_component.html.erb` to utilize the new `EditableCell` for cleaner code and improved maintainability. - Updated `samples_controller.rb` to render Turbo Stream updates for editable fields. - Introduced `inline_edit_controller.js` to manage focus and submission of inline edit forms. - Enhanced `_edit_field_form.html.erb` to integrate with the new inline editing functionality. - Created `_editable_table_field.html.erb` partial to encapsulate editable cell rendering logic. * feat: Refactor sample metadata update logic in SamplesController - Replaced direct update call with a new service object for handling metadata updates. - Enhanced the update process to better manage field updates and original values. - Improved code readability and maintainability by encapsulating update logic within a dedicated service. * feat: Refactor update_field action in SamplesController for improved error handling and code organization - Introduced private methods to encapsulate logic for setting field variables and updating sample fields. - Enhanced error handling by rendering appropriate responses based on sample validation errors. - Improved code readability and maintainability by organizing the update process into dedicated methods. * feat: Enhance editable cell functionality and refactor update_field action - Updated the styling of the submit button in the EditableCell component for improved visual consistency. - Refactored the update_field action in SamplesController to streamline the update process using a single service call. - Improved the HTML structure of the edit field form to enhance integration with Turbo Streams and maintain a consistent user experience. * feat: Add test for editing sample fields in samples_test.rb - Introduced a new test case to verify the functionality of editing a sample field. - The test navigates to the sample's edit page, interacts with the metadata toggle, and checks for the correct display of the sample's metadata. - Ensures that the edit field button is functional and properly integrated with the existing sample editing features. * feat: Implement inline editing for sample fields with check_editable and update_field actions - Added `check_editable` method in SamplesController to verify field editability and render appropriate forms. - Introduced `update_field` action to handle updates for sample fields, utilizing a dedicated service for processing. - Created `_edit_field_form.html.erb` partial for inline editing, including necessary hidden fields for seamless updates. - Added `_editable_table_field.html.erb` partial to encapsulate editable cell rendering logic. - Updated routes to include new paths for `check_editable` and `update_field` actions. - Enhanced `_table.html.erb` to integrate the new editable functionality, improving user experience with Turbo Streams. * fix: Update sample field test to assert correct button interaction and field visibility - Modified the test in `samples_test.rb` to assert the presence of the sample's metadata value directly. - Changed the interaction from clicking an edit field button to clicking a button that directly represents the sample's value, enhancing the test's accuracy and clarity. - Added an assertion to verify that the field is correctly displayed after the interaction, ensuring the test covers the expected behavior of the inline editing feature. * feat: Enhance inline editing functionality with cancel support and original value handling - Added `original` value support in `inline_edit_controller.js` to store the initial input value. - Implemented `cancel` method to revert changes on Escape key press, improving user experience. - Updated `_edit_field_form.html.erb` and `_edit_field_form.html.erb` to pass original value to the inline edit controller. - Modified input field actions to include cancel functionality, allowing users to easily discard edits. * feat: Add field_editable? method to Sample model for user metadata validation - Introduced `field_editable?` method to determine if a field is editable based on its provenance. - Enhanced the model's capability to manage user-specific metadata, improving the inline editing functionality. * refactor: Update sample field test for inline editing functionality - Renamed test to clarify it checks inline editing of a sample metadata field. - Added assertions to verify the presence of metadata toggle label and correct sample values in the table. - Enhanced test coverage for inline editing interactions, ensuring accurate representation of sample data. * refactor: Simplify editable cell functionality and remove unused actions - Updated `EditableCell` component to remove the `check_editable_url` parameter, streamlining the initialization process. - Refactored `SamplesController` to eliminate `check_editable` and `update_field` actions, consolidating field update logic into the `metadata/fields_controller`. - Introduced new `editable` and `update_value` actions in `fields_controller` to handle inline editing more effectively. - Removed obsolete partials related to inline editing and updated views to reflect the new structure. - Enhanced Turbo Stream responses for editable fields, improving user experience during inline edits. * fix: Update sample field test to assert button presence for inline editing - Modified assertions in `samples_test.rb` to check for button elements instead of table cell text for sample metadata values. - Ensured the test accurately reflects the current inline editing functionality by verifying the presence of buttons for sample values. * refactor: Streamline field update logic and improve rendering methods * refactor: Remove unused hidden field for sample ID in editing field cell * test: Add tests for unchanged field rendering and sample metadata updates * test: Add test for editing a metadata field and update unchanged field response * fix: Update response status for editable field checks and add test for analysis field restriction * test: Add test for building update parameters in FieldsController * test: Update sample table assertions to check input values instead of text * test: Add test for updating metadata value not from an analysis * feat: Enhance metadata field handling and submission logic * test: Refactor sample table interaction and update value submission logic * test: Add check to prevent updating metadata value from an analysis * test: Update metadata value handling in sample tests * test: Update table cell index in sample tests for accurate element selection * feat: Add success flash message upon successful field update in metadata * feat: Add autofocus option to editable cell for improved user experience * feat: Refactor editable cell to use button for submission and improve accessibility * feat: Improve button accessibility by updating aria-label and maintaining autofocus option * refactor: Simplify field rendering logic and improve error handling in fields controller * fix: Update error message to display specific validation error for non-editable fields * feat: Enhance editable field handling by adding non-editable field error message and create metadata field functionality * feat: Add success message for metadata updates and refactor button interactions in sample fields * feat: Implement editable cell component with button interactions and update tests for button selectors * fix: Remove unnecessary class from editable cell and adjust z-index for sticky table headers * test: Refactor sample tests to include setup, actions, and verification sections for clarity * test: Refactor sample tests to include setup, actions, and verification sections for better clarity * test: Add new test to ensure project analysts cannot edit samples, enhancing access control verification * feat: Enhance sample table rendering by conditionally displaying editable cells based on user permissions, improving user experience and access control * refactor: Simplify editable cell initialization and update table rendering logic to enhance user experience by conditionally rendering editable cells based on row actions * test: Test to ensure need permissions to edit metadata * test: Analysts should not be able to edit metadata * test: Remove redundant label click assertion in sample metadata tests to streamline test flow * refactor: Clean up HTML class attributes in table component for improved readability * test: Remove outdated test for metadata update restriction to streamline test suite * fix: Update metadata access methods to use field? for consistency * test: Remove unnecessary assertions in sample metadata tests to streamline test flow * fix: Update sample since the other one went away :( * test: Simplify sample metadata toggle interaction in system tests * test: Add assertion for the number of table headers in sample metadata toggle * refactor: Disable RuboCop class length metric for FieldsController * test: Add assertion for updated table header count in sample metadata toggle * test: Update sample metadata toggle test to use specific project and subgroup * test: Add assertion for the presence of table headers in sample metadata toggle * fix: Use fetch method for safer metadata access in metadata_value * fix: Use fetch method for safer access to sample metadata in editable_cell * fix: Improve updatable_field? method to handle missing metadata provenance * fix: Simplify condition for rendering editable fields in fields_controller * test: Add unit tests for field? and updatable_field? methods in Sample model * test: Update sample tests to assert table header count after metadata toggle * feat: Refactor timestamp handling in sample forms to use a shared partial * fix: Update timestamp handling in fields_controller and timestamp_input partial * fix: Adjust timestamp handling in fields_controller to prevent timing issues * fix: Update timestamp assignment in fields_controller to use sample's updated_at --- .../samplesheet/metadata_cell_component.rb | 2 +- app/components/samples/editable_cell.html.erb | 29 +++++ app/components/samples/editable_cell.rb | 12 ++ .../samples/table_component.html.erb | 33 ++--- .../samples/metadata/fields_controller.rb | 117 +++++++++++++++++- .../controllers/inline_edit_controller.js | 28 +++++ app/models/sample.rb | 10 ++ .../samples/metadata/update_service.rb | 6 +- app/views/groups/samples/index.html.erb | 11 +- app/views/projects/samples/index.html.erb | 11 +- app/views/shared/_flash.html.erb | 3 + .../shared/samples/_timestamp_input.html.erb | 7 ++ .../fields/_editable_field_cell.html.erb | 5 + .../fields/_editing_field_cell.html.erb | 16 +++ config/locales/en.yml | 4 + config/locales/fr.yml | 4 + config/routes/project.rb | 5 +- .../metadata/fields/fields_controller_test.rb | 64 ++++++++++ test/models/sample_test.rb | 27 ++++ test/system/groups/samples_test.rb | 71 +++++++++-- test/system/projects/samples_test.rb | 93 ++++++++++++++ 21 files changed, 504 insertions(+), 54 deletions(-) create mode 100644 app/components/samples/editable_cell.html.erb create mode 100644 app/components/samples/editable_cell.rb create mode 100644 app/javascript/controllers/inline_edit_controller.js create mode 100644 app/views/shared/_flash.html.erb create mode 100644 app/views/shared/samples/_timestamp_input.html.erb create mode 100644 app/views/shared/samples/metadata/fields/_editable_field_cell.html.erb create mode 100644 app/views/shared/samples/metadata/fields/_editing_field_cell.html.erb diff --git a/app/components/nextflow/samplesheet/metadata_cell_component.rb b/app/components/nextflow/samplesheet/metadata_cell_component.rb index f09628bc17..2d407ae4ef 100644 --- a/app/components/nextflow/samplesheet/metadata_cell_component.rb +++ b/app/components/nextflow/samplesheet/metadata_cell_component.rb @@ -15,7 +15,7 @@ def initialize(form:, name:, sample:, field: nil, required: false) end def metadata_value(name) - sample.metadata.key?(name) ? sample.metadata[name] : '' + sample.metadata.fetch(name, '') end end end diff --git a/app/components/samples/editable_cell.html.erb b/app/components/samples/editable_cell.html.erb new file mode 100644 index 0000000000..c4f85b95ec --- /dev/null +++ b/app/components/samples/editable_cell.html.erb @@ -0,0 +1,29 @@ + + <%= form_with( + url: editable_namespace_project_sample_metadata_field_path( + @sample.project.namespace.parent, + @sample.project, + @sample + ), + method: :get, + class: "w-full" + ) do |form| %> + + <%= form.hidden_field :field, value: @field %> + <%= form.hidden_field :format, value: "turbo_stream" %> + + + <% end %> + diff --git a/app/components/samples/editable_cell.rb b/app/components/samples/editable_cell.rb new file mode 100644 index 0000000000..a706a4d8e3 --- /dev/null +++ b/app/components/samples/editable_cell.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Samples + # Component for rendering an editable cell + class EditableCell < Component + def initialize(field:, sample:, autofocus: false) + @sample = sample + @field = field + @autofocus = autofocus + end + end +end diff --git a/app/components/samples/table_component.html.erb b/app/components/samples/table_component.html.erb index d9e5f8dc8e..c5c09bf938 100644 --- a/app/components/samples/table_component.html.erb +++ b/app/components/samples/table_component.html.erb @@ -2,16 +2,10 @@ <%= render Viral::BaseComponent.new(**wrapper_arguments) do %> <%= render Viral::BaseComponent.new(**system_arguments) do %> <% @columns.each_with_index do |column, index| %> @@ -73,10 +67,7 @@ <% @samples.each do |sample| %> <%= render Viral::BaseComponent.new(**row_arguments(sample)) do %> @@ -84,7 +75,7 @@ <%= render_cell( tag: index.zero? ? 'th' :'td', scope: index.zero? ? 'row' : nil, - classes: class_names('px-3 py-3', 'sticky left-0 min-w-56 max-w-56 bg-slate-50 dark:bg-slate-700': index.zero?, 'sticky left-56 bg-slate-50 dark:bg-slate-700': index == 1) + classes: class_names('px-3 py-3', 'sticky left-0 z-5 min-w-56 max-w-56 bg-slate-50 dark:bg-slate-700': index.zero?, 'sticky z-5 left-56 bg-slate-50 dark:bg-slate-700': index == 1) ) do %> <% if index.zero? && @abilities[:select_samples] %> <%= check_box_tag "sample_ids[]", @@ -133,13 +124,18 @@ <% end %> <% end %> <% @metadata_fields.each do |field| %> - <%= render_cell( + <% if helpers.allowed_to?(:update_sample?, sample.project) %> + <%= render Samples::EditableCell.new(field: field, sample: sample) %> + <% else %> + <%= render_cell( tag: 'td', classes: class_names('px-3 py-3') ) do %> - <%= sample.metadata[field] %> + <%= sample.metadata[field] %> + <% end %> <% end %> <% end %> + <% if @renders_row_actions %> <%= render_cell( tag: 'td', @@ -183,10 +179,7 @@ - + <% end %> diff --git a/app/controllers/projects/samples/metadata/fields_controller.rb b/app/controllers/projects/samples/metadata/fields_controller.rb index 81dcd376d6..b6f7a5fb9a 100644 --- a/app/controllers/projects/samples/metadata/fields_controller.rb +++ b/app/controllers/projects/samples/metadata/fields_controller.rb @@ -4,7 +4,7 @@ module Projects module Samples module Metadata # Controller actions for Project Samples Metadata Fields Controller - class FieldsController < Projects::Samples::ApplicationController + class FieldsController < Projects::Samples::ApplicationController # rubocop:disable Metrics/ClassLength respond_to :turbo_stream # Param received as: @@ -42,6 +42,34 @@ def update # rubocop:disable Metrics/AbcSize end end + def editable + authorize! @project, to: :update_sample? + @field = params[:field] + @value = @sample.metadata[@field] + + if @sample.updatable_field?(@field) + render_editable_field + else + render_non_editable_error + end + end + + def update_value + authorize! @project, to: :update_sample? + + @field = params[:field] + value = params[:value] + original_value = params[:original_value] + + if value == original_value + render_unchanged_field + elsif !@sample.field?(@field) + create_metadata_field(@field, value) + else + update_field_value(original_value, value) + end + end + private def create_field_params @@ -62,6 +90,22 @@ def get_create_status(added_keys, existing_keys) end end + def render_editable_field + render status: :partial_content, turbo_stream: turbo_stream.replace( + helpers.dom_id(@sample, @field), + partial: 'shared/samples/metadata/fields/editing_field_cell', + locals: { sample: @sample, field: @field, value: @value } + ) + end + + def render_non_editable_error + render status: :unprocessable_entity, turbo_stream: turbo_stream.append( + 'flashes', + partial: 'shared/flash', + locals: { type: 'error', message: t('samples.editable_cell.not_editable', field: @field) } + ) + end + def get_create_messages(added_keys, existing_keys) # rubocop:disable Metrics/MethodLength messages = [] if added_keys.count == 1 @@ -99,6 +143,77 @@ def get_update_status_and_message(updated_metadata_field) end update_render_params end + + def create_metadata_field(field, value) + create_params = { field => value } + ::Samples::Metadata::Fields::CreateService.new(@project, @sample, current_user, create_params).execute + + if @sample.errors.any? + render_update_error + else + render_update_success + end + end + + def render_unchanged_field + render turbo_stream: turbo_stream.replace( + helpers.dom_id(@sample, @field), + partial: 'shared/samples/metadata/fields/editable_field_cell', + locals: { sample: @sample, field: @field } + ) + end + + def update_field_value(original_value, new_value) + perform_field_update(original_value, new_value) + + if @sample.errors.any? + render_update_error + else + render_update_success + end + end + + def perform_field_update(original_value, new_value) + ::Samples::Metadata::Fields::UpdateService.new( + @project, + @sample, + current_user, + build_update_params(original_value, new_value) + ).execute + end + + def build_update_params(original_value, new_value) + { + 'update_field' => { + 'key' => { @field => @field }, + 'value' => { original_value => new_value } + } + } + end + + def render_update_error + render status: :unprocessable_entity, + locals: { type: 'error', message: @sample.errors.full_messages.first } + end + + def render_update_success + # When the timestamp is rendered to the form, it only renders down to the second, this was causing timing + # issues for selecting current samples. To fix this, we are adding a second to the timestamp so that the + # timestamp is always greater than the current time. + @timestamp = @sample.updated_at + 1.second + render turbo_stream: [turbo_stream.replace( + helpers.dom_id(@sample, @field), + partial: 'shared/samples/metadata/fields/editable_field_cell', + locals: { sample: @sample, field: @field } + ), + turbo_stream.append( + 'flashes', + partial: 'shared/flash', + locals: { type: 'success', + message: t('samples.editable_cell.update_success') } + ), + turbo_stream.replace('timestamp', partial: 'shared/samples/timestamp_input')] + end end end end diff --git a/app/javascript/controllers/inline_edit_controller.js b/app/javascript/controllers/inline_edit_controller.js new file mode 100644 index 0000000000..b4c769a8dc --- /dev/null +++ b/app/javascript/controllers/inline_edit_controller.js @@ -0,0 +1,28 @@ +import { Controller } from "@hotwired/stimulus"; + +export default class extends Controller { + static targets = ["input"]; + static values = { + original: String, + }; + + connect() { + this.inputTarget.focus(); + this.inputTarget.select(); + } + + submit() { + if (!this.submitted) { + this.element.requestSubmit(); + } + } + + cancel(event) { + if (event.key === "Escape") { + this.inputTarget.value = this.originalValue; + this.inputTarget.blur(); + } else if (event.key === "Enter") { + this.submitted = true; + } + } +} diff --git a/app/models/sample.rb b/app/models/sample.rb index 762596d920..ad7fdcb7b2 100644 --- a/app/models/sample.rb +++ b/app/models/sample.rb @@ -144,6 +144,16 @@ def search_data }.compact end + def field?(field) + metadata.key?(field) + end + + def updatable_field?(field) + return true unless metadata_provenance.key?(field) + + metadata_provenance[field]['source'] == 'user' + end + def should_index? !deleted? end diff --git a/app/services/samples/metadata/update_service.rb b/app/services/samples/metadata/update_service.rb index 77845ce0d0..c8cad25836 100644 --- a/app/services/samples/metadata/update_service.rb +++ b/app/services/samples/metadata/update_service.rb @@ -114,14 +114,14 @@ def add_metadata_to_sample(key, value) def get_metadata_change_status(key, value) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity if value.blank? - :deleted if @sample.metadata.key?(key) + :deleted if @sample.field?(key) elsif @sample.metadata_provenance.key?(key) && @analysis_id.nil? && @sample.metadata_provenance[key]['source'] == 'analysis' :not_updated - elsif @sample.metadata.key?(key) && @sample.metadata[key] == value + elsif @sample.field?(key) && @sample.metadata[key] == value :unchanged else - @sample.metadata.key?(key) ? :updated : :added + @sample.field?(key) ? :updated : :added end end diff --git a/app/views/groups/samples/index.html.erb b/app/views/groups/samples/index.html.erb index 31306d1a11..afdbbdf12b 100644 --- a/app/views/groups/samples/index.html.erb +++ b/app/views/groups/samples/index.html.erb @@ -72,12 +72,7 @@ ) do |f| %> - + <%= render "shared/samples/timestamp_input" %> <%= f.submit t(".select_all_button"), class: "button button--state-default button--size-default" %> <% end %> @@ -104,9 +99,7 @@ "t-search-component block w-full p-2.5 pl-10 text-sm text-slate-900 border border-slate-300 rounded-lg bg-slate-50 focus:ring-primary-500 focus:border-primary-500 dark:bg-slate-700 dark:border-slate-600 dark:placeholder-slate-400 dark:text-white dark:focus:ring-primary-500 dark:focus:border-primary-500", placeholder: t(".search.placeholder") %>
<%= viral_icon( name: "magnifying_glass", diff --git a/app/views/projects/samples/index.html.erb b/app/views/projects/samples/index.html.erb index b1378dc701..3f879dadc3 100644 --- a/app/views/projects/samples/index.html.erb +++ b/app/views/projects/samples/index.html.erb @@ -123,12 +123,7 @@ ) do |f| %> - + <%= render "shared/samples/timestamp_input" %> <%= f.submit t(".select_all_button"), class: "button button--state-default button--size-default" %> <% end %> @@ -153,9 +148,7 @@ "t-search-component block w-full p-2.5 pl-10 text-sm text-slate-900 border border-slate-300 rounded-lg bg-slate-50 focus:ring-primary-500 focus:border-primary-500 dark:bg-slate-700 dark:border-slate-600 dark:placeholder-slate-400 dark:text-white dark:focus:ring-primary-500 dark:focus:border-primary-500", placeholder: t(".search.placeholder") %>
<%= viral_icon( name: "magnifying_glass", diff --git a/app/views/shared/_flash.html.erb b/app/views/shared/_flash.html.erb new file mode 100644 index 0000000000..83ebf6d09e --- /dev/null +++ b/app/views/shared/_flash.html.erb @@ -0,0 +1,3 @@ +<%= turbo_stream.prepend 'flashes' do %> + <%= viral_flash(type: type, data: message) %> +<% end %> diff --git a/app/views/shared/samples/_timestamp_input.html.erb b/app/views/shared/samples/_timestamp_input.html.erb new file mode 100644 index 0000000000..c850125668 --- /dev/null +++ b/app/views/shared/samples/_timestamp_input.html.erb @@ -0,0 +1,7 @@ + diff --git a/app/views/shared/samples/metadata/fields/_editable_field_cell.html.erb b/app/views/shared/samples/metadata/fields/_editable_field_cell.html.erb new file mode 100644 index 0000000000..719579a160 --- /dev/null +++ b/app/views/shared/samples/metadata/fields/_editable_field_cell.html.erb @@ -0,0 +1,5 @@ +<%= render Samples::EditableCell.new( + field: @field, + sample: @sample, + autofocus: true, +) %> diff --git a/app/views/shared/samples/metadata/fields/_editing_field_cell.html.erb b/app/views/shared/samples/metadata/fields/_editing_field_cell.html.erb new file mode 100644 index 0000000000..f051eaba6e --- /dev/null +++ b/app/views/shared/samples/metadata/fields/_editing_field_cell.html.erb @@ -0,0 +1,16 @@ +
diff --git a/config/locales/en.yml b/config/locales/en.yml index fbce330cad..83af042ea1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1536,6 +1536,10 @@ en: updated_at_asc: Oldest updated updated_at_desc: Updated At samples: + editable_cell: + aria_label: Click to edit %{value} + not_editable: The field "%{field}" is not editable + update_success: Metadata was successfully updated table_component: action: Action attachments_updated_at: Files Last Updated diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 1aed3831fd..d1744de172 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -1536,6 +1536,10 @@ fr: updated_at_asc: Oldest updated updated_at_desc: Updated At samples: + editable_cell: + aria_label: Click to edit %{value} + not_editable: The field "%{field}" is not editable + update_success: Metadata was successfully updated table_component: action: Action attachments_updated_at: Files Last Updated diff --git a/config/routes/project.rb b/config/routes/project.rb index f44387020d..fb80f5d485 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -76,7 +76,10 @@ resource :metadata, module: :samples, only: %i[new edit destroy] do scope module: :metadata, as: :metadata do collection do - resource :field, only: %i[update create] + resource :field, only: %i[update create] do + get :editable + patch :update_value + end end collection do resource :deletion, only: %i[new destroy] diff --git a/test/controllers/projects/samples/metadata/fields/fields_controller_test.rb b/test/controllers/projects/samples/metadata/fields/fields_controller_test.rb index 7c73e357f3..1b6f4b8ef0 100644 --- a/test/controllers/projects/samples/metadata/fields/fields_controller_test.rb +++ b/test/controllers/projects/samples/metadata/fields/fields_controller_test.rb @@ -156,6 +156,70 @@ class FieldsControllerTest < ActionDispatch::IntegrationTest } }, format: :turbo_stream } assert_response :unprocessable_entity end + + test 'check to edit a metadata field' do + get editable_namespace_project_sample_metadata_field_path( + @namespace, @project29, @sample32 + ), params: { + 'field' => 'metadatafield1', + 'format' => :turbo_stream + } + assert_response :partial_content + end + + test 'checking to see if a field can be edited fails if it belongs to an analysis' do + sample34 = samples(:sample34) + project31 = projects(:project31) + namespace = groups(:subgroup_twelve_a_a) + get editable_namespace_project_sample_metadata_field_path( + namespace, project31, sample34 + ), params: { + 'field' => 'metadatafield1', + 'format' => :turbo_stream + } + assert_response :unprocessable_entity + end + + test 'builds correct update params for updating a value' do + controller = FieldsController.new + controller.instance_variable_set(:@field, @field) + + expected_params = { + 'update_field' => { + 'key' => { @field => @field }, + 'value' => { 'old_value' => 'new_value' } + } + } + + assert_equal expected_params, + controller.send(:build_update_params, 'old_value', 'new_value') + end + + test 'renders unchanged field when value does not change' do + patch update_value_namespace_project_sample_metadata_field_path( + @namespace, @project29, @sample32 + ), + params: { + 'field' => 'metadatafield1', + 'value' => 'old_value', + 'original_value' => 'old_value', + 'format' => :turbo_stream + } + assert_response :ok + end + + test 'updates sample metadata with new value' do + patch update_value_namespace_project_sample_metadata_field_path( + @namespace, @project29, @sample32 + ), + params: { + 'field' => 'metadatafield1', + 'value' => 'new_value', + 'original_value' => 'old_value', + 'format' => :turbo_stream + } + assert_response :ok + end end end end diff --git a/test/models/sample_test.rb b/test/models/sample_test.rb index 7d0b699f05..86f92036ca 100644 --- a/test/models/sample_test.rb +++ b/test/models/sample_test.rb @@ -171,4 +171,31 @@ def setup assert files.empty? end + + test 'field? returns true when metadata has field' do + @sample.metadata = { test_field: 'value' } + assert @sample.field?('test_field') + end + + test 'field? returns false when metadata does not have field' do + @sample.metadata = { other_field: 'value' } + assert_not @sample.field?('test_field') + end + + test 'updatable_field? returns true when field not in metadata_provenance' do + @sample.metadata = { test_field: 'value' } + assert @sample.updatable_field?('test_field') + end + + test 'updatable_field? returns true when field source is user' do + @sample.metadata = { test_field: 'value' } + @sample.metadata_provenance = { test_field: { source: 'user' } } + assert @sample.updatable_field?('test_field') + end + + test 'updatable_field? returns false when field source is not user' do + @sample.metadata = { test_field: 'value' } + @sample.metadata_provenance = { test_field: { source: 'analysis' } } + assert_not @sample.updatable_field?('test_field') + end end diff --git a/test/system/groups/samples_test.rb b/test/system/groups/samples_test.rb index 5eda210ec4..604db97361 100644 --- a/test/system/groups/samples_test.rb +++ b/test/system/groups/samples_test.rb @@ -391,16 +391,9 @@ def retrieve_puids assert_selector 'table thead tr th', count: 9 within('table tbody tr:first-child') do assert_text @sample30.name - assert_selector 'td:nth-child(7)', text: 'value1' - assert_selector 'td:nth-child(8)', text: 'value2' - assert_selector 'td:nth-child(9)', text: '' - end - - within('table tbody tr:nth-child(3)') do - assert_text @sample28.name - assert_selector 'td:nth-child(7)', text: '' - assert_selector 'td:nth-child(8)', text: '' - assert_selector 'td:nth-child(9)', text: 'unique_value' + assert_selector 'td:nth-child(7) button', text: 'value1' + assert_selector 'td:nth-child(8) button', text: 'value2' + assert_selector 'td:nth-child(9) button', text: '' end find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click @@ -767,5 +760,63 @@ def retrieve_puids click_on I18n.t('shared.samples.metadata.file_imports.errors.ok_button') end end + + test 'can update metadata value that is not from an analysis' do + ### SETUP START ### + visit group_samples_url(@group) + assert_selector 'table thead tr th', count: 6 + + fill_in placeholder: I18n.t(:'groups.samples.index.search.placeholder'), with: @sample1.name + find('input.t-search-component').native.send_keys(:return) + + assert_selector 'label', text: I18n.t('projects.samples.shared.metadata_toggle.label'), count: 1 + find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click + assert_selector 'table thead tr th', count: 9 + + within 'div.overflow-auto.scrollbar' do |div| + div.scroll_to div.find('table thead th:nth-child(7)') + end + ### SETUP END ### + + ### ACTIONS START ### + within('table tbody tr:first-child td:nth-child(7)') do + within('form[method="get"]') do + find('button').click + end + assert_selector "form[data-controller='inline-edit']" + + within('form[data-controller="inline-edit"]') do + find('input[name="value"]').send_keys 'value2' + find('input[name="value"]').send_keys :return + end + ### ACTIONS END ### + + ### VERIFY START ### + assert_no_selector "form[data-controller='inline-edit']" + assert_selector 'form[method="get"]' + assert_selector 'button', text: 'value2' + ### VERIFY END ### + end + end + + test 'project analysts should not be able to edit samples' do + ### SETUP START ### + login_as users(:ryan_doe) + visit group_samples_url(@group) + + find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click + assert_selector 'table thead tr th', count: 9 + + fill_in placeholder: I18n.t(:'projects.samples.index.search.placeholder'), with: @sample28.name + find('input.t-search-component').native.send_keys(:return) + + ### SETUP END ### + + ### VERIFY START ### + within('table tbody tr:first-child td:nth-child(7)') do + assert_no_selector "form[method='get']" + end + ### VERIFY END ### + end end end diff --git a/test/system/projects/samples_test.rb b/test/system/projects/samples_test.rb index 09ee5bb56b..40d9eeab5a 100644 --- a/test/system/projects/samples_test.rb +++ b/test/system/projects/samples_test.rb @@ -2261,6 +2261,99 @@ class SamplesTest < ApplicationSystemTestCase ### VERIFY END ### end + test 'can update metadata value that is not from an analysis' do + ### SETUP START ### + visit namespace_project_samples_url(@namespace, @project) + assert_selector 'table thead tr th', count: 6 + + find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click + assert_selector 'table thead tr th', count: 8 + + fill_in placeholder: I18n.t(:'projects.samples.index.search.placeholder'), with: @sample1.name + find('input.t-search-component').native.send_keys(:return) + + within 'div.overflow-auto.scrollbar' do |div| + div.scroll_to div.find('table thead th:nth-child(7)') + end + + assert_selector 'table thead tr th', count: 8 + ### SETUP END ### + + within('table tbody tr:first-child td:nth-child(7)') do + ### ACTIONS START ### + # check within the from with method get that the value is 'value1': + within('form[method="get"]') do + find('button').click + end + assert_selector "form[data-controller='inline-edit']" + + within('form[data-controller="inline-edit"]') do + find('input[name="value"]').send_keys 'value2' + find('input[name="value"]').send_keys :return + end + ### ACTIONS END ### + + ### VERIFY START ### + assert_no_selector "form[data-controller='inline-edit']" + assert_selector 'form[method="get"]' + assert_selector 'button', text: 'value2' + end + assert_text I18n.t('samples.editable_cell.update_success') + ### VERIFY END ### + end + + test 'should not update metadata value that is from an analysis' do + ### SETUP START ### + subgroup12aa = groups(:subgroup_twelve_a_a) + project31 = projects(:project31) + visit namespace_project_samples_url(subgroup12aa, project31) + assert_selector 'table thead tr th', count: 6 + + find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click + assert_selector 'table thead tr th', count: 8 + + click_on I18n.t('projects.samples.show.table_header.last_updated') + assert_selector 'table thead th:nth-child(4) svg.icon-arrow_up' + + within 'div.overflow-auto.scrollbar' do |div| + div.scroll_to div.find('table thead th:nth-child(8)') + end + + ### SETUP END ### + + within('table tbody tr:nth-child(1) td:nth-child(6)') do + ### ACTIONS START ### + within('form[method="get"]') do + find('button').click + end + ### ACTIONS END ### + + ### VERIFY START ### + assert_no_selector "form[data-controller='inline-edit']" + ### VERIFY END ### + end + end + + test 'project analysts should not be able to edit samples' do + ### SETUP START ### + login_as users(:ryan_doe) + visit namespace_project_samples_url(@namespace, @project) + + find('label', text: I18n.t('projects.samples.shared.metadata_toggle.label')).click + assert_selector 'table thead tr th', count: 7 + + fill_in placeholder: I18n.t(:'projects.samples.index.search.placeholder'), with: @sample2.name + find('input.t-search-component').native.send_keys(:return) + + ### SETUP END ### + + ### VERIFY START ### + within('table tbody tr:first-child td:nth-child(7)') do + assert_no_selector "form[method='get']" + end + ### VERIFY END ### + end + def long_filter_text text = (1..500).map { |n| "sample#{n}" }.join(', ') "#{text}, #{@sample1.name}" # Need to comma to force the tag to be created
@@ -203,7 +196,7 @@ >0
+ <%= form_with(url: update_value_namespace_project_sample_metadata_field_path(sample_id: @sample.id), method: :patch, + data: { controller: "inline-edit", "inline-edit-original-value": @value }) do |f| %> + <%= f.hidden_field :field, value: @field %> + <%= f.hidden_field :original_value, value: @value %> + <%= f.hidden_field :format, value: "turbo_stream" %> + <%= f.text_field :value, + value: @value, + class: + "w-full m-0 border-slate-300 text-slate-900 text-sm focus:ring-primary-500 focus:border-primary-500 block w-full p-2.5 dark:bg-slate-700 dark:border-slate-600 dark:text-white dark:focus:ring-primary-500 dark:focus:border-primary-500", + data: { + "inline-edit-target": "input", + action: "blur->inline-edit#submit keydown->inline-edit#cancel", + } %> + <% end %> +