Arvados

Brett Smith wrote:

• load_contents_objects seems to assume that if we order by created_at, it's always desc. Should we check that before reversing the sorted results array and generating @next_page_filters? Or just make it part of the starting regexp condition?

Good plan. Added support for (implicit or explicit) "asc".

• Where you render partial: 'show_tab_contents', there's a mispaste: tab_name is usually 'Data_collections', but it should be unique to each partial. (Maybe show_tab_contents should get this from @params[:tab_name] or something like that?)

Indeed, whoops. I've fixed this by rearranging the tab_pane stuff (quite a bit more than I expected). Now the tab pane partials are given a tab_pane local which they can use to make unique element IDs.

While rearranging this stuff, I had to fix something that's been bothering me for a while: "tab_line_buttons" getting built by the leftmost tab's partial, rather than the appropriate show/index partial. See 41e77a9

• In the Sharing tab, when we fetch users and groups, should we call .limit(\@share_links.size)? Maybe add a small buffer to allow for some links added in the meantime? I don't think it should be common for projects to have tons of sharing links, but I'm guessing it'll happen sooner or later, and this would help keep the name rendering working.

Added a limit of 10000. (I don't think it matters whether new share links appear -- we won't display the names anyway if we didn't get the share links -- but I didn't feel confident that we will only ever find one permission link per user/group.)

• The bit in 'projects#show tab infinite scroll partial does not group object types' where you iterate over an array and then call #count on the return value is pretty dense. Please consider breaking it up.

Good point. Split it.

• Testing that results aren't grouped by kind seems redundant. We already test that they're ordered strictly by timestamp; that's the behavior we care about, and I think it should necessarily imply that they're not grouped by kind (given the improvements you made to the test fixtures—thanks for that).

I added a comment here to justify/explain it (confirm that the fixtures are actually good enough to test the behavior). I suspect factories would be better than fixtures for this sort of reason, but meanwhile "check that we have the right fixtures" seems like decent insurance value.

Now at 41e77a9

Thanks for all the changes. Just a couple of smaller comments (I hope).

Tom Clegg wrote:

Brett Smith wrote:

• load_contents_objects seems to assume that if we order by created_at, it's always desc. Should we check that before reversing the sorted results array and generating @next_page_filters? Or just make it part of the starting regexp condition?

Good plan. Added support for (implicit or explicit) "asc".

This version still reverses objects in all cases, which sorts them descending. I'd also like to see the regexp anchored with $@ at the end, to help avoid false positives in case of weird input.

• Where you render partial: 'show_tab_contents', there's a mispaste: tab_name is usually 'Data_collections', but it should be unique to each partial. (Maybe show_tab_contents should get this from @params[:tab_name] or something like that?)

Indeed, whoops. I've fixed this by rearranging the tab_pane stuff (quite a bit more than I expected). Now the tab pane partials are given a tab_pane local which they can use to make unique element IDs.

I like the thrust of this, but it seems to have a similar problem in some uses. The projects partials all call render_pane 'tab_contents'. If I've followed right, the tab_pane local will be set to 'tab_contents' in all of them, and the element IDs that are supposed to be unique won't be.

Brett Smith wrote:

This version still reverses @objects in all cases, which sorts them descending.

Good point, fixed. And added a test.

I'd also like to see the regexp anchored with $ at the end, to help avoid false positives in case of weird input.

Hm, somehow I had imagined that desc was just an acceptable abbreviation of descending and I was being clever, but apparently the keyword is just plain desc. Fixed.

• Where you render partial: 'show_tab_contents', there's a mispaste: tab_name is usually 'Data_collections', but it should be unique to each partial. (Maybe show_tab_contents should get this from @params[:tab_name] or something like that?)

Indeed, whoops. I've fixed this by rearranging the tab_pane stuff (quite a bit more than I expected). Now the tab pane partials are given a tab_pane local which they can use to make unique element IDs.

I like the thrust of this, but it seems to have a similar problem in some uses. The projects partials all call render_pane 'tab_contents'. If I've followed right, the tab_pane local will be set to 'tab_contents' in all of them, and the element IDs that are supposed to be unique won't be.

Hmph. Still only 3/4-baked! I'll revisit.

Unique element IDs fixed in 588a450

588a450 is good to merge. Thank you.