Story #3531

[Workbench] On projects#show, sort {jobs and pipelines} by date instead of listing all jobs before any pipelines.

Added by Tom Clegg about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
08/10/2014
Due date:
% Done:

100%

Estimated time:
(Total: 5.00 h)
Story points:
0.5

Subtasks

Task #3558: Interleave jobs and pipelines, sort by created_at onlyResolvedTom Clegg

Task #3559: Write testsResolvedTom Clegg

Task #3561: Review 3531-sort-jobs-with-pipelinesResolvedTom Clegg

Associated revisions

Revision b6e28fbe
Added by Tom Clegg about 5 years ago

Merge branch '3531-sort-jobs-with-pipelines' closes #3531

History

#1 Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress

#2 Updated by Brett Smith about 5 years ago

  • Assigned To set to Tom Clegg

Reviewing 6060e47

  • 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?
  • 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?)
  • 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.
  • 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.
  • 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).

#3 Updated by Tom Clegg about 5 years ago

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

#4 Updated by Brett Smith about 5 years ago

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.

#5 Updated by Tom Clegg about 5 years ago

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.

#6 Updated by Tom Clegg about 5 years ago

Unique element IDs fixed in 588a450

#7 Updated by Brett Smith about 5 years ago

588a450 is good to merge. Thank you.

#8 Updated by Anonymous about 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:b6e28fbe905737bd0ea5bda5f4fd74fd259c58b1.

Also available in: Atom PDF