Bug #6640

[Workbench] Ensure projects menu consistently shows all "my projects"

Added by Brett Smith about 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
08/05/2015
Due date:
% Done:

100%

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

Description

Background

I (Brett) have attached two screenshots of the projects menu pulldown in two tabs opened back-to-back in the same session. They don't show the same list of projects. Presumably this has something to do with me being an admin, and there's some explicit or implicit limit on the number of projects Workbench renders.

At the very least, the menu should always render all of my own projects, and it should always render the same set of projects. We can hash out how we want to deal with large project lists generally.

I (now Tom) am guessing the "unpredictable subset of projects shown" bug means Workbench is fetching only one page of projects from the API, and without specifying a predictable order. But the real problem here is that we are trying to show the entire set of readable projects in a dropdown in the first place. Rather than try to make the bogus UI work better, we're going to change the UI.

Implementation

Instead of trying to show a large number of projects in a dropdown the "projects" drop-down should reduce to:
  • a "Search all projects (N)" item, where N is the total number of projects readable by the current user. This opens the "search projects" dialog, much like the existing "move to project..." feature does but with a "Show" action button instead of "Move".
  • a "Browse public projects" item. Just like we have now, but formatted as a menu item instead of a button.
  • a "Create a new project" item. Just like we have now, but formatted as a menu item instead of a button.
  • a heading "My projects"
  • the current user's "Home" project
  • the hierarchy below it (just like we have now)
  • no list of "projects shared with me"

Similarly, the "project" dropdown in the "search" dialog should lose the "projects shared with me" part: only "All projects" and the "My projects" hierarchy will remain.

This will leave us without a hierarchical view of "projects shared with me", which is probably a good thing. The hierarchy of shared projects looks different depending on (for example) whether the common parent of two shared projects is visible, so this view is inherently unpredictable.

The current API doesn't offer an efficient way to get a list of the current user's project hierarchy. For now, we'll just do it the way we do now (i.e., do multiple API calls to get all projects, arrange them into trees, and just show the tree whose root is current_user.uuid) in order to improve the UI. Adding a more efficient API might be done concurrently or in the future, but is not a dependency of this UI change in any case.

Meanwhile this story does require a new test case (and perhaps some fixtures) that specifically confirms the original bug here is fixed: i.e., if a user has more than one page worth of projects, possibly even in the "My projects" hierarchy, the same subset gets loaded every time (presumably this subset will be the full set).

Brett projects 1.png (15.5 KB) Brett projects 1.png Brett Smith, 07/16/2015 11:37 AM
Brett projects 2.png (12.7 KB) Brett projects 2.png Brett Smith, 07/16/2015 11:37 AM
project_menu.png (53 KB) project_menu.png Manoj Malipeddu, 08/10/2015 06:41 PM
alternate_project_menu.png (52.2 KB) alternate_project_menu.png Manoj Malipeddu, 08/10/2015 07:28 PM
project_menu.png (56.3 KB) project_menu.png Manoj Malipeddu, 08/11/2015 01:49 PM

Subtasks

Task #6898: Review 6640-projects-menu-incompleteResolvedManoj Malipeddu

Task #6944: Mock up specified UIResolvedManoj Malipeddu

Task #6945: Review UI design mocksResolvedManoj Malipeddu

Associated revisions

Revision 36b7f52b
Added by Manoj Malipeddu about 4 years ago

refs #6640
Merge branch '6640-projects-menu'

Revision 1cecdfb6
Added by Manoj Malipeddu about 4 years ago

closes #6640
Merge branch '6640-all-projects-dup'

History

#1 Updated by Brett Smith about 4 years ago

  • Target version changed from Arvados Future Sprints to 2015-08-19 sprint

#2 Updated by Brett Smith about 4 years ago

  • Story points set to 3.0

Discussed writing a separate infinite scrolling, filterable list page. Tom to write a spec. Story pointing based on the outline discussed.

#3 Updated by Tom Clegg about 4 years ago

  • Description updated (diff)

#4 Updated by Radhika Chippada about 4 years ago

  • Assigned To set to Manoj Malipeddu

#5 Updated by Manoj Malipeddu about 4 years ago

  • Status changed from New to In Progress

#6 Updated by Manoj Malipeddu about 4 years ago

Snapshot of new project menu.

#7 Updated by Manoj Malipeddu about 4 years ago

A different option for the project menu.

#8 Updated by Tom Clegg about 4 years ago

Looking good. Couple of requests:
  • "alternate_project_menu.png" is better in every way. (The story description asked for "Create" to be under "Home" but now that I see it in black and white I think the way you've done it here is much better.)
  • The first three items ("Search", "Browse", and "Create") don't need separators between them.
  • I think the set of three Search/Browse/Create will look even better if "Browse" gets an icon, too. Maybe "open book"??
  • Use fa-fw on the icons so the text lines up.
  • Between the Search/Browse/Create section and the list of actual projects, there should be a separator and a "My projects" heading (formatted just like the "Projects shared with me" heading is now).
  • The "Projects shared with me" heading should go away.

#9 Updated by Manoj Malipeddu about 4 years ago

Updated project menu.

#10 Updated by Tom Clegg about 4 years ago

Looks great, thanks! (Assuming the separator and the shared projects listed at the bottom go away of course -- I assume I'm not supposed to be looking at that part)

#11 Updated by Tom Clegg about 4 years ago

  • Subject changed from [Workbench] The projects menu is randomly incomplete to [Workbench] Ensure projects menu consistently shows all "my projects"
  • Description updated (diff)

#12 Updated by Manoj Malipeddu about 4 years ago

In 2af4936125781b0e60ce66f47f45ed6f856bcda0:

Project menu has been updated to have three new menu items and only show projects owned by the user.

Added test "project menu shows all projects owned" in projects_test to check if all projects are being shown.

Fixed other tests to use new links on project menu, and check new links.

Added a fixture for 301 projects owned by user1_with_load.

#13 Updated by Tom Clegg about 4 years ago

New menu

Looks good. Two small requests:

I get a "text cursor" pointer when my mouse is on "Search" and "Create". This seems to be related to the Rails unobtrusive (hah) JS stuff: the A elements don't have href attributes so Chrome thinks they're anchors but not links. It seems we can deal with this by putting .dropdown-menu a { cursor: pointer; } in app/assets/stylesheets/application.css.scss.

Does "copy_from_search_box: true" do something here? It sounds like something we don't want, but afaict it doesn't do anything at all. Perhaps it was already dead code in the top-nav search?

Tests for new menu

Is there a test for the new "Search projects" button?

Instead of assert page.has_text?('My projects'), how about something more specific that won't conflict with other stuff on the page in the future, like

assert_selector '#projects-menu li.dropdown-header', text: 'My projects'

"Too many projects" bugfix

We need to figure out how to fetch multiple pages if necessary to get all projects. We have a mechanism in Workbench for fetching multiple pages, and it looks like it's enabled here. Why isn't it working? Increasing limit to 10000 is not an acceptable way to make the question go away. Some things to investigate:
  • Is workbench really making multiple API calls like it should be? What do the logs say?
  • If there are three pages of groups, does it get all three pages, or is it stopping after two pages? (Reducing limit to 1 might make this easier to test.)

One possibility is that we're not asking for a sort order on the projects, and our "page1" and "page2" results are coming from Postgres results that aren't sorted the same way, so we're getting some results more than once and others not at all. If this is what's going on, sorting by "created_at ASC" might fix it.

Adding 300 more fixtures in order to test this seems a bit silly: all we're doing is proving that either we use limit=10000 when we make that API call and the API doesn't restrict it to less than 300, or the effective page size is less than 300 but our multi-page code is working (but we don't know which). A spy/mock could tell us exactly what's going on, without adding any fixtures.

I'd rather see a test case that mocks the API call to use a lower limit (after all, the API isn't guaranteed to return 10000 results just because there are 10000 results and you asked for 10000) to make sure the paging stuff gets invoked even though there aren't very many test fixtures. But the choice of test case will depend on what the bug turns out to be, so we'll see...

The "project menu shows all projects owned" test looks like it's trying to test infinite scroll, but that's unnecessary, right? All of the projects should be right there on the page as soon as it loads. And in that case, this should be a functional test instead of an integration test.

It should also do better than counting all LI elements that happen to exist anywhere on the dashboard page -- it should only be counting the items in the projects dropdown that come after the "my projects" heading.

#14 Updated by Radhika Chippada about 4 years ago

  • Finally, after much digging, identified the root cause of the issue that resulted in the two different Project dropdown lists in Brett1 and Brett2 images. In fact, the answer is right in front of us actually. Brett1 is the project dropdown when in the Dashboard. Brett2 is when Brett traversed to a specific project, Home project in this case.
    • The project dropdown shows the results from all_projects helper method from application_controller.
    • When the dashboard is shown, projects#index is loaded which invokes all_projects method with an @limit of 200. When a project is visited, @limit is nil when the all_projects helper method is executed.
    • There are currently 333 projects in qr1hi and the retrieval of the first 200 projects caused the disparity in both these cases.
    • It appears that best resolution for this issue would be to set @limit to nil in all_projects helper method in application_controller, which will result in fetching all or first 1000 (server MAX_LIMIT) matching the sort order.
  • Also, verified a few things during this debugging process.
    • Confirmed that fetch_multiple_pages working as expected.
    • Confirmed that "sort order" is working when included in the api request

#15 Updated by Manoj Malipeddu about 4 years ago

In 041fb50eb0b55d37bd165bec25a2285183326cce:

Cursor is now a pointer when hovering over add and search buttons. Removed unnecessary code from body.

Added a test for search all projects in projects_test. Changed assertion assert page.has_text?('My projects') to check that my projects is a dropdown header. The "project menu shows all projects owned" test no longer tests for infinite scroll and was moved to projects controller test.

Fixed bug so that all pages get all projects in projects dropdown.

#17 Updated by Tom Clegg about 4 years ago

Radhika Chippada wrote:

  • When the dashboard is shown, projects#index is loaded which invokes all_projects method with an @limit of 200. When a project is visited, @limit is nil when the all_projects helper method is executed.

I don't see where ProjectsController's @limit attribute is being used in the all_projects helper:

  helper_method :all_projects
  def all_projects
    @all_projects ||= Group.
      filter([['group_class','=','project']]).order('name')
  end

AFAICT, the added test does not exercise this interaction between projects#index and the helper method, nor does it fail when I revert the change to all_projects. I'm not convinced by this diagnosis/fix, but if I see a test that reproduces the bug, that'll definitely help.

#18 Updated by Brett Smith about 4 years ago

  • Target version changed from 2015-08-19 sprint to 2015-09-02 sprint
  • Story points changed from 3.0 to 0.5

#19 Updated by Manoj Malipeddu about 4 years ago

  • Target version changed from 2015-09-02 sprint to 2015-08-19 sprint
  • Story points changed from 0.5 to 3.0

In 60aa782479f0f97d880bddec1bb35a397f06707e:

In branch: 6640-projects-menu-incomplete-bugfix
Added tests in projects_test to reproduce bug.

#20 Updated by Manoj Malipeddu about 4 years ago

  • Target version changed from 2015-08-19 sprint to 2015-09-02 sprint
  • Story points changed from 3.0 to 0.5

#21 Updated by Tom Clegg about 4 years ago

OK! This (60aa782) is an excellent demonstration. I see now how @limit was infecting the all_projects list.

See 6640-all-projects-dup. I think 1569639 is the fix -- and it also covers various other filter/order/limit/offset parameters that would otherwise have the same mysterious effect.

I think the appropriate test for this is a controller test that verifies any client-provided limit, offset, filters don't get applied to the "my projects" menu. This could be done by calling projects#index with filters that don't match any results -- I expect that will cause the projects menu to be empty without the bugfix, and correct with the bugfix. (Test added in 9a4c7cb)

It would be even better to fix the ArvadosResourceList API so the caller doesn't have to remember to dup in cases like this -- i.e., each method like limit() acts more like merge(), returning a new ArvadosResourceList object instead of modifying itself in place -- and it might not even be that hard. If we can't deal with that, I wonder if there is at least a systematic way we can identify similar usage errors that are causing (or likely to cause) similar bugs?

set_a = Fubar.filter [['foo', '=', 'bar']]
set_b = set_a.filter [['baz', '=', 'waz']]
set_c = set_b.limit 5
set_a.each do |x|
  puts x
end

# Naturally expected behavior:
#  print all results matching foo=bar

# Actual behavior:
#  print <= 5 results matching foo=bar && baz=waz

#22 Updated by Manoj Malipeddu about 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 67 to 100

Applied in changeset arvados|commit:1cecdfb6fabf47c921e7f422063368621619dcfb.

#23 Updated by Manoj Malipeddu about 4 years ago

  • Status changed from Resolved to In Progress

#7100 is created to address the issue with ArvadosResourceList.

#24 Updated by Manoj Malipeddu about 4 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF