From 52817d8aaa1d02260446111e0c782c6e266958e4 Mon Sep 17 00:00:00 2001
From: Eric Enns <492127+ericenns@users.noreply.github.com>
Date: Fri, 4 Oct 2024 16:15:50 -0500
Subject: [PATCH] Samples Table: Search by metadata field and value presence
(#807)
* feat: add in metadata key value searching to samples table
* chore: cleanup code and add some comments
* chore: add in tests for filtering samples table by metadata and fixed accessibility issue with Samples::TableComponent
* chore: fix tests
* chore: update metadata searching to be case insensitive
* chore: fix issue with select all not updating timestamp param on page morphs
* chore: fix bad rebase
* chore: attempt to fix flaky test
* chore: actually fix flaky test
---
Gemfile | 3 +
Gemfile.lock | 6 ++
.../samples/table_component.html.erb | 1 -
app/controllers/groups/samples_controller.rb | 10 +--
.../projects/samples_controller.rb | 9 ++-
app/views/groups/samples/index.html.erb | 10 ++-
app/views/projects/samples/index.html.erb | 10 ++-
lib/irida/search_syntax/ransack.rb | 23 ++++++
.../search_syntax/ransack_transformer.rb | 77 +++++++++++++++++++
test/system/groups/samples_test.rb | 31 ++++++++
test/system/projects/samples_test.rb | 47 +++++++++++
11 files changed, 213 insertions(+), 14 deletions(-)
create mode 100644 lib/irida/search_syntax/ransack.rb
create mode 100644 lib/irida/search_syntax/ransack_transformer.rb
diff --git a/Gemfile b/Gemfile
index 33fb427c35..1d2c0f2076 100644
--- a/Gemfile
+++ b/Gemfile
@@ -68,6 +68,9 @@ gem 'pagy', '~> 9.0.5' # omit patch digit
# Ransack
gem 'ransack', '~> 4.2.1'
+# Search Syntax
+gem 'search_syntax'
+
# Use Active Storage variants [https://guides.rubyonrails.org/active_storage_overview.html#transforming-images]
# gem "image_processing", "~> 1.2"
diff --git a/Gemfile.lock b/Gemfile.lock
index 1beb1c49ab..2076b2c9d5 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -417,6 +417,7 @@ GEM
ast (~> 2.4.1)
racc
pg (1.5.7)
+ polyglot (0.3.5)
prettier_print (1.2.1)
psych (5.1.2)
stringio
@@ -536,6 +537,8 @@ GEM
nokogiri (>= 1.13.10)
rexml
rubyzip (2.3.2)
+ search_syntax (0.1.3)
+ treetop (~> 1.6)
securerandom (0.3.1)
signet (0.19.0)
addressable (~> 2.8)
@@ -603,6 +606,8 @@ GEM
timecop (0.9.10)
timeout (0.4.1)
trailblazer-option (0.1.2)
+ treetop (1.6.12)
+ polyglot (~> 0.3)
turbo-rails (2.0.6)
actionpack (>= 6.0.0)
activejob (>= 6.0.0)
@@ -707,6 +712,7 @@ DEPENDENCIES
rubocop
rubocop-graphql
rubocop-rails
+ search_syntax
simplecov
solargraph
sprockets-rails
diff --git a/app/components/samples/table_component.html.erb b/app/components/samples/table_component.html.erb
index 804d716a97..554a966377 100644
--- a/app/components/samples/table_component.html.erb
+++ b/app/components/samples/table_component.html.erb
@@ -148,7 +148,6 @@
<% @metadata_fields.each do |field| %>
<%= render_cell(
tag: 'td',
- scope: 'col',
classes: class_names('px-3 py-3')
) do %>
<%= sample.metadata[field] %>
diff --git a/app/controllers/groups/samples_controller.rb b/app/controllers/groups/samples_controller.rb
index 3d0894ff94..caa44940dd 100644
--- a/app/controllers/groups/samples_controller.rb
+++ b/app/controllers/groups/samples_controller.rb
@@ -7,7 +7,7 @@ class SamplesController < Groups::ApplicationController
include Storable
before_action :group, :current_page
- before_action :process_samples, only: %i[index search]
+ before_action :process_samples, only: %i[index search select]
include Sortable
def index
@@ -27,8 +27,7 @@ def select
respond_to do |format|
format.turbo_stream do
if params[:select].present?
- @q = authorized_samples.ransack(search_params.merge({ updated_at_lt: params[:timestamp] }))
- @selected_sample_ids = @q.result.select(:id).pluck(:id)
+ @selected_sample_ids = @q.result.where(updated_at: ..params[:timestamp].to_datetime).select(:id).pluck(:id)
end
end
end
@@ -76,8 +75,9 @@ def process_samples
authorize! @group, to: :sample_listing?
@search_params = search_params
set_metadata_fields
-
- @q = authorized_samples.ransack(@search_params)
+ query_parser = Irida::SearchSyntax::Ransack.new(text: :name_or_puid_cont, metadata_fields: @fields)
+ parsed_params = query_parser.parse(@search_params.fetch(:name_or_puid_cont, nil))
+ @q = authorized_samples.ransack(@search_params.except(:name_or_puid_cont).merge(parsed_params))
end
def search_params
diff --git a/app/controllers/projects/samples_controller.rb b/app/controllers/projects/samples_controller.rb
index f17f36a519..a64ab22aa7 100644
--- a/app/controllers/projects/samples_controller.rb
+++ b/app/controllers/projects/samples_controller.rb
@@ -9,7 +9,7 @@ class SamplesController < Projects::ApplicationController # rubocop:disable Metr
before_action :sample, only: %i[show edit update view_history_version]
before_action :current_page
- before_action :process_samples, only: %i[index search]
+ before_action :process_samples, only: %i[index search select]
include Sortable
def index
@@ -84,8 +84,7 @@ def select
respond_to do |format|
format.turbo_stream do
if params[:select].present?
- @q = load_samples.ransack(search_params.merge({ updated_at_lt: params[:timestamp] }))
- @samples = @q.result.select(:id)
+ @samples = @q.result.where(updated_at: ..params[:timestamp].to_datetime).select(:id)
end
end
end
@@ -145,7 +144,9 @@ def process_samples
@search_params = search_params
set_metadata_fields
- @q = load_samples.ransack(@search_params)
+ query_parser = Irida::SearchSyntax::Ransack.new(text: :name_or_puid_cont, metadata_fields: @fields)
+ parsed_params = query_parser.parse(@search_params.fetch(:name_or_puid_cont, nil))
+ @q = load_samples.ransack(@search_params.except(:name_or_puid_cont).merge(parsed_params))
end
def search_params
diff --git a/app/views/groups/samples/index.html.erb b/app/views/groups/samples/index.html.erb
index 4a5f4d1978..22effed069 100644
--- a/app/views/groups/samples/index.html.erb
+++ b/app/views/groups/samples/index.html.erb
@@ -28,7 +28,7 @@
},
class:
"flex items-center px-4 py-2 bg-slate-100 text-slate-600 dark:bg-slate-600 dark:text-slate-300
- border-slate-100 dark:border-slate-600 pointer-events-none cursor-not-allowed",
+ border-slate-100 dark:border-slate-600 pointer-events-none cursor-not-allowed",
) %>
<% else %>
<% dropdown.with_item(
@@ -72,7 +72,12 @@
) do |f| %>
-
+
<%= f.submit t(".select_all_button"),
class: "button button--state-default button--size-default" %>
<% end %>
@@ -111,6 +116,7 @@
<%= viral_icon(name: "magnifying_glass", classes: "h-5 w-5") %>
<%= f.search_field :name_or_puid_cont,
+ value: @search_params[:name_or_puid_cont],
"data-action": "filters#submit",
class:
"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",
diff --git a/app/views/projects/samples/index.html.erb b/app/views/projects/samples/index.html.erb
index ebddfe5688..9149da4d0e 100644
--- a/app/views/projects/samples/index.html.erb
+++ b/app/views/projects/samples/index.html.erb
@@ -57,7 +57,7 @@
},
class:
"flex items-center px-4 py-2 bg-slate-100 text-slate-600 dark:bg-slate-600 dark:text-slate-300
- border-slate-100 dark:border-slate-600 pointer-events-none cursor-not-allowed",
+ border-slate-100 dark:border-slate-600 pointer-events-none cursor-not-allowed",
) %>
<% else %>
<% dropdown.with_item(
@@ -126,7 +126,12 @@
) do |f| %>
-
+
<%= f.submit t(".select_all_button"),
class: "button button--state-default button--size-default" %>
<% end %>
@@ -168,6 +173,7 @@
<%= viral_icon(name: "magnifying_glass", classes: "h-5 w-5") %>
<%= f.search_field :name_or_puid_cont,
+ value: @search_params[:name_or_puid_cont],
"data-action": "filters#submit",
class:
"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",
diff --git a/lib/irida/search_syntax/ransack.rb b/lib/irida/search_syntax/ransack.rb
new file mode 100644
index 0000000000..9ada5fdc3b
--- /dev/null
+++ b/lib/irida/search_syntax/ransack.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require_relative 'ransack_transformer'
+
+module Irida
+ module SearchSyntax
+ # IRIDA Next specific Ransack search syntax parser that supports metadata_fields
+ class Ransack
+ def initialize(text:, params: [], metadata_fields: [], sort: nil)
+ @transformer = RansackTransformer.new(text:, params:, metadata_fields:, sort:)
+ @parser = ::SearchSyntax::Parser.new
+ end
+
+ def parse_with_errors(text)
+ @transformer.transform_with_errors(@parser.parse(text || '').value)
+ end
+
+ def parse(text)
+ parse_with_errors(text)[0]
+ end
+ end
+ end
+end
diff --git a/lib/irida/search_syntax/ransack_transformer.rb b/lib/irida/search_syntax/ransack_transformer.rb
new file mode 100644
index 0000000000..cb12ed4605
--- /dev/null
+++ b/lib/irida/search_syntax/ransack_transformer.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+module Irida
+ module SearchSyntax
+ # IRIDA Next specific RansackTransformer that supoorts metadata_fields
+ class RansackTransformer < ::SearchSyntax::RansackTransformer
+ def initialize(text:, params:, metadata_fields:, sort: nil)
+ super(text:, params:, sort:)
+ @metadata_fields = metadata_fields
+ end
+
+ # Note this method is a copy of SearchSyntax::RanstackTronsformer but with additional processing for metadata
+ def transform_with_errors(ast) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity,Metrics/MethodLength,Metrics/PerceivedComplexity
+ errors = []
+ result = {}
+
+ if @allowed_params.length.positive? || @metadata_fields.length.positive?
+ ast = ast.filter do |node| # rubocop:disable Metrics/BlockLength
+ if node[:type] != :param
+ true
+ elsif node[:name] == @sort
+ result[:s] = transform_sort_param(node[:value])
+ false
+ elsif @allowed_params.include?(node[:name])
+ key = name_with_predicate(node)
+ if result.key?(key)
+ errors.push(::SearchSyntax::DuplicateParamError.new(
+ name: node[:name],
+ start: node[:start],
+ finish: node[:finish]
+ ))
+ else
+ result[key] = node[:value]
+ end
+ false
+ elsif @metadata_fields.include?(node[:name].downcase)
+ # all metadata searching uses metadata_jcont ransacker
+ key = 'metadata_jcont'
+ search_value = {}
+ search_value = JSON.parse(result[key]) if result.key?(key)
+ # if we already processed a metadata field, ignore subsequent instances of field and push an error
+ if search_value.key?(node[:name].downcase)
+ errors.push(::SearchSyntax::DuplicateParamError.new(
+ name: node[:name],
+ start: node[:start],
+ finish: node[:finish]
+ ))
+ # else add it to the search_value hash and then transform back to a JSON string
+ else
+ search_value[node[:name].downcase] = node[:value]
+ result[key] = JSON.generate(search_value)
+ end
+ false
+ else
+ errors.push(::SearchSyntax::UnknownParamError.new(
+ name: node[:name],
+ start: node[:start],
+ finish: node[:finish],
+ did_you_mean: @spell_checker.correct(node[:name])
+ ))
+ true
+ end
+ end
+ end
+
+ previous = -1
+ result[@text] = ast.map do |node|
+ separator = previous == node[:start] || previous == -1 ? '' : ' '
+ previous = node[:finish]
+ separator + node[:raw]
+ end.join
+
+ [result, errors]
+ end
+ end
+ end
+end
diff --git a/test/system/groups/samples_test.rb b/test/system/groups/samples_test.rb
index 72a075df31..8e0f477129 100644
--- a/test/system/groups/samples_test.rb
+++ b/test/system/groups/samples_test.rb
@@ -128,6 +128,37 @@ def retrieve_puids
assert_no_text @sample2.name
end
+ test 'can search the list of samples by metadata field and value presence when metadata is toggled' do
+ visit group_samples_url(@group)
+ filter_text = 'metadatafield1:value1'
+
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 20, count: 26,
+ locale: @user.locale))
+
+ assert_selector 'table tbody tr', count: 20
+ assert_text @sample1.name
+ assert_text @sample2.name
+
+ 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_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 20, count: 26,
+ locale: @user.locale))
+
+ assert_selector '#samples-table table thead tr th', count: 9
+ assert_selector 'div#limit-component button div span', text: '20'
+
+ fill_in placeholder: I18n.t(:'groups.samples.index.search.placeholder'), with: filter_text
+
+ assert_selector '#samples-table table tbody tr', count: 1
+ assert_selector '#samples-table table thead tr th', count: 9
+ assert_selector 'div#limit-component button div span', text: '20'
+
+ assert_text @sample30.name
+ assert_no_text @sample1.name
+ assert_no_text @sample2.name
+ end
+
test 'can sort the list of samples' do
visit group_samples_url(@group)
diff --git a/test/system/projects/samples_test.rb b/test/system/projects/samples_test.rb
index b6536fcf38..9dfd66864d 100644
--- a/test/system/projects/samples_test.rb
+++ b/test/system/projects/samples_test.rb
@@ -482,6 +482,8 @@ class SamplesTest < ApplicationSystemTestCase
Project.reset_counters(project38.id, :samples_count)
visit namespace_project_samples_url(namespace17, project38)
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 20, count: 200,
+ locale: @user.locale))
click_button I18n.t(:'projects.samples.index.select_all_button')
@@ -510,6 +512,9 @@ class SamplesTest < ApplicationSystemTestCase
Project.reset_counters(project2.id, :samples_count)
visit namespace_project_samples_url(namespace1, project2)
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 20, count: 220,
+ locale: @user.locale))
+
click_button I18n.t(:'projects.samples.index.select_all_button')
within 'tbody' do
@@ -667,6 +672,48 @@ class SamplesTest < ApplicationSystemTestCase
assert_no_text @sample3.name
end
+ test 'can search the list of samples by metadata field and value presence when metadata is toggled' do
+ visit namespace_project_samples_url(@namespace, @project)
+ filter_text = 'metadatafield1:value1'
+
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 3, count: 3,
+ locale: @user.locale))
+ assert_selector '#samples-table table tbody tr', count: 3
+ assert_text @sample1.name
+ assert_text @sample2.name
+ assert_text @sample3.name
+
+ 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_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 3, count: 3,
+ locale: @user.locale))
+ assert_selector '#samples-table table tbody tr', count: 3
+ assert_selector '#samples-table table thead tr th', count: 8
+ assert_selector 'div#limit-component button div span', text: '20'
+
+ fill_in placeholder: I18n.t(:'projects.samples.index.search.placeholder'), with: filter_text
+
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 1, count: 1,
+ locale: @user.locale))
+ assert_selector '#samples-table table tbody tr', count: 1
+ assert_field type: 'search', with: filter_text
+ assert_no_text @sample1.name
+ assert_no_text @sample2.name
+ assert_text @sample3.name
+
+ # Refresh the page to ensure the search is still active
+ visit namespace_project_samples_url(@namespace, @project)
+
+ assert_text strip_tags(I18n.t(:'viral.pagy.limit_component.summary', from: 1, to: 1, count: 1,
+ locale: @user.locale))
+ assert_selector '#samples-table table tbody tr', count: 1
+ assert_field type: 'search', with: filter_text
+ assert_no_text @sample1.name
+ assert_no_text @sample2.name
+ assert_text @sample3.name
+ end
+
test 'can change pagination and then filter by name' do
visit namespace_project_samples_url(@namespace, @project)