Skip to content

Commit

Permalink
Samples Table: Search by metadata field and value presence (#807)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
ericenns authored Oct 4, 2024
1 parent e8800ab commit 52817d8
Show file tree
Hide file tree
Showing 11 changed files with 213 additions and 14 deletions.
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
6 changes: 6 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -707,6 +712,7 @@ DEPENDENCIES
rubocop
rubocop-graphql
rubocop-rails
search_syntax
simplecov
solargraph
sprockets-rails
Expand Down
1 change: 0 additions & 1 deletion app/components/samples/table_component.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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] %>
Expand Down
10 changes: 5 additions & 5 deletions app/controllers/groups/samples_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 5 additions & 4 deletions app/controllers/projects/samples_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions app/views/groups/samples/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -72,7 +72,12 @@
) do |f| %>
<input type="hidden" name="format" value="turbo_stream"/>
<input type="hidden" name="select" value="on"/>
<input type="hidden" name="timestamp" value="<%=@timestamp%>"/>
<input
type="hidden"
name="timestamp"
value="<%=@timestamp%>"
data-turbo-temporary
/>
<%= f.submit t(".select_all_button"),
class: "button button--state-default button--size-default" %>
<% end %>
Expand Down Expand Up @@ -111,6 +116,7 @@
<%= viral_icon(name: "magnifying_glass", classes: "h-5 w-5") %>
</div>
<%= 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",
Expand Down
10 changes: 8 additions & 2 deletions app/views/projects/samples/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -126,7 +126,12 @@
) do |f| %>
<input type="hidden" name="format" value="turbo_stream"/>
<input type="hidden" name="select" value="on"/>
<input type="hidden" name="timestamp" value="<%=@timestamp%>"/>
<input
type="hidden"
name="timestamp"
value="<%=@timestamp%>"
data-turbo-temporary
>
<%= f.submit t(".select_all_button"),
class: "button button--state-default button--size-default" %>
<% end %>
Expand Down Expand Up @@ -168,6 +173,7 @@
<%= viral_icon(name: "magnifying_glass", classes: "h-5 w-5") %>
</div>
<%= 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",
Expand Down
23 changes: 23 additions & 0 deletions lib/irida/search_syntax/ransack.rb
Original file line number Diff line number Diff line change
@@ -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
77 changes: 77 additions & 0 deletions lib/irida/search_syntax/ransack_transformer.rb
Original file line number Diff line number Diff line change
@@ -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
31 changes: 31 additions & 0 deletions test/system/groups/samples_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
47 changes: 47 additions & 0 deletions test/system/projects/samples_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 52817d8

Please sign in to comment.