Arvados

Reviewing 752b916. This is very nice, thanks. Just a few small things.

• I think we always want the "Advanced" tab to be the last tab in the pane list on Workbench when it appears. show_pane_list in the RepositoriesController adds "Sharing" after "Advanced."
• In ShareObjectHelper, please refactor the common code in show_repository_using and show_project_using.
• This bug existed before your branch, but I happened to notice it and it would probably be nice to deal with now: show_project_using can visit any project page, but the assertion at the bottom is specific to aproject. I think it might be best to just eliminate this assertion, but if you disagree, it should at least be made more flexible for the sake of future users.
• In the repositories integration test, test "#{user} can manage sharing for another group" doesn't actually use the user variable, making the loop redundant. I'm not sure whether the test should use the variable, or the loop should be eliminated—please take a look.

Thanks.

From IRC conversation between Tom and Radhika on Jan 23rd regarding the missing repository link for non-admin users:
radhika
It seems simple enough to address. In the Manage Account page, can I just make the repository Name column values links?
That is, I see "radhika" in my page right now. I just make it a link that takes me to that page?
tom
I'd suggest also making Repository return [] for editable_attributes, and losing the Attributes tab if the user isn't an admin. Link to #Sharing for good measure, although that'll become the first tab anyway. And don't link to it if it isn't "manage"able, because in that case there's nothing interesting there.
radhika
so if the user is not admin, he will see no tabs but for Sharing (provided writable)
tom
s/writable/manageable/ but yes. That's all that page can do, right? It would be awesome if it showed recent commits on the first tab, and if you want to throw that in, I'm all for it
yes, I think it's fine to leave the Advanced tab there. But the generic Attributes page is just confusing. It's advanced. We should hide it. IMO
radhika
and regarding recent commits, it would be a separate / new tab, i assume.
tom
yes. we have a bunch of code to import git commits, and we used to use it, but it's been turned off for a while. It's somewhat tempting... but I'm inclined to say you probably don't really want to go there right now. Probably a can of ... octopus merges?

Updated the "3686-sharing-repositories" branch to add a link to Repository page in the Manage Account page. Details about the update:

• When the user can_manage, the repository name in manage_account page acts as a link to that Repository page.

• The Repository page no longer displays the Attributes tab when the user is not an admin.

Note: Did not add a new tab for "recent commits" with the intent of doing it as a separate ticket as Tom suggested in the last comment.

Made all suggested enhancements. In addition, added an exclusive Share link instead of making the name clickable for repository rows.

Reviewing 1ceddc7. This is very nice; thanks very much for following up on everything.

• If I'm following correctly, it looks like the new repositories integration tests are redundant with the controller tests, and unnecessary. They both check that both users see the Advanced tab, but only admins see the Attributes tab. Let's just have the controller test.
• The test "verify repositories for user admin" doesn't assert anything. Without any repositories listed, verify_repositories is a no-op. Please remove this.
• In the repos.each loop of verify_repositories, instead of indexing, you can unpack the list into separate variables: repos.each do |(repo, writable, sharable)|—this should help readability. Please double-check the spelling of "writable."
• Please move verify_repositories closer to the test that calls it, to aid browsing.
• In the controller test, prefer .each over .select for a simple loop, and assert_includes would generate a better diagnostic.
• In the unit test, prefer assert_empty and refute_empty to bare @assert@s, again for better diagnostics.

Thanks.

Thanks Brett. Addressed all of these suggestions and merged into master.