Skip to content

Commit

Permalink
Adjusting hash interaction and boolean test
Browse files Browse the repository at this point in the history
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
  • Loading branch information
jeremyf committed Feb 23, 2021
1 parent 034ac2c commit 88186e3
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions app/services/hyrax/edit_permissions_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ def initialize(object:, ability:)
# @param [Hash] one set of permission fields for object {:name, :access}
# @return [Boolean] true if user cannot edit the given permissions
def cannot_edit_permissions?(permission_hash)
@unauthorized_managers.include?(permission_hash[:name]) && permission_hash[:access] == "edit"
permission_hash.fetch(:access) == "edit" && @unauthorized_managers.include?(permission_hash.fetch(:name))
end

# @param [Hash] one set of permission fields for object {:name, :access}
# @return [Boolean] true if given permissions are one of fixed exclusions
def excluded_permission?(permission_hash)
exclude_from_display.include? permission_hash[:name].downcase
exclude_from_display.include? permission_hash.fetch(:name).downcase
end

private
Expand Down

0 comments on commit 88186e3

Please sign in to comment.