4088: code review feedback

* Filter files by regex before .take(10000)
* Added "regular expression" placeholder in the filter input box
* Issue a Rails "alert" div if the regex could not be parsed
* Test that we're actually viewing a collection when no matches are
  found (and not a fiddlesticks page or something)

diff --git a/apps/workbench/app/views/collections/_show_files.html.erb b/apps/workbench/app/views/collections/_show_files.html.erb
index 9b39b61d44240e44a81ef913487cd30ed768c4d7..805591349a3413a6159fbee664549cc50ecf9c0a 100644 (file)
--- a/apps/workbench/app/views/collections/_show_files.html.erb
+++ b/apps/workbench/app/views/collections/_show_files.html.erb
@@ -19,7 +19,7 @@
     </div>
     <div class="pull-right">
       <%= form_tag collection_path(@object.uuid), {method: 'get'} do %>
-        <input class="form-control" id="file_regex" name="file_regex" value="<%= params[:file_regex] %>" type="text"/>
+        <input class="form-control" id="file_regex" name="file_regex" placeholder="regular expression" value="<%= params[:file_regex] %>" type="text"/>
         <button id="file_regex_submit" type="submit" class="btn btn-primary" autofocus>Filter</button>
       <% end %>
     </div>
@@ -27,18 +27,20 @@
   <p/>
   <% end %>
 
-<%
-  file_regex = nil
-  if params[:file_regex]
-    begin
-      file_regex = Regexp.new(params[:file_regex])
-    rescue RegexpError
-      # If the pattern is not a valid regex, quote it
-      # (i.e. use it as a simple substring search)
-      file_regex = Regexp.new(Regexp.quote(params[:file_regex]))
-    end
-  end
-%>
+<% file_regex = nil %>
+<% if params[:file_regex] %>
+  <% begin %>
+    <% file_regex = Regexp.new(params[:file_regex]) %>
+  <% rescue RegexpError %>
+    <% # If the pattern is not a valid regex, quote it %>
+    <% # (i.e. use it as a simple substring search) %>
+    <div class="alert alert-info">
+      <p>The search term <code><%= params[:file_regex] %></code> could not be parsed as a regular expression.</p>
+      <p>Searching for files named <code><%= params[:file_regex] %></code> instead.</p>
+    </div>
+    <% file_regex = Regexp.new(Regexp.quote(params[:file_regex])) %>
+  <% end %>
+<% end %>
 
 <% file_tree = @object.andand.files_tree %>
 <% if file_tree.nil? or file_tree.empty? %>
@@ -46,7 +48,13 @@
 <% else %>
   <ul id="collection_files" class="collection_files">
   <% dirstack = [file_tree.first.first] %>
-  <% file_tree.take(10000).each_with_index do |(dirname, filename, size), index| %>
+  <% file_tree.reject { |(dirname, filename, size)|
+       # Eliminate any files that don't match file_regex
+       # (or accept all files if no file_regex was given)
+       file_regex and !file_regex.match(filename)
+       }
+       .take(10000)
+       .each_with_index do |(dirname, filename, size), index| %>
     <% file_path = CollectionsHelper::file_path([dirname, filename]) %>
     <% while dirstack.any? and (dirstack.last != dirname) %>
       <% dirstack.pop %></ul></li>
@@ -59,9 +67,6 @@
       </div>
       <ul class="collection_files">
     <% else %>
-      <% if !file_regex.nil? and !file_regex.match(filename) %>
-        <% next %>
-      <% end %>
       <% link_params = {controller: 'collections', action: 'show_file',
                         uuid: @object.portable_data_hash, file: file_path, size: size} %>
        <div class="collection_files_row">

diff --git a/apps/workbench/test/integration/collections_test.rb b/apps/workbench/test/integration/collections_test.rb
index ea42052a3e068ebbb7583715f5d2ed0fff9dead1..3abbf6f6ac31feb659a99bdd503fde8012663ee9 100644 (file)
--- a/apps/workbench/test/integration/collections_test.rb
+++ b/apps/workbench/test/integration/collections_test.rb
@@ -225,11 +225,16 @@ class CollectionsTest < ActionDispatch::IntegrationTest
     assert page.has_no_text?("file1")
     assert page.has_no_text?("file2")
     assert page.has_no_text?("file3")
+    # make sure that we actually are looking at the collections
+    # page and not e.g. a fiddlesticks
+    assert page.has_text?("multilevel_collection_1")
+    assert page.has_text?(col['portable_data_hash'])
 
     # Syntactically invalid regex
     # Page loads, but does not match any files
     page.find_field('file_regex').set('file[2')
     find('button#file_regex_submit').click
+    assert page.has_text?('could not be parsed as a regular expression')
     assert page.has_no_text?("file1")
     assert page.has_no_text?("file2")
     assert page.has_no_text?("file3")