Sample Exports: Format Filtering UI (#690)

* migrate js selection controller to be universal * cleanup * Change sample and linelist to use same new modal description for samples * add count to sample header * fix, add tests; remove select_all conditionals, add sample count to inifite scroll * make fixes to include attachment_formats * add fr translations * cleanup * change var name * make PR comment changes, remove unneeded spinner controller, fix associated tests * cleanup * change var name * cleanup tests * fix eventlisteners for disconnect * try to fix flaky tests in group links sort * simplify selectors for another flaky test
phac-nml · Aug 14, 2024 · b8793f7 · b8793f7
1 parent 3c8b4f7
commit b8793f7
Show file tree

Hide file tree

Showing 23 changed files with 488 additions and 218 deletions.
diff --git a/app/components/viral/sortable_list/list_component.html.erb b/app/components/viral/sortable_list/list_component.html.erb
@@ -2,9 +2,10 @@
   <%= title %>
   <ul
     id="<%= id %>"
-    data-controller="sortable-list"
-    data-sortable-list-group-name-value="<%= group %>"
+    data-controller="viral--sortable-lists--list"
+    data-viral--sortable-lists--list-group-name-value="<%= group %>"
     class="<%= @system_arguments[:list_classes] %>"
+    tabindex="0"
     >
     <% list_items.each do |list_item| %>
       <li

diff --git a/app/components/viral/sortable_lists_component.html.erb b/app/components/viral/sortable_lists_component.html.erb
@@ -11,3 +11,19 @@
     <%= list %>
   <% end %>
 </div>
+<div class="flex -mt-3 font-medium text-slate-900 dark:text-white">
+  <button
+    class="mr-2 text-sm"
+    data-action="click->viral--sortable-lists--two-lists-selection#addAll"
+    data-viral--sortable-lists--two-lists-selection-target="addAll"
+    >
+    <%= t(".add_all") %>
+  </button>
+  <button
+    class="ml-2 text-sm"
+    data-action="click->viral--sortable-lists--two-lists-selection#removeAll"
+    data-viral--sortable-lists--two-lists-selection-target="removeAll"
+    >
+    <%= t(".remove_all") %>
+  </button>
+</div>
diff --git a/app/controllers/data_exports_controller.rb b/app/controllers/data_exports_controller.rb
@@ -24,20 +24,9 @@ def show
   end
 
   def new
-    if params[:export_type] == 'analysis'
-      render turbo_stream: turbo_stream.update('export_dialog',
-                                               partial: 'new_analysis_export_dialog',
-                                               locals: {
-                                                 open: true, workflow_execution_id: params[:workflow_execution_id]
-                                               }), status: :ok
-    else
-      render turbo_stream: turbo_stream.update('samples_dialog',
-                                               partial: "new_#{params[:export_type]}_export_dialog",
-                                               locals: {
-                                                 open: true, namespace_id: params[:namespace_id]
-                                               }), status: :ok
-
-    end
+    render turbo_stream: turbo_stream.update(params[:export_type] == 'analysis' ? 'export_dialog' : 'samples_dialog',
+                                             partial: "new_#{params[:export_type]}_export_dialog",
+                                             locals:), status: :ok
   end
 
   def create
@@ -140,4 +129,15 @@ def context_crumbs
         path: data_export_path(@data_export)
       }]
   end
+
+  def locals
+    case params[:export_type]
+    when 'analysis'
+      { open: true, workflow_execution_id: params[:workflow_execution_id] }
+    when 'sample'
+      { open: true, namespace_id: params[:namespace_id], formats: Attachment::FORMAT_REGEX.keys.sort }
+    when 'linelist'
+      { open: true, namespace_id: params[:namespace_id] }
+    end
+  end
 end
diff --git a/app/javascript/controllers/infinite_scroll_controller.js b/app/javascript/controllers/infinite_scroll_controller.js
@@ -9,19 +9,25 @@ export default class extends Controller {
     "pageFormContent",
     "scrollable",
     "summary",
+    "sampleCount"
   ];
   static values = {
     pagedFieldName: String,
-    singular: String,
-    plural: String,
+    singularDescription: String,
+    pluralDescription: String,
+    nonZeroHeader: String,
   };
 
   #page = 1;
 
   connect() {
     this.allIds = this.selectionOutlet.getStoredItems();
+    this.numSelected = this.selectionOutlet.getNumSelected()
     this.#makePagedHiddenInputs();
-    this.#replaceCountPlaceholder();
+    this.#replaceDescriptionPlaceholder();
+    if (this.hasSampleCountTarget) {
+      this.#replaceCountPlaceholder(this.sampleCountTarget, this.nonZeroHeaderValue);
+    }
   }
 
   scroll() {
@@ -33,20 +39,21 @@ export default class extends Controller {
     }
   }
 
-  #replaceCountPlaceholder() {
-    const numSelected = this.selectionOutlet.getNumSelected();
-    let summary = this.summaryTarget;
-
-    if (numSelected == 1) {
-      summary.innerHTML = this.singularValue;
+  #replaceDescriptionPlaceholder() {
+    if (this.numSelected === 1) {
+      this.summaryTarget.innerHTML = this.singularDescriptionValue;
     } else {
-      summary.innerHTML = this.pluralValue.replace(
-        "COUNT_PLACEHOLDER",
-        numSelected
-      );
+      this.#replaceCountPlaceholder(this.summaryTarget, this.pluralDescriptionValue);
     }
   }
 
+  #replaceCountPlaceholder(textNode, countPlaceholderText) {
+    textNode.innerHTML = countPlaceholderText.replace(
+      "COUNT_PLACEHOLDER",
+      this.numSelected
+    );
+  }
+
   #makePagedHiddenInputs() {
     const itemsPerPage = 100;
     const start = (this.#page - 1) * itemsPerPage;

diff --git a/...t/controllers/sortable_list_controller.js → ...s/viral/sortable_lists/list_controller.js b/...t/controllers/sortable_list_controller.js → ...s/viral/sortable_lists/list_controller.js
diff --git a/..._exports/metadata_selection_controller.js → ...e_lists/two_lists_selection_controller.js b/..._exports/metadata_selection_controller.js → ...e_lists/two_lists_selection_controller.js
@@ -6,12 +6,9 @@ export default class extends Controller {
   static targets = ["field", "submitBtn", "addAll", "removeAll"];
 
   static values = {
-    selectedList: {
-      type: String,
-    },
-    availableList: {
-      type: String,
-    }
+    selectedList: String,
+    availableList: String,
+    fieldName: String
   };
 
   #disabledClasses = ["pointer-events-none", "cursor-not-allowed", "text-slate-300", "dark:text-slate-700"];
@@ -20,33 +17,53 @@ export default class extends Controller {
   connect() {
     this.availableList = document.getElementById(this.availableListValue)
     this.selectedList = document.getElementById(this.selectedListValue)
-    this.fullListItems = this.availableList.querySelectorAll("li")
-    this.selectedList.addEventListener("mouseover", () => { this.#checkButtonStates() })
-    this.availableList.addEventListener("mouseover", () => { this.#checkButtonStates() })
+    this.allListItems = this.#constructAllListItems(this.availableList, this.selectedList)
+    this.#setInitialSelectAllState(this.availableList, this.addAllTarget)
+    this.#setInitialSelectAllState(this.selectedList, this.removeAllTarget)
+
+    this.buttonStateListener = this.#checkButtonStates.bind(this)
+    this.selectedList.addEventListener("mouseover", this.buttonStateListener)
+    this.availableList.addEventListener("mouseover", this.buttonStateListener)
   }
 
   addAll() {
-    for (const item of this.fullListItems) {
+    for (const item of this.allListItems) {
       this.selectedList.append(item)
     }
     this.#checkButtonStates()
   }
 
   removeAll() {
-    for (const item of this.fullListItems) {
+    for (const item of this.allListItems) {
       this.availableList.append(item)
     }
     this.#checkButtonStates()
   }
 
+  #constructAllListItems(listOne, listTwo) {
+    const listOneItems = Array.prototype.slice.call(listOne.querySelectorAll('li'))
+    const listTwoItems = Array.prototype.slice.call(listTwo.querySelectorAll('li'))
+    return listOneItems.concat(listTwoItems)
+  }
+
+  #setInitialSelectAllState(list, button) {
+    const list_values = list.querySelectorAll("li")
+    if (list_values.length === 0) {
+      button.classList.add(...this.#disabledClasses)
+      button.setAttribute("aria-disabled", "true")
+    } else {
+      button.classList.add(...this.#enabledClasses)
+    }
+  }
+
   #checkButtonStates() {
-    const selected_metadata = this.selectedList.querySelectorAll("li")
-    const available_metadata = this.availableList.querySelectorAll("li")
-    if (selected_metadata.length == 0) {
+    const selected_values = this.selectedList.querySelectorAll("li")
+    const available_values = this.availableList.querySelectorAll("li")
+    if (selected_values.length === 0) {
       this.#setSubmitButtonDisableState(true)
       this.#setAddOrRemoveButtonDisableState(this.removeAllTarget, true)
       this.#setAddOrRemoveButtonDisableState(this.addAllTarget, false)
-    } else if (available_metadata.length == 0) {
+    } else if (available_values.length === 0) {
       this.#setSubmitButtonDisableState(false)
       this.#setAddOrRemoveButtonDisableState(this.removeAllTarget, false)
       this.#setAddOrRemoveButtonDisableState(this.addAllTarget, true)
@@ -73,16 +90,21 @@ export default class extends Controller {
     }
   }
 
-  constructMetadataParams() {
-    const metadata_fields = this.selectedList.querySelectorAll("li")
+  constructParams() {
+    const list_values = this.selectedList.querySelectorAll("li")
 
-    for (const metadata_field of metadata_fields) {
+    for (const list_value of list_values) {
       this.fieldTarget.appendChild(
         createHiddenInput(
-          `data_export[export_parameters][metadata_fields][]`,
-          metadata_field.innerText
+          this.fieldNameValue,
+          list_value.innerText
         )
       );
     }
   }
+
+  disconnect() {
+    this.selectedList.removeEventListener("mouseover", this.buttonStateListener)
+    this.availableList.removeEventListener("mouseover", this.buttonStateListener)
+  }
 }
diff --git a/app/jobs/data_exports/create_job.rb b/app/jobs/data_exports/create_job.rb
@@ -257,12 +257,12 @@ def sample_query(data_export)
     end
 
     def attachments_query(sample, data_export)
-      if data_export.export_parameters.key?('attachment_formats')
+      if (Attachment::FORMAT_REGEX.keys - data_export.export_parameters['attachment_formats']).empty?
+        sample.attachments
+      else
         sample.attachments.select do |attachment|
           data_export.export_parameters['attachment_formats'].include?(attachment.metadata['format'])
         end
-      else
-        sample.attachments
       end
     end
   end

diff --git a/app/models/data_export.rb b/app/models/data_export.rb
@@ -22,19 +22,26 @@ def validate_export_parameters
                  I18n.t('activerecord.errors.models.data_export.attributes.export_parameters.missing_ids'))
     end
 
-    validate_attachment_formats if export_type == 'sample' && export_parameters.key?('attachment_formats')
+    validate_attachment_formats if export_type == 'sample'
     validate_namespace_id unless export_type == 'analysis'
     validate_linelist_export_parameters if export_type == 'linelist'
   end
 
   def validate_attachment_formats
-    invalid_formats = export_parameters['attachment_formats'] - Attachment::FORMAT_REGEX.keys
+    if export_parameters.key?('attachment_formats')
+      invalid_formats = export_parameters['attachment_formats'] - Attachment::FORMAT_REGEX.keys
 
-    return nil if invalid_formats.empty?
+      return nil if invalid_formats.empty?
 
-    errors.add(:export_parameters,
-               I18n.t('activerecord.errors.models.data_export.attributes.export_parameters.invalid_attachment_format',
-                      invalid_formats: invalid_formats.join(', ')))
+      errors.add(:export_parameters,
+                 I18n.t('activerecord.errors.models.data_export.attributes.export_parameters.invalid_attachment_format',
+                        invalid_formats: invalid_formats.join(', ')))
+    else
+      errors.add(:export_parameters,
+                 I18n.t(
+                   'activerecord.errors.models.data_export.attributes.export_parameters.missing_attachment_formats'
+                 ))
+    end
   end
 
   def validate_namespace_id