Arvados

Radhika Chippada wrote:

One thought: How about we add a right-justified button/dropdown menu in the breadcrumbs navigation (Dashboard and Projects dropdown area)? If the user has more than one shell account, this can be a "Shell login" dropdown menu (similar to Projects dropdown). Otherwise, it can be directly clickbale.

This is just my two cents as an engineer: We already have some situations where we can't fit everything we want on the breadcrumbs menu. Because of that, I'd be reluctant to add more to it.

What if a Web shell login link was added to your user menu for each shell you had access to? Appended to that menu, "Log in to <shell name>" for each shell you can use.

• Home project (same as current)
• Virtual machines
  • Same content as the "VMs" panel on the current "manage account" page, plus a note/link re adding SSH keys
• Git repositories
  • Same content as the current "Repositories" panel
• SSH keys
  • Same content as the current "SSH keys" panel
• API token
  • Same content as the current "Current token" panel
• Manage account (same as current -- yes, this is redundant, but it means the current docs still work)
• Manage profile (same as current)

This is helpful, thanks! I think I see what the next step is going to be in fixing the nav/breadcrumbs aspects. If we have time, maybe we can even do it this sprint. But let's get the pages we need first.

• Icons for repos/VMs/token menu items should come from the corresponding entries in the admin menu
• "Manage profile" should come back (perhaps this is missing only because your local config doesn't enable profiles?) and icon should change to "user" icon (this is just a "related bugfix" but might as well do it now)
• On SSH keys panel, get rid of the "Click here" (argh) sentence and just add a "Learn more" link at the end of the previous sentence. (Another "related bugfix")
• Add a sentence to the "current token" page
  • Arvados virtual machines [link to VMs page] do this for you automatically. This setup is needed only when you use the API remotely (e.g., from your own workstation).
• Move the sample SSH config out of the "Virtual Machines" panel. Replace the "Sample SSH Conig" text (whoops, how long has that been there!) with something more verbose, like
  • In order to access virtual machines using SSH, add an SSH key to your account [link to keys page] and add a section like this to your SSH configuration file (~/.ssh/config):
• Add a the SSH configuration instructions to the repositories page too, e.g.,
  • In order to clone git repositories using SSH, add an SSH key to your account [link to keys page] and add a section like this in your SSH configuration file (~/.ssh/config): [...]
  • When you are using an Arvados virtual machine, you should clone the https:// URLs. This will authenticate automatically using your API token.
  • (We could add a section about setting up git credential helpers here too, but it's also OK to set it aside for now until we have the more essential stuff worked out.)

(Actually the first three items don't really need mockups, as long as we remember them in the implementation.)

If those changes look as good in pictures as they do in my imagination, I think we can go ahead with implementation.

The screenshots look great and are quite helpful in understanding what the updated UI looks like. Thanks.

One comment after looking at the updated system_menu_updated.png.

• If we are renaming a few options in the admin settings menu to say "All repositories" etc, I think we need to update all of them to say "All xxx" to be consistent.

• Alternatively, we can call user settings menu options as "My virtual machines", "My repositories", "My home project" etc.

Thanks.

A few minor tweaks, and a realization I asked for something I shouldn't have asked for:

On "current_token_updated" the "do this for you automatically" sentence should be below the HISTIGNORE section. (And needs to fix "machines does this" → "machines do this".)

(Oh, assuming it's trivial, we should change "authenticate to your account, youremail@example.com" to "authenticate to your username account". The current text predates usernames.)

On "new_menu_updated" add the "fa-fw" class to the icons to make all the text line up. (This is Font Awesome's Fixed With feature)

On "virtual_machines_updated" it looks there's an extra colon in there, "(~/.ssh/config:):" should be "(~/.ssh/config):"

And (sorry) I just noticed the SSH setup instructions are actually wrong on the "repositories" page: that SSH configuration stuff doesn't have any effect when cloning from git@git.zzzzz.arvadosapi.com. So the "add a section..." text should change to "clone the git@ URLs." ...and the "Host *.arvados" example should be removed.

The rest looks good, thanks.

In da56d26d09a6b315bdb93aed4473310b939e1bd7:

Added new pages in notifications menu, and added a test for these new pages in application_layout_test.

In fc8a283968b08b152d57a24d2c5711c876a3b379:

Added tests to user_manage_account_test and modified tests in user_profile_test and user_manage_account_test to use manage account less.

Moved a test from application_layout_test to user_manage_account_test.

The admin-only "list all authorized_keys" page and the new "manage my keys" page should each have its own route.

• /authorized_keys → new "manage my keys" page
• /authorized_keys/show_all → old admin-only "show all keys" page (it doesn't need to be restricted to admins for security reasons, it's admin-only only in the sense that it isn't linked from anywhere unless you're an admin).

(Ditto for the virtual_machines, repositories, and tokens pages.)

This should give us nicer URLs and avoid putting logic in views like "if params['page'] ... else ..."

In cda68ba838b2595c631231a08a8e3b3ea03d2c7f

Changed links to use new routes, modified text in notification menu, changed tests and changed links in pages.

I did not want to edit the admin menu routes as part of this story so i just created 4 new routes.

At cda68ba...

I think the word "manage" is unnecessary in these routes. And if we are putting these routes under /users, we should make them more RESTful by doing this in config/routes.rb:

  resources :users do
    get 'choose', :on => :collection
    # ... other existing routes
    get 'manage_account', :on => :member
    get 'authorized_keys', :on => :member
    get 'repositories', :on => :member
    # ...
  end

("current token", doesn't make sense as a :member route because you can't see someone else's current token.)

•                 <li role="menuitem">
                    <%= link_to ssh_keys_user_path(current_user), role: 'menu-item' do %>
                      <i class="fa fa-lg fa-key fa-fw"></i> SSH keys
                    <% end %>
                  </li>
  

• "SSH keys", not "My SSH keys", see screen shots ;)
• You copied some of our code that has hard coded links, but we should be using the path helpers Rails gives us.

...and the controller methods can use @object instead of current_user (it will be populated for you by the find_object_by_uuid method in ApplicationController) ...and, once we patch up a few other pieces, admins will be able to look at other users' "SSH keys" pages so they can spot/fix problems, etc.

One thing you inherited from manage_account seems to be superfluous, both in manage_account and in the new methods. This is just what Rails does by default if you return from a controller action without rendering anything, so you can delete it:

    respond_to do |f|
      f.html { render template: 'users/manage_account' }
    end

Once you do that, you should be able to call the manage_X methods from manage_account. We shouldn't have two copies of that code.

    def manage_account
      repositories
      virtual_machines
      ssh_keys
    end

Aside: It seems like we should stop calling this the "notification menu" soon... should it be "user menu", "tools menu", "account menu"...?

In a789f895a5cdbc2a548787f99197f9861521106b

Changed routes to be under /users, used @object, removed rendering of templates and updated tests.

Looks great, thanks!

Couple of minor requests:

Fix indentation in the new test cases in apps/workbench/test/integration/user_manage_account_test.rb

• in the layout tests, while we're looking at the menu dropdown, check that the link targets are correct (currently we just test whether the links exist)
• in the integration tests, start the "repositories" test by visiting the repositories page, start the VMs page by visiting the VMs page, etc., instead of loading the whole front page in each test just to confirm that the menu dropdown works.

In 8f93d1332a25d29aa96211920e1b399a1e94482c:

Made the changes you recommended to the tests.

Could you change "/users/zzzzz-tpzed-l1s2piq4t4mps8r/virtual_machines" to "/users/#{users(:spectator).uuid}/virtual_machines", and ditto for the "active" user uuid?

Also - it looks like the "test notification menu for page #{page_name}" and "test notification menu for page #{page_name} when page is empty" tests still start at the front page and click the menu items, instead of going directly to the page being tested.

The "test notification menu for page #{page_name}" tests should use a normal user (e.g., "active"), not admin (sorry, I didn't notice this earlier).

That particular set of tests would be even better if they tested for some of the "real" content, like key name/fingerprint, the actual token being used, name of repository/VM, etc., in addition to the boilerplate text.

In cca0d86de72487988b1a7d657e67143121461a0f:

Go directly to pages instead of going through home page and look for more specific details when checking pagesl

LGTM, thanks! Running tests now after merging master, but assuming you've already done that, please merge.

In 89fccf123374c67e738381ea840e3535b2d1074f:

Got rid of manage account button and updated tests.

Review comments for "delete manage account button" at 89fccf123374c67e738381ea840e3535b2d1074f

• arvados/services/api/app/views/user_notifier/account_is_setup.text.erb has one reference to manage_account page. Please update this to use the user_uuid/virtual_machines page URL
  • It would not hurt to update arvados/services/api/test/functional/arvados/v1/users_controller_test.rb to assert the full link in the test "setup user with send notification param true and verify email"

• These are not show-stoppers and not breaking anything. But, can you please update all references to "manage account" with "user settings menu" in test/integration/user_settings_menu_test.rb? There are 5 such references.

Everything else looks good. Thanks.

In 44d4d43331979c87cee5df9ff952fd80a6e9c5f8:

Removed last use of manage account page and reworded tests to not use manage account.

Review comments:

• Can you please use the phrase "user setting menu" in apps/workbench/test/integration/user_settings_menu_test.rb instead of settings menu? I just want to make sure no one gets confused in the future with "admin settings" page.

• In services/api/app/views/user_notifier/account_is_setup.text.erb: I think we would need to check if config param ends with '/' or not and add it if it does not

• In services/api/test/functional/arvados/v1/users_controller_test.rb, please look for the full url <config_param>/users/<uuid>/virtual_machines instead of two different asserts

In bdabb9aa520b598107e319e51638f899f136aff5:

Used phrase "user setting menu" and checks for full link.

Just a few minor.

• Thanks for noticing the icon for the virtual machines menu item and fixing it :)

• In test "verify repositories for active user", you defined user object (from fixture), but did not use it in the visit line. Please update it to use the user object you defined.

• Same as above comment on test "request shell access"

You can merge after addressing those. Thanks.