Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow readonly works to be added to collections #3338

Merged
merged 1 commit into from Oct 24, 2018
Merged

Conversation

@elrayle
Copy link
Contributor

elrayle commented Oct 18, 2018

Fixes #3323

Allow logged in users to add read-only works to collections. This supports the exhibit building use case as described in issue #3323.

Backward Compatibility Analysis

Public API

  • 0 public methods changed
  • 1 public method added to work_show_presenter

UI Behavior

impacts work show page only

  • behavior for editors of the work is unchanged
  • behavior for viewers of the work - now will show the Add to Collection button if the user can deposit to at least one collection or can create a collection

Guidance for testing, such as acceptance criteria or new user interface behaviors:

  • As editor of the work, see Add to Collection button.
  • As a logged in viewer of the work, see Add to Collection button IFF the viewer can deposit in at least one collection OR can create a new collection
  • As a non-logged in user viewing the work, do NOT see the Add to Collection button

@samvera/hyrax-code-reviewers

@elrayle elrayle added the in progress label Oct 18, 2018
@@ -133,7 +133,7 @@ def manifest
private

def user_collections
collections_service.search_results(:deposit) if presenter.editor?
collections_service.search_results(:deposit)

This comment has been minimized.

Copy link
@elrayle

elrayle Oct 18, 2018

Author Contributor

The restriction to if presenter.editor? prevents viewers of the work to have access to their collections.

@@ -1,4 +1,4 @@
RSpec.describe "display a work as its owner" do
RSpec.describe "work show view" do
include Selectors::Dashboard

This comment has been minimized.

Copy link
@elrayle

elrayle Oct 18, 2018

Author Contributor

Made a few modifications to test labels in this file because the output of the combined labels did not make sense.

@elrayle elrayle self-assigned this Oct 18, 2018
@@ -204,6 +204,13 @@ def manifest_metadata
metadata
end

# determine if the user can add this work to a collection
# @return true if the user can deposit to at least one collection OR if the user can create a collection; otherwise, false
def can_add_to_collection?(depositable_collections)

This comment has been minimized.

Copy link
@no-reply

no-reply Oct 19, 2018

Member

I'm slightly uncomfortable with the Presenter leaking this permission information into the view, and the view conditional on https://github.com/samvera/hyrax/pull/3338/files#diff-600b68f4bb5cd3833e5a8be24f8b356eR25 that results.

Could we explore refactors that avoid this--hopefully by making the new presenter method return some presentable data?

One (maybe too awkward?) possibility that comes to mind is to change the new method to change the new method to:

  def depositable_work_ids(collections)
    return [] unless collections.present? || current_ability.can?(:create_any, Collection)
    [id]
  end

Another option might be to just rename the existing method to something presentation oriented:

  def show_deposit_for?(collections:)
    collections.present? || current_ability.can?(:create_any, Collection)
  end

The second option doesn't squash the need for the view conditional, but does at least reframe so the presenter is focused on responding to messages in its own domain.

Thoughts?

@elrayle elrayle force-pushed the fix/3323_add_to_col branch from 8b8a769 to 9e06f09 Oct 19, 2018
Fixes #3323

* 0 public methods changed
* 1 public method added to work_show_presenter

impacts work show page only
* behavior for editors of the work is unchanged
* behavior for viewers of the work - now will show the Add to Collection button if the user can deposit to at least one collection or can create a collection
@elrayle elrayle force-pushed the fix/3323_add_to_col branch from 9e06f09 to 13bfc07 Oct 19, 2018
@elrayle

This comment has been minimized.

Copy link
Contributor Author

elrayle commented Oct 21, 2018

@no-reply Thanks for the quick review. Requested changes have been applied. All tests passing.

@elrayle

This comment has been minimized.

Copy link
Contributor Author

elrayle commented Oct 23, 2018

@vantuyls @julesies Do either of you want to comment on this PR to indicate acceptance of the proposed UI changes? I'm hoping for this to get the final code review tomorrow after Tech call.

@vantuyls

This comment has been minimized.

Copy link

vantuyls commented Oct 23, 2018

this looks like the changes we'd be hoping for. thanks.

Copy link
Member

no-reply left a comment

Thanks for following through on this. 👏

@no-reply no-reply merged commit 7023671 into master Oct 24, 2018
5 checks passed
5 checks passed
Hound No violations found. Woof!
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0002%) to 97.5%
Details
@no-reply no-reply deleted the fix/3323_add_to_col branch Oct 24, 2018
@no-reply no-reply removed the in progress label Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.