Arvados

• "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.

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)

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.

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¶

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

• 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

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.

In 2c630802a9bf570c3ced2f7d42c2346ef814b578:

Reverted bugfix.

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.

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