Merge pull request #2275 from broadinstitute/jb-search-filter-order

Better sorting/uniquifying of facet filters (SCP-6023)

Author: bistline

app/models/search_facet.rb

@@ -400,10 +400,14 @@ def get_unique_filter_values(public_only: false)
         filter_map += filters_from_metadatum(metadatum)
       end
     end
-    filter_map.uniq
+    return filter_map if is_numeric?
+
+    filter_map.uniq { |filter| [filter[:id]&.downcase, filter[:name]&.downcase] }.reject do |filter|
+      filter[:id].blank? || filter[:name].blank?
+    end
   end
 
-  # find all matching CellMetadum objects for a given facet
+  # find all matching CellMetadatum objects for a given facet
   # can scope query to matching values array if needed or by a sub-query if provided
   def associated_metadata(values:, query: {})
     annotation_type = is_numeric? ? 'numeric' : 'group'
@@ -452,15 +456,15 @@ def update_filter_values!(external_facet = nil)
       merged_values = values.dup
       if external_facet[:filters]
         Rails.logger.info "Merging #{external_facet[:filters]} into '#{name}' facet filters"
-        external_facet[:filters].each do |filter|
+        external_facet[:filters].uniq(&:downcase).each do |filter|
           merged_values << { id: filter, name: filter } unless filters_include?(filter)
         end
       end
       return false if values.empty? # found no results, meaning an error occurred
 
-      values.sort_by! { |f| f[:name] }
-      merged_values.sort_by! { |f| f[:name] }
-      public_values = get_unique_filter_values(public_only: true)
+      values.sort_by! { |f| f[:name].downcase }
+      merged_values.sort_by! { |f| f[:name].downcase }
+      public_values = get_unique_filter_values(public_only: true).sort_by { |f| f[:name].downcase }
       update(filters: values, public_filters: public_values, filters_with_external: merged_values)
     end
   end

test/models/search_facet_test.rb

@@ -43,6 +43,19 @@ class SearchFacetTest < ActiveSupport::TestCase
       { id: 'NCBITaxon_9606', name: 'Homo sapiens' },
       { id: 'NCBITaxon_10090', name: 'Mus musculus' }
     ]
+
+    SearchFacet.create(name: 'Cell type', identifier: 'cell_type', is_mongo_based: true,
+                       ontology_urls: [
+                         {
+                           name: 'Cell Ontology',
+                           url: 'https://www.ebi.ac.uk/ols/api/ontologies/cl',
+                           browser_url: 'https://www.ebi.ac.uk/ols/ontologies/cl'
+                         }
+                       ],
+                       data_type: 'string', is_ontology_based: true, is_array_based: false,
+                       big_query_id_column: 'cell_type',
+                       big_query_name_column: 'cell_type__ontology_label',
+                       convention_name: 'Alexandria Metadata Convention', convention_version: '2.2.0')
   end
 
   after(:all) do
@@ -158,6 +171,7 @@ class SearchFacetTest < ActiveSupport::TestCase
   test 'should find all filter matches' do
     azul_diseases = AzulSearchService.get_all_facet_filters['disease']
     disease_keyword = 'cancer'
+    skip 'Azul search service not available' unless azul_diseases
     cancers = azul_diseases[:filters].select { |d| d.match?(disease_keyword) }
     disease_facet = SearchFacet.find_by(identifier: 'disease')
     disease_facet.update_filter_values!(azul_diseases)
@@ -215,18 +229,7 @@ class SearchFacetTest < ActiveSupport::TestCase
                           ]
                         }
                       ])
-    facet = SearchFacet.create(name: 'Cell type', identifier: 'cell_type', is_mongo_based: true,
-                               ontology_urls: [
-                                 {
-                                   name: 'Cell Ontology',
-                                   url: 'https://www.ebi.ac.uk/ols/api/ontologies/cl',
-                                   browser_url: 'https://www.ebi.ac.uk/ols/ontologies/cl'
-                                 }
-                               ],
-                               data_type: 'string', is_ontology_based: true, is_array_based: false,
-                               big_query_id_column: 'cell_type',
-                               big_query_name_column: 'cell_type__ontology_label',
-                               convention_name: 'Alexandria Metadata Convention', convention_version: '2.2.0')
+    facet = SearchFacet.find_by(identifier: 'cell_type')
     expected_filters = [
       { id: 'CL_0000236', name: 'B cell' },
       { id: 'CL_0000561', name: 'amacrine cell' },
@@ -255,4 +258,16 @@ class SearchFacetTest < ActiveSupport::TestCase
     facet.reload
     assert_equal expected_filter, facet.filter_for_presence
   end
+
+  test 'should sort and uniquify filters properly' do
+    facet = SearchFacet.find_by(identifier: 'cell_type')
+    facet.update_filter_values!
+    assert facet.filters.first[:name] == 'amacrine cell'
+    external_filters = { filters: ['b cell', 'B cell', 'amacrine cell', 'Amacrine Cell', 'T cell'] }
+    facet.update_filter_values!(external_filters)
+    facet.reload
+    assert_equal 1, facet.filters_with_external.select { |f| f[:name] == 'amacrine cell' }.count
+    assert_equal 1, facet.filters_with_external.select { |f| f[:name] == 'T cell' }.count
+    assert_equal 'T cell', facet.filters_with_external.last[:name]
+  end
 end