Idea #3531
closed[Workbench] On projects#show, sort {jobs and pipelines} by date instead of listing all jobs before any pipelines.
Added by Tom Clegg about 10 years ago. Updated about 10 years ago.
Updated by Brett Smith about 10 years ago
- Assigned To set to Tom Clegg
Reviewing 6060e47
load_contents_objects
seems to assume that if we order bycreated_at
, it's alwaysdesc
. 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).
Updated by Tom Clegg about 10 years ago
Brett Smith wrote:
load_contents_objects
seems to assume that if we order bycreated_at
, it's alwaysdesc
. 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
Updated by Brett Smith about 10 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 bycreated_at
, it's alwaysdesc
. 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.
Updated by Tom Clegg about 10 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, thetab_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.
Updated by Anonymous about 10 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|commit:b6e28fbe905737bd0ea5bda5f4fd74fd259c58b1.