Enhancement: Metadata edit confirmation dialog

app/components/viral/button_component.html.erb

@@ -1,16 +1,16 @@
 <%= render Viral::BaseComponent.new(tag: 'button', **system_arguments) do %>
   <% if content.present? %>
-    <span><%= content %></span>
+    <span class="pointer-events-none"><%= content %></span>
   <% end %>
 
   <% if disclosure.present? %>
     <% case disclosure %>
-      <% when :down %>
-        <%= viral_icon(name: "chevron_down", classes: "ml-2 w-4 h-4") %>
-        <% when :up %>
-          <%= viral_icon(name: "chevron_up", classes: "ml-2 w-4 h-4") %>
-          <% when :right %>
-            <%= viral_icon(name: "chevron_right", classes: "ml-2 w-4 h-4") %>
-          <% end %>
-        <% end %>
-      <% end %>
+    <% when :down %>
+      <%= viral_icon(name: "chevron_down", classes: "ml-2 w-4 h-4") %>
+    <% when :up %>
+      <%= viral_icon(name: "chevron_up", classes: "ml-2 w-4 h-4") %>
+    <% when :right %>
+      <%= viral_icon(name: "chevron_right", classes: "ml-2 w-4 h-4") %>
+    <% end %>
+  <% end %>
+<% end %>

app/javascript/controllers/inline_edit_controller.js

@@ -1,31 +1,97 @@
 import { Controller } from "@hotwired/stimulus";
 
+/**
+ * Controller for handling inline editing functionality
+ * @extends Controller
+ */
 export default class extends Controller {
-  static targets = ["input"];
+  static targets = ["input", "newValue"];
   static values = {
     original: String,
   };
 
   connect() {
     this.inputTarget.focus();
-    this.inputTarget.select();
   }
 
-  submit() {
-    this.element.requestSubmit();
+  // Handle keyboard events
+  keydown(event) {
+    if (event.key === "Escape") {
+      this.reset();
+    } else if (event.key === "Tab") {
+      event.preventDefault();
+      this.submit();
+    }
   }
 
-  reset() {
-    this.inputTarget.value = this.originalValue;
-    this.submit();
+  // Handle blur event
+  async blur(event) {
+    if (!this.hasChanges) {
+      this.reset();
+      return;
+    }
+
+    event.preventDefault();
+    const dialog = this.element.querySelector("dialog");
+    if (!dialog) return;
+
+    this.updateNewValue();
+    await this.showConfirmDialog(dialog);
+  }
+
+  // Private methods
+  get hasChanges() {
+    const newValue = this.inputTarget.value.trim();
+    return newValue && newValue !== this.originalValue;
+  }
+
+  updateNewValue() {
+    if (this.hasNewValueTarget) {
+      this.newValueTarget.textContent = this.inputTarget.value;
+    }
+  }
+
+  async showConfirmDialog(dialog) {
+    dialog.showModal();
+
+    // Focus the cancel button for accessibility
+    const cancelButton = dialog.querySelector('button[value="cancel"]');
+    if (cancelButton) {
+      requestAnimationFrame(() => cancelButton.focus());
+    }
+
+    // Handle dialog actions
+    dialog.addEventListener(
+      "click",
+      (e) => {
+        if (e.target.tagName !== "BUTTON") return;
+
+        e.target.value === "confirm" ? this.submit() : this.reset();
+        dialog.close();
+      },
+      { once: true },
+    );
+
+    // Handle dialog close
+    dialog.addEventListener(
+      "close",
+      () => {
+        this.inputTarget.focus();
+      },
+      { once: true },
+    );
   }
 
-  inputKeydown(event) {
-    if(event.key === "Escape") {
+  submit() {
+    try {
+      this.element.requestSubmit();
+    } catch {
       this.reset();
-    } else if(event.key === "Tab") {
-      event.preventDefault();
-      this.submit();
     }
   }
+
+  reset() {
+    this.inputTarget.value = this.originalValue;
+    this.submit();
+  }
 }

app/views/shared/samples/metadata/fields/_editing_field_cell.html.erb

@@ -5,13 +5,34 @@
     <%= f.hidden_field :original_value, value: @value %>
     <%= f.hidden_field :format, value: "turbo_stream" %>
     <%= f.text_field :value,
-                 value: @value,
+                 placeholder: @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: {
+                   "test-selector": "field-input",
                    "inline-edit-target": "input",
-                   action:
-                     "blur->inline-edit#reset keydown->inline-edit#inputKeydown",
+                   action: "blur->inline-edit#blur keydown->inline-edit#keydown",
                  } %>
+    <%= viral_dialog(open: false) do |dialog| %>
+      <%= dialog.with_header(
+        title: t("shared.samples.metadata.editing_field_cell.dialog.title"),
+      ) %>
+      <%= dialog.with_section do %>
+        <p class="text-base text-slate-900 dark:text-white text-wrap" data-description>
+          <%= raw t(
+            "shared.samples.metadata.editing_field_cell.dialog.description",
+            value: @value,
+          ) %>
+        </p>
+      <% end %>
+      <% dialog.with_secondary_action do %>
+        <%= viral_button(value: "confirm", state: :primary) do
+          t("shared.samples.metadata.editing_field_cell.dialog.confirm_button")
+        end %>
+        <%= viral_button(value: "cancel", state: :default) do
+          t("shared.samples.metadata.editing_field_cell.dialog.discard_button")
+        end %>
+      <% end %>
+    <% end %>
   <% end %>
 </td>

config/locales/en.yml

@@ -1670,6 +1670,12 @@ en:
         loading: Loading...
     samples:
       metadata:
+        editing_field_cell:
+          dialog:
+            confirm_button: Save
+            description: Click `Save` to update the value to <span class='font-bold' data-inline-edit-target="newValue"></span>, or `Discard` to cancel and return to the previous value of <span class='font-bold'>%{value}</span>.
+            discard_button: Discard
+            title: Confirmation required
         file_imports:
           dialog:
             description: Importing a metadata spreadsheet allows multiple metadata fields to multiple samples be added, updated or removed at once.

config/locales/fr.yml

@@ -1670,6 +1670,12 @@ fr:
         loading: Loading...
     samples:
       metadata:
+        editing_field_cell:
+          dialog:
+            confirm_button: Save
+            description: Click `Save` to update the value to <span class='font-bold' data-inline-edit-target="newValue"></span>, or `Discard` to cancel and return to the previous value of <span class='font-bold'>%{value}</span>.
+            discard_button: Discard
+            title: Confirmation required
         file_imports:
           dialog:
             description: Importing a metadata spreadsheet allows multiple metadata fields to multiple samples be added, updated or removed at once.

test/system/projects/samples_test.rb

@@ -694,7 +694,7 @@ class SamplesTest < ApplicationSystemTestCase
         assert_text "#{I18n.t('samples.table_component.counts.samples')}: 21"
         assert_selector 'strong[data-selection-target="selected"]', text: '0'
       end
-      ### VERIFY END
+      ### VERIFY END ###
     end
 
     test 'empty state of transfer sample project selection' do
@@ -1414,18 +1414,16 @@ class SamplesTest < ApplicationSystemTestCase
       # toggle metadata on for samples table
       find('label', text: I18n.t(:'projects.samples.shared.metadata_toggle.label')).click
       assert_selector '#samples-table table thead tr th', count: 8
-      within('#samples-table table') do
-        within('thead') do
-          # metadatafield1 and 2 already exist, 3 does not and will be added by the import
-          assert_text 'METADATAFIELD1'
-          assert_text 'METADATAFIELD2'
-          assert_no_text 'METADATAFIELD3'
-        end
-        # sample 1 has no current value for metadatafield 1 and 2
-        within("tr[id='#{@sample1.id}']") do
-          assert_selector 'td:nth-child(6)', text: ''
-          assert_selector 'td:nth-child(7)', text: ''
-        end
+      within('#samples-table table thead') do
+        # metadatafield1 and 2 already exist, 3 does not and will be added by the import
+        assert_text 'METADATAFIELD1'
+        assert_text 'METADATAFIELD2'
+        assert_no_text 'METADATAFIELD3'
+      end
+      # sample 1 has no current value for metadatafield 1 and 2
+      within("tr[id='#{@sample1.id}']") do
+        assert_selector 'td:nth-child(6)', text: ''
+        assert_selector 'td:nth-child(7)', text: ''
       end
       ### SETUP END ###
 
@@ -1464,7 +1462,6 @@ class SamplesTest < ApplicationSystemTestCase
       ### SETUP START ###
       subgroup12aa = groups(:subgroup_twelve_a_a)
       project31 = projects(:project31)
-      sample34 = samples(:sample34)
       visit namespace_project_samples_url(subgroup12aa, project31)
       # verify samples table has loaded to prevent flakes
       assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 2, count: 2,
@@ -1488,7 +1485,7 @@ class SamplesTest < ApplicationSystemTestCase
 
         ### VERIFY START ###
         assert_text I18n.t('services.samples.metadata.import_file.sample_metadata_fields_not_updated',
-                           sample_name: sample34.name, metadata_fields: 'metadatafield1')
+                           sample_name: samples(:sample34).name, metadata_fields: 'metadatafield1')
         click_on I18n.t('shared.samples.metadata.file_imports.errors.ok_button')
       end
       # metadatafield3 still added
@@ -1498,7 +1495,7 @@ class SamplesTest < ApplicationSystemTestCase
           assert_text 'METADATAFIELD3'
         end
         # new metadata value
-        within("tr[id='#{sample34.id}']") do
+        within("tr[id='#{samples(:sample34).id}']") do
           assert_selector 'td:nth-child(8)', text: '20'
         end
         ### VERIFY END ###
@@ -2371,6 +2368,88 @@ class SamplesTest < ApplicationSystemTestCase
       ### VERIFY END ###
     end
 
+    test 'shows confirmation dialog when editing metadata field with changes' 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)
+      ### 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 'New Value'
+        end
+      end
+      find('body').click
+
+      assert_selector 'dialog[open]'
+      assert_selector 'dialog button', text: I18n.t('shared.samples.metadata.editing_field_cell.dialog.confirm_button')
+      assert_selector 'dialog button', text: I18n.t('shared.samples.metadata.editing_field_cell.dialog.discard_button')
+
+      click_button I18n.t('shared.samples.metadata.editing_field_cell.dialog.confirm_button')
+      ### ACTIONS END ###
+
+      ### VERIFY START ###
+      assert_no_selector 'dialog[open]'
+      within('table tbody tr:first-child td:nth-child(7)') do
+        assert_selector 'button', text: 'New Value'
+      end
+      ### VERIFY END ###
+    end
+
+    test 'shows confirmation dialog can be cancelled resetting the value' 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)
+      ### 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 'New Value'
+        end
+      end
+      find('body').click
+
+      assert_selector 'dialog[open]'
+      assert_selector 'dialog button', text: I18n.t('shared.samples.metadata.editing_field_cell.dialog.confirm_button')
+      assert_selector 'dialog button', text: I18n.t('shared.samples.metadata.editing_field_cell.dialog.discard_button')
+
+      click_button I18n.t('shared.samples.metadata.editing_field_cell.dialog.discard_button')
+      ### ACTIONS END ###
+
+      ### VERIFY START ###
+      assert_no_selector 'dialog[open]'
+      within('table tbody tr:first-child td:nth-child(7)') do
+        assert_selector 'button', text: ''
+      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