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

add feature tests for creating admin sets and collections #5093

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Sep 8, 2021

Extended testing for admin sets:

  • add feature tests for creating admin sets and collections

Changes in the controller test…

  • specify more precisely failures redirects and flash messages
  • test abilities of admin set collection type managers and creators to manipulate admin sets through the controller.

This extended testing confirms that abilities are sufficient to control access to admin sets.

Unexpected behaviors that are fixed in this PR:

  • Admin sets were not copying over manager permissions.
  • path /admin/admin_sets did not forward to /dashboard/my/collections

The behavioral changes these fixes impart were discussed in the tech call and approved by Hyrax PO.

@samvera/hyrax-code-reviewers

add_breadcrumb t(:'hyrax.admin.sidebar.admin_sets'), hyrax.admin_admin_sets_path
@admin_sets = Hyrax::AdminSetService.new(self).search_results(:edit)
# admin sets are listed with collections
redirect_to hyrax.my_collections_url
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior change to redirect :index to /dashboard/my/collections

@@ -74,7 +74,21 @@ def access_grants_attributes
].tap do |attribute_list|
# Grant manage access to the creating_user if it exists. Should exist for all but default Admin Set
attribute_list << { agent_type: 'user', agent_id: creating_user.user_key, access: Hyrax::PermissionTemplateAccess::MANAGE } if creating_user
end + managers_of_admin_set
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior change to copy the managers from the admin set collection type to the admin set when it is created.

@@ -84,7 +98,8 @@ def admin_group_name
##
# @return [PermissionTemplate]
def create_permission_template
permission_template = PermissionTemplate.create!(source_id: admin_set.id, access_grants_attributes: access_grants_attributes)
permission_template = PermissionTemplate.create!(source_id: admin_set.id,
access_grants_attributes: access_grants_attributes.uniq)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be dups in the grants if one of the managers from the admin set collection type is the creating user. Adding .uniq to only add a user once.

@@ -12,7 +12,7 @@
<% @collection_type_list_presenter.each do |row_presenter| %>
<div class="radio radio-button-list">
<label>
<input type="radio" name="collection_type"
<input type="radio" name="collection_type" value="<%= row_presenter.title.gsub(/\s+/, '') %>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the title as the value is to support feature tests which need a way to select a radio button.

end

class CollectionTypeFactoryHelper
def self.process_access(collection_type, evaluator) # rubocop:disable Metrics/MethodLength
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved access grants to the helper class to allow the code to be shared among factories.

@elrayle elrayle force-pushed the admin_set_perm_tests branch 2 times, most recently from 654e814 to 2effa87 Compare September 8, 2021 21:31
@@ -4,9 +4,7 @@ class Admin::AdminSetsController < ApplicationController
include Hyrax::CollectionsControllerBehavior

before_action :authenticate_user!
before_action :ensure_manager!, except: [:show]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks for manager and viewer are covered by cancan authentication. Feature tests and controller tests behave correctly for each user type. Interactions with the test app in the browser also confirm that the controller limits access as expected.

Extended testing for admin sets:
* add feature tests for creating admin sets and collections

Changes in the controller test…
* specify more precisely failures redirects and flash messages
* test abilities of admin set collection type managers and creators to manipulate admin sets through the controller.

This extended testing confirms that abilities are sufficient to control access to admin sets.

Unexpected behaviors that are fixed in this PR:
* Admin sets were not copying over manager permissions.
* path `/admin/admin_sets` did not forward to `/dashboard/my/collections`

The behavioral changes these fixes impart were discussed in the tech call and approved by Hyrax PO.
Copy link
Contributor

@jlhardes jlhardes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes help adjust some application behaviors so that viewing the list of admin sets and collections in the dashboard can only be done in the UI through the single collections view that includes admin sets with collections. These changes also make sure that any manager role assigned to an admin set collection type is given edit permissions on any admin set created with that admin set collection type. As Hyrax PO, I approve these changes.

@dlpierce dlpierce merged commit 817c11d into main Sep 10, 2021
@dlpierce dlpierce deleted the admin_set_perm_tests branch September 10, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants