Arvados

I've managed to create something that seems like a good start. You can click on 'name' in the Collections tab, as well as the Jobs and Pipelines tab, and sort ascending or descending. An icon will indicate the current direction.

To use the new mechanism, add to a call to render_pane the sortable_columns option. It is a hash keyed by column header codes. Currently the project tabs' contents all hard-code name and description so these are likewise hard-coded in this option hash.

The basic committed example case is data collections, where clicking on 'name' will add 'collections.name asc' to the ordering of the select call to the collection model.

Another example is jobs and pipelines, which involves two models, so the sortable_column value for 'name' is set to 'jobs.script, pipeline_instances.name'. This means that the select to pipeline_instances will be sorted by name, and the select to jobs will be sorted by script. See the comments in Workbench's application_controller#load_filters_and_paging_params method.

Not exemplified but now possible is to pass an 'order' option to the render_pane method, which will override the default ordering. Note that in this case if the order is preset like this in the call to render_pane that the ordering will not be reflected by the icon shown next to the column header.

It should be noted though that this whole mechanism can only do SQL ordering, since the ordering occurs in the final SQL call, so virtual attributes can't be used for sorting.

A possible improvement for a next iteration might be a way for it to remember the sort order that was last selected when switching from one tab to another.

I should like to sanity-check the direction this went in and hear comments about possible problems or improvements before continuing, since posibly this will be sufficient enough for now, except probably to add a test or two.

On reviewing 4e62496, I think this approach is solid and worth continuing with. My comments so far:

• On a project page, when I switch away from a tab with sortable columns, then switch back, the contents table in that tab refreshes. This could add noticeable overhead to browsing, especially if I did significant scrolling in the tab before I switched away. Is it possible to avoid this?
• In the table ERB, why do the sort orderings need to be rendered raw? I'm concerned this could potentially open us up to XSS later, if we ever generate sort columns based on user input.
• From a code discoverability standpoint, I'm not sure I'd go looking for this JavaScript in infinite_scroll.js, since it has to do with sorting more than scrolling. Maybe the fix is to rename the file to reflect that it includes JavaScript for data tables generally?
• I think we like to use === and !== in our JavaScript as much as possible to avoid strange type coercions.
• As you suggested, it'd be good to API server tests and a Workbench integration test for these additions.

Thanks.

• Switching tabs causes refresh: this is true and I tend to agree, but the problem is not due to this feature addition: this was already the way it worked beforehand. I should think this would need to be a new issue, and anyway more closely related to the work I did on #3634.
• XSS: I know what you mean, but when I considered it I realized that since the columns required by the code here are actually database table columns, which are entirely internal, it is not really likely that we'd ever opt for the user to specify them directly. There will always have to be a "translation" of sorts, between a limited set of virtual columns that the user can sort by and what actual database columns will need to be internally appended to the SQL for the ORDER BY clause to work. I've tried to anticipate this by using the "sortable_columns" hash, which performs this translation. The user may end up specifying the key(s), but it is the value(s) from the sortable_columns hash that will be rendered raw.
• Name of javascript file: I agree. What about "infinite-tables.js"?
• === and !==: Very good point and done.
• Tests: Coming up next.

Thinking further about the sortable_columns hash in Workbench, I wonder if we shouldn't more strongly consider having this "translation" be in the API Server rather than in Workbench. It's only that when trying to resolve this particular Redmine issue I judged it best to make the changes as closely as possible to the existing framework, so that this feature could be added in an incremental fashion. And as it stood, Workbench was already sending ORDER BY clauses pretty much directly to the API Server. To implement this feature with minimal impact, I have used the existing framework. Perhaps here too we should create a new issue where all of the "translation" to actual internal database columns is performed only on the API Server, and Workbench/CLI clients will always use the virtual column names of "name" and "description" and so on. This would be most consistent but would require farther-reaching changes.

So, as it stands now, to use the new sort feature from the CLI client will require knowledge of internal table and column names, just as before.

The "description" columns in the various Project tabs, I'm discovering, are a bit strange, in that they do not seem to always be the contents of the "description" column of the underlying table, even if the description can be edited by the user in Workbench, etc.

As a result, I wonder if this solution, which revolves around sorting on actual database columns, is going to suffice for all our needs, or whether the ability to sort on virtual attributes should also be introduced. We'd also have to determine if we want to be able to sort on API model virtual attributes and/or Workbench model virtual attributes.

Phil Hodgson wrote:

• Switching tabs causes refresh: this is true and I tend to agree, but the problem is not due to this feature addition: this was already the way it worked beforehand. I should think this would need to be a new issue, and anyway more closely related to the work I did on #3634.

Yeah, my bad, I noticed this later. Apologies. I agree that fixing it is a separate thing.

• XSS: I know what you mean, but when I considered it I realized that since the columns required by the code here are actually database table columns, which are entirely internal, it is not really likely that we'd ever opt for the user to specify them directly.

I agree with all of this, but I'm not sure addresses the issue. Are there database columns with characters that we need to render raw? If not, why go through the extra step that increases our risk?

• Name of javascript file: I agree. What about "infinite-tables.js"?

Sounds good to me.

Thinking further about the sortable_columns hash in Workbench, I wonder if we shouldn't more strongly consider having this "translation" be in the API Server rather than in Workbench.

Feel free to run this idea by Tom, but personally I'm not seeing it. My feeling is that the columns that are not directly database-backed are Workbench-specific, and it wouldn't be a good separation of concerns to teach the API server about those columns and have it deal with them. Plus it means that whenever we change how the columns work in Workbench, we'd have to simultaneously update the API server, which is the sort of interdependency that has caused us deployment trouble in the past.

As a result, I wonder if this solution, which revolves around sorting on actual database columns, is going to suffice for all our needs, or whether the ability to sort on virtual attributes should also be introduced.

I've asked Tom to clarify which columns need to be sortable for this ticket.

Thanks for the great feedback.

• The "raw" rendering of database columns: apologies for wasting our energy; this was a remnant from a previous experimental sub-iteration; you are absolutely correct and I've removed the raw directives.

• As for the "translations" of columns (and the related problem of virtual Workbench attributes), I see your point. I wasn't thinking about teaching the API server about Workbench virtual attributes, but rather just not having to teach Workbench about columns otherwise internal to the API server. Anyway, perhaps that's a rather large undertaking, seeing how far-reaching is Workbench's entanglement with the API server, so perhaps for this iteration of this feature we can keep it more or less as is. We can continue to teach Workbench about the API servers table structures. If we need virtual attribute sorting, we'll have to limit the extent to which this is possible to whatever database column names can be passed on to the API server, because otherwise obviously we lose the benefits of fetching the source rows in batches. We'll see what Tom has to say about which columns really need to be sortable and analyze it from there.

• I'll do the rename to "infinite-tables.js" right before I merge.

Today I'll move on to looking into subtask #4320 - I am thinking of starting with seeing how satisfying an URL rewriting mechanism would be when sorting, since at least then doing a browser refresh and a browser back will behave in a sensible fashion.

Reviewing c10b68e

• There seems to be a bug in the history handling for the Description column. Steps to reproduce, from a project page:
  1. Order a table by description.
  2. Open another tab on the page.
  3. Reopen the tab ordered by description. The table will be empty. It never loads, and it doesn't have sort arrows for any of the columns.
• load_filters_and_paging_params seems to expect that params[:order] might be an array, but it doesn't do anything in that case. Should it set @order = params[:order]?
• In the API server's load_param.rb, please have the new branch use the same safety checks as the if clause above it (some refactoring may help).
• Please refactor your new integration test to share code with the general scrolling tests. While you're at it: visit page_with_token can specify the desired URL as a second argument, after the token name. Going directly to the project page would save us some test overhead, which is becoming a real issue.

Thanks.

Thanks again for the great feedback Brett. I've addressed all of the points you've raised. The refactoring of the scroll tests could probably be taken further, but I'm already exceeding 4 days work on this puppy. I can say that all of the tests that already existed still exist and are working, and the sorting stuff from this redmine is also tested via the collections.

Thanks Phil, I like all the changes as of 2a48239. Just one more thing: changing the test fixture indices has broken the "pipeline start and finish time display" tests at the bottom of test/integration/pipeline_instances_test.rb. I think any fix that tests the same time durations would be fine, whether that means changing the fixtures or the test setup.

pipeline_instances_test.rb now works with the new fixture indexing. Thanks!

Phil Hodgson wrote:

pipeline_instances_test.rb now works with the new fixture indexing. Thanks!

But you did it by testing different time durations. Those durations are the most interesting part of the test: we want to test a pipeline that runs for 0 seconds, because that's an obvious corner case; and we want to test a pipeline that runs for more than 12 but less than 24 hours, to test proper handling of the AM/PM boundary. I guess the change for the 9→10 index is fine, but please restore the test for a 0-second pipeline.

No problem! This now uses the same run-times as before.

Phil Hodgson wrote:

No problem! This now uses the same run-times as before.

Perfect, thanks. db2411a5 is good to merge.