Project

General

Profile

Actions

Feature #4088

closed

[Workbench] On collections#show page, filter displayed files by filename regexp, and add a "select all" / "deselect all" button.

Added by Tom Clegg almost 8 years ago. Updated almost 8 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
Workbench
Target version:
Start date:
10/28/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

On "show collection" page:

  • Add a regexp text box
  • Use existing filterable.js functionality to hide rows that don't match the regexp
  • Add "select all" and "deselect all" buttons

Tests:

  • Collection with some files that match the regex
  • Collection with all files matching the regex
  • Collection with no files matching the regex
  • User enters syntactically invalid regex
  • Select-all selects all visible files
  • Deselect-all deselects all files
  • "Select all → Enter regexp → create collection from selected files" works correctly (e.g., files that were selected but are now invisible do not get included in the new collection)

Files

4088 Buttons.png (1.76 KB) 4088 Buttons.png Collection view buttons not quite contiguous in Firefox Brett Smith, 11/05/2014 09:19 PM

Subtasks 5 (0 open5 closed)

Task #4206: write workbench testsResolvedTim Pierce10/28/2014

Actions
Task #4384: Review 4088-collection-show-files-filterResolvedTim Pierce10/28/2014

Actions
Task #4205: Add "select all" / "deselect all" buttons to app/views/collections/_show_files.html.erbResolvedTim Pierce10/29/2014

Actions
Task #4332: Review 4088-collection-show-files-filterResolvedTim Pierce10/28/2014

Actions
Task #4204: Add regex box and filter to app/views/collections/_show_files.html.erbResolvedTim Pierce10/28/2014

Actions

Related issues

Related to Arvados - Bug #4358: [Tests] When comparing pipelines, the graph should highlight the differences by using different colors for pipeline A/B/both.New10/29/2014

Actions
Actions #1

Updated by Ward Vandewege almost 8 years ago

  • Target version changed from Arvados Future Sprints to 2014-10-29 sprint
Actions #2

Updated by Tim Pierce almost 8 years ago

  • Category set to Workbench
  • Assigned To set to Tim Pierce
Actions #3

Updated by Tim Pierce almost 8 years ago

  • Description updated (diff)
Actions #4

Updated by Tim Pierce almost 8 years ago

  • Status changed from New to In Progress
Actions #5

Updated by Brett Smith almost 8 years ago

Reviewing e34135f677

  • I know the story specifies this, so it's not on you, but I'll throw it out there: I'm surprised that we want to send the regexp to the server and (apparently) do the filtering there. It's not difficult to write regular expressions with exponential runtime. A mischievious user could put the API server in some agony by running a pathological filter on a large collection.
    I realize that, in general, we put a lot of trust in our users, since we literally run their arbitrary code. But I think there's a difference in degree between "A user can SSH to a shell node and push code to a git repository to run it on dedicated compute nodes" and "A user can fill in a small text box to DOS Arvados' most central component."
    Doing the filtering purely client-side would eliminate this mischief, provide richer interactions with the de/select all buttons, and make it easier to report errors.
  • I don't think it's clear enough that the text box is expecting a regular expression. Dim text explaining this, like we have in the search box, would be nice to see.
  • For consistency with our other search fields, I think the filter button should be attached to the right of the text box.
  • It would be really nice if we could report some kind of warning to the user when compiling the regular expression fails. For a complex regular expression, it could be hard to tell the difference between "My regexp has a logical bug and isn't matching what it should" and "My regexp has a syntax error and has deformed to a plaintext search." Maybe a Rails alert would be good for this?
  • The tests that expect to find no matches would be a little more reassuring if they checked that they were still on the Collections page and not, e.g., seeing fiddlesticks. An idea off the top of my head, the bad regexp case might be easiest as a functional test; you could submit a bad filter and then assert_response :success.
  • Assuming that we stick to server-side filtering, it might be nice if the filter was applied before the take(10000) in _show_files.html.erb. That would give users a way to see and select files in very large collections that might be unreachable otherwise.

Thanks.

Actions #6

Updated by Tim Pierce almost 8 years ago

New revision at f549c9f

Brett Smith wrote:

Reviewing e34135f677

  • I know the story specifies this, so it's not on you, but I'll throw it out there: I'm surprised that we want to send the regexp to the server and (apparently) do the filtering there. It's not difficult to write regular expressions with exponential runtime. A mischievious user could put the API server in some agony by running a pathological filter on a large collection.
    I realize that, in general, we put a lot of trust in our users, since we literally run their arbitrary code. But I think there's a difference in degree between "A user can SSH to a shell node and push code to a git repository to run it on dedicated compute nodes" and "A user can fill in a small text box to DOS Arvados' most central component."
    Doing the filtering purely client-side would eliminate this mischief, provide richer interactions with the de/select all buttons, and make it easier to report errors.

I agree on all counts. Particularly on the user interface issues, but the potential DOS attack is excellent.

Rewriting the tab that way would involve a substantial rewrite of the _show_files code. I'm willing to take it on but it's definitely a larger story than this one. I do think it's the preferable approach.

  • I don't think it's clear enough that the text box is expecting a regular expression. Dim text explaining this, like we have in the search box, would be nice to see.

Yup, added that.

  • For consistency with our other search fields, I think the filter button should be attached to the right of the text box.

I've been trying to figure out how to do that but with no luck. I tried copying the code for the search box but it didn't work to put the buttons inline. If I have time I'll get back to this. Maybe one of our CSS gods will know the right answer.

  • It would be really nice if we could report some kind of warning to the user when compiling the regular expression fails. For a complex regular expression, it could be hard to tell the difference between "My regexp has a logical bug and isn't matching what it should" and "My regexp has a syntax error and has deformed to a plaintext search." Maybe a Rails alert would be good for this?

That seems to make sense. Added a rails alert (and updated the "no match" integration test).

  • The tests that expect to find no matches would be a little more reassuring if they checked that they were still on the Collections page and not, e.g., seeing fiddlesticks. An idea off the top of my head, the bad regexp case might be easiest as a functional test; you could submit a bad filter and then assert_response :success.

I've added a few more assertions to make sure we're still looking at a page that's displaying multilevel_collection_1. The functional test seems like a good idea too.

  • Assuming that we stick to server-side filtering, it might be nice if the filter was applied before the take(10000) in _show_files.html.erb. That would give users a way to see and select files in very large collections that might be unreachable otherwise.

I think we can do that. Updated.

Actions #7

Updated by Brett Smith almost 8 years ago

Tim Pierce wrote:

New revision at f549c9f

Rewriting the tab that way would involve a substantial rewrite of the _show_files code. I'm willing to take it on but it's definitely a larger story than this one. I do think it's the preferable approach.

Yeah, I think we would ideally get Tom to weigh in on this. I'll try to flag him.

  • For consistency with our other search fields, I think the filter button should be attached to the right of the text box.

I've been trying to figure out how to do that but with no luck. I tried copying the code for the search box but it didn't work to put the buttons inline. If I have time I'll get back to this. Maybe one of our CSS gods will know the right answer.

Try this:

    <div class="pull-right">
      <%= form_tag collection_path(@object.uuid), {method: 'get'} do %>
        <div class="input-group">
          <input class="form-control" id="file_regex" name="file_regex" placeholder="regular expression" value="<%= params[:file_regex] %>" type="text"/>
          <span class="input-group-btn">
            <button id="file_regex_submit" type="submit" class="btn btn-primary" autofocus>Filter</button>
          </span>
        </div>
      <% end %>
    </div>

The magic is the div.input-group and span.input-group-btn wrappers. I found this on the Bootstrap documentation.

  • It would be really nice if we could report some kind of warning to the user when compiling the regular expression fails. For a complex regular expression, it could be hard to tell the difference between "My regexp has a logical bug and isn't matching what it should" and "My regexp has a syntax error and has deformed to a plaintext search." Maybe a Rails alert would be good for this?

That seems to make sense. Added a rails alert (and updated the "no match" integration test).

The alert text says "Searching for files named…" but that's not strictly correct; it's more like "Searching for files whose names include…" I also personally think the alert would look slightly better as one paragraph instead of two, but that's highly subjective and nitpicky and not required for merge.

  • Assuming that we stick to server-side filtering, it might be nice if the filter was applied before the take(10000) in _show_files.html.erb. That would give users a way to see and select files in very large collections that might be unreachable otherwise.

I think we can do that. Updated.

Sweet, thanks.

Actions #8

Updated by Tim Pierce almost 8 years ago

Brett Smith wrote:

Tim Pierce wrote:

  • For consistency with our other search fields, I think the filter button should be attached to the right of the text box.

I've been trying to figure out how to do that but with no luck. I tried copying the code for the search box but it didn't work to put the buttons inline. If I have time I'll get back to this. Maybe one of our CSS gods will know the right answer.

Try this:

[...]

As per discussion in the office: this worked with the addition of <div class="col-lg-3"> to force it into a column of a specific width. Fixed at 5509974 -- thanks.

  • It would be really nice if we could report some kind of warning to the user when compiling the regular expression fails. For a complex regular expression, it could be hard to tell the difference between "My regexp has a logical bug and isn't matching what it should" and "My regexp has a syntax error and has deformed to a plaintext search." Maybe a Rails alert would be good for this?

That seems to make sense. Added a rails alert (and updated the "no match" integration test).

The alert text says "Searching for files named…" but that's not strictly correct; it's more like "Searching for files whose names include…" I also personally think the alert would look slightly better as one paragraph instead of two, but that's highly subjective and nitpicky and not required for merge.

It seems like the distinction will be clear in practice, when the user's search for "a[b" turns up files named "mamalama[bananawama" and the like. I like the less wordy explanation as one that's easier to grasp at a glance, but if you find this likely to be confusing then I'll change it.

Actions #9

Updated by Brett Smith almost 8 years ago

Tim Pierce wrote:

It seems like the distinction will be clear in practice, when the user's search for "a[b" turns up files named "mamalama[bananawama" and the like. I like the less wordy explanation as one that's easier to grasp at a glance, but if you find this likely to be confusing then I'll change it.

Not that big of a deal. 5509974 is good to merge (with the caveat it doesn't close the ticket, since the de/select all parts still need implementing). Thanks.

Actions #10

Updated by Tom Clegg almost 8 years ago

Brett Smith wrote:

Tim Pierce wrote:

Rewriting the tab that way would involve a substantial rewrite of the _show_files code. I'm willing to take it on but it's definitely a larger story than this one. I do think it's the preferable approach.

Yeah, I think we would ideally get Tom to weigh in on this. I'll try to flag him.

Is there a reason not to do the filtering in the browser? That would be way quicker, and have zero server impact...

Actions #11

Updated by Tom Clegg almost 8 years ago

  • Description updated (diff)
Actions #12

Updated by Tom Clegg almost 8 years ago

  • Target version changed from 2014-10-29 sprint to 2014-11-19 sprint
Actions #13

Updated by Tim Pierce almost 8 years ago

Summary of everything that's happened in the last couple of days:

Updated this branch at 50efff371 with changes to implement client-side filtering, using the magic in app/assets/javascripts/filterable.js.

  • Designated filename rows as filterable
  • Designated the filename pattern input field as filterable-control
  • Added Select all and Unselect all buttons with matching select_all_files() and unselect_all_files() actions
  • Limit selection actions (compare, move, copy etc) to visible page elements only (so "select all" and then filtering a subset of files will only act on that subset, not on checkboxes that are selected but now hidden).
  • Added integration tests "Filtering collection files by regexp" and "Creating collection from list of filtered files".
Actions #14

Updated by Brett Smith almost 8 years ago

Reviewing 50efff3

  • It looks like the Selection pulldown, Select all button, and Deselect all button are supposed to run in a contiguous bar, but don't quite get there. There's no margin between them, but the border between Selection and Select all doesn't collapse like the border between De/select all; and the corners of Selection are rounded while the corners of Select all are not, like they're expected to join the borders of adjacent widgets. I don't have strong opinions about how this gets fixed, but the current presentation just looks "off" so I feel like it should be addressed one way or another. I've attached a screenshot from my current Firefox if it's helpful.
  • I don't like being a broken record about this, but changing the placeholder text to "filename filter" brings me back to my earlier concern that it's not sufficiently clear that this field is expecting a regular expression. Especially since people asked about using shell globs when this was shown at sprint review, I still feel like it's worthwhile to be clear about the expected input format.
  • According to the Capybara docs, assert page.has_no_text? is always preferable to refute page.has_text? because that will give AJAX events a little time to settle if the assertion fails at first.
  • It might be nice to refactor the four-line pattern of "assert selector exists; loop all the checkboxes with that selector; assert/refute they're checked" into a method. Something like def check_checkboxes_state(test_method, selector, msg=nil).

Thanks.

Actions #15

Updated by Tim Pierce almost 8 years ago

Brett Smith wrote:

Reviewing 50efff3

  • It looks like the Selection pulldown, Select all button, and Deselect all button are supposed to run in a contiguous bar, but don't quite get there. There's no margin between them, but the border between Selection and Select all doesn't collapse like the border between De/select all; and the corners of Selection are rounded while the corners of Select all are not, like they're expected to join the borders of adjacent widgets. I don't have strong opinions about how this gets fixed, but the current presentation just looks "off" so I feel like it should be addressed one way or another. I've attached a screenshot from my current Firefox if it's helpful.

The styling is definitely kind of weird here -- I think it's a consequence of having three buttons in a single btn-group where one of them is a dropdown in disguise. I've moved the "Select all" and "Unselect all" buttons into their own group, which makes the style look a little cleaner. Let me know if you feel like this is enough of an improvement.

  • I don't like being a broken record about this, but changing the placeholder text to "filename filter" brings me back to my earlier concern that it's not sufficiently clear that this field is expecting a regular expression. Especially since people asked about using shell globs when this was shown at sprint review, I still feel like it's worthwhile to be clear about the expected input format.

Not a broken record at all, it's a very good point. Does "filename regex" give enough of a hint? (I know "regex" is awfully jargony but I think our users will know enough that they can work with that.)

  • According to the Capybara docs, assert page.has_no_text? is always preferable to refute page.has_text? because that will give AJAX events a little time to settle if the assertion fails at first.

Thanks for reminding me. Removed refutations.

While I'm at it, removed a stray 'refute' in jobs_test.rb. This one was probably okay since it came after a wait_for_ajax call, but we might as well be strict about avoiding 'refute' in our integration tests everywhere.

  • It might be nice to refactor the four-line pattern of "assert selector exists; loop all the checkboxes with that selector; assert/refute they're checked" into a method. Something like def check_checkboxes_state(test_method, selector, msg=nil).

That's a nice idea. Added and refactored.

New revision at 5b5c9a4

Actions #16

Updated by Brett Smith almost 8 years ago

Tim Pierce wrote:

The styling is definitely kind of weird here -- I think it's a consequence of having three buttons in a single btn-group where one of them is a dropdown in disguise. I've moved the "Select all" and "Unselect all" buttons into their own group, which makes the style look a little cleaner. Let me know if you feel like this is enough of an improvement.

Yeah, that definitely works for me.

Not a broken record at all, it's a very good point. Does "filename regex" give enough of a hint? (I know "regex" is awfully jargony but I think our users will know enough that they can work with that.)

I think that's fair, yeah.

Please merge 5b5c9a4. Thanks.

Actions #17

Updated by Tim Pierce almost 8 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:308a6da1a9fd716f3957b116110a932c08aefafe.

Actions

Also available in: Atom PDF