Arvados

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.

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.

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.

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.

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.

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...

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".

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

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.