Feature #3686

[Workbench] Provide a sharing tab for repositories similar to setting up sharing for projects.

Added by Peter Amstutz over 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
01/15/2015
Due date:
% Done:

100%

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

Subtasks

Task #4990: Review branch: 3686-sharing-repositoriesResolvedRadhika Chippada

Task #5088: Review branch: 3686-sharing-repositories (one more time, for repository link in manage_account page)ResolvedRadhika Chippada


Related issues

Has duplicate Arvados - Feature #4905: [Workbench] Make repository-sharing feature accessible from Manage Account pageClosed01/06/2015

Associated revisions

Revision 204c4620
Added by Radhika Chippada about 5 years ago

closes #3686
Merge branch '3686-sharing-repositories'

Revision 2717e707
Added by Radhika Chippada about 5 years ago

closes #3686
Merge branch '3686-sharing-repositories'

History

#1 Updated by Peter Amstutz over 5 years ago

  • Category set to Workbench

#2 Updated by Peter Amstutz over 5 years ago

  • Target version set to Arvados Future Sprints

#3 Updated by Ward Vandewege over 5 years ago

  • Story points set to 1.0

#4 Updated by Tom Clegg over 5 years ago

  • Target version changed from Arvados Future Sprints to 2015-01-28 Sprint

#5 Updated by Radhika Chippada over 5 years ago

  • Assigned To set to Radhika Chippada

#6 Updated by Brett Smith about 5 years ago

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.

#7 Updated by Radhika Chippada about 5 years ago

  • Status changed from New to In Progress

#8 Updated by Radhika Chippada about 5 years ago

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

Applied in changeset arvados|commit:204c462041de0b063ed1169c0f600f082400815f.

#9 Updated by Radhika Chippada about 5 years ago

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?

#10 Updated by Radhika Chippada about 5 years ago

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.

#11 Updated by Brett Smith about 5 years ago

  • Status changed from Resolved to In Progress

Reviewing 194eaad

  • For the repository link, I don't see a need to use raw() to escape the repository name. It seems like it could cause trouble if the repository name includes special HTML characters.
  • The repository link should include the #Sharing anchor to go directly to that tab, according to Tom's comments earlier.
  • Please add unit tests for the changes to the Repository model, and controller tests for the changes to the Repositories controller (making sure panes don't include Attributes, and do include Sharing for managers).
  • Comments on the verify_repositories integration test method:
    • Please make this a separate test. Having large tests that exercise many different features dilutes the diagnostic value of the test: if it fails, you have to spend more time tracking down which feature was affected to understand why it failed. The more specific test failures can be, the more they help us with debugging.
    • Please assert directly that repositories you expect to find are listed correctly on the page. If the test asks the API server for a list of repositories, that makes the test less strict, because we're only checking that the API server and Workbench agree about how repositories should be listed. It's preferable to assert that the repositories that we know should be there (because they're listed in the fixtures) are listed correctly.
    • Clicking the repository links adds a lot of overhead to the test for relatively little gain. If you want to make sure the link goes to the right place, it would be quicker to assert that the link's href attribute matches some value. If you want to test the UI of the repository page, again, it's preferable to do that in a separate test.

Thanks.

#12 Updated by Radhika Chippada about 5 years ago

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

#13 Updated by Brett Smith about 5 years ago

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.

#14 Updated by Radhika Chippada about 5 years ago

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

#15 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:2717e707ce048dd9b47754d620663a76256958cf.

Also available in: Atom PDF