[WIP] update AdminSetCreateService to create valkyrie resources #5083

elrayle · 2021-08-23T21:32:25Z

Work In Progress -- DO NOT MERGE

While investigating ensure_manager!, it became clear that the permission_template is not adding the access rights as expected. The code to add the access rights needs to be reviewed and corrected to be sure the rights are assigned.

See PR #5093 -- This PR adds feature tests that demonstrates it is safe to remove ensure_manager! and ensure_viewer!

Add…

.find_or_create_default_admin_set = replacement for AdminSet.find_or_create_default_admin_set_id
.default_admin_set?(id:) = replacement for AdminSet.default_set?
.call! - returns object instead of true/false returned by .call
#create! - returns object instead of true/false returned by #create

Deprecate...

.create_default_admin_set - use .find_or_create_default_admin_set instead

Replace uses of...

AdminSet.find_or_create_default_admin_set_id with Hyrax::AdminSetCreateService.find_or_create_default_admin_set.id
AdminSet.default_set? with Hyrax::AdminSetCreateService.AdminSetCreateService.default_set?

@samvera/hyrax-code-reviewers

app/services/hyrax/admin_set_create_service.rb

lib/tasks/default_admin_set.rake

no-reply · 2021-08-24T16:25:05Z

this looks pretty great.

if you can get it passing i'll take a final pass through and usher it through!

thanks!

no-reply

this is looking pretty good to me!

the main substantive question i have is about the ::Ability handling. i've feared that changes to ::Ability on Collection, AdminSet and FileSet might require some cleverness because ::Ability belongs to the application. your thoughts would be helpful

lib/tasks/default_admin_set.rake

no-reply · 2021-08-26T17:14:28Z

app/controllers/hyrax/admin/admin_sets_controller.rb

    end

    def ensure_manager!
      # TODO: Review for possible removal.  Doesn't appear to apply anymore.
      # Even though the user can view this admin set, they may not be able to view
      # it on the admin page.
-      authorize! :manage_any, AdminSet
+      authorize! :manage_any, Hyrax::AdministrativeSet


can you say more about this?

do these two authorization checks return the same values already(?). will that be the case ::Ability in general, or just the baseline Hyrax::Ability setup? my intuition would be that we'd need to keep the existing check, but i could be missing something.

Commit 'remove check for :ensure_manager! in admin_sets_controller' removed this check altogether along with the associate before_action. I confirmed that tests pass and UI interactions behave as expected. See commit notes for more details.

are we sure it's no longer needed? i'm not really understanding why that would be the case. if we were before rejecting requests from non-managers, i think we probably need to keep it? or if not, at least have a clear understanding of why the new auth pattern is non-breaking.

no-reply · 2021-08-26T17:16:09Z

app/controllers/hyrax/admin/admin_sets_controller.rb

@@ -120,7 +120,9 @@ def ensure_viewer!
    end

    def create_admin_set
-      admin_set_create_service.call(admin_set: @admin_set, creating_user: current_user)
+      # TODO: Should be able to remove the valkryie_resource check when create/edit form is updated


which form does this refer to? i'm not sure what action this todo points to as a next step.

I removed the TODO. I also moved the conversion back and forth to private methods to make the method cleaner. When this is fully working with valkyrie, the conversion back and forth methods can be removed.

app/controllers/hyrax/admin/admin_sets_controller.rb

app/models/admin_set.rb

no-reply · 2021-08-26T17:26:19Z

spec/controllers/hyrax/admin/admin_sets_controller_spec.rb

          post :create, params: { admin_set: { title: 'Test title',
                                               description: 'test description',
                                               workflow_name: 'default' } }
          admin_set = assigns(:admin_set)
-          expect(response).to redirect_to(edit_admin_admin_set_path(admin_set))
+          expect(response).to redirect_to(edit_admin_admin_set_path(admin_set.id.to_s))


is the #to_s needed? do the rails path helpers not handle that?

I was able to remove this change. With the change to have the admin_set id return nil instead of an empty string, this test began failing. The service was not set up to receive :call!, so the test was actually not testing the create process. You will see a change to the service in this test to mock Hyrax::AdminSetCreateService.call!, which allows the create tests to pass.

elrayle · 2021-09-01T21:39:50Z

@no-reply Requested changes are complete and ready for review.

Add… * .`find_or_create_default_admin_set` = replacement for `AdminSet.find_or_create_default_admin_set_id` * `.default_admin_set?(id:)` = replacement for `AdminSet.default_set?` * `.call!` - returns object instead of true/false returned by `.call` * `#create!` - returns object instead of true/false returned by `#create` Deprecate * `.create_default_admin_set` - use `.find_or_create_default_admin_set` instead Publish * update metadata when adminset created rc

…nSetCreateService.find_or_create_default_admin_set.id

…ativeSet This is required for the UI to work with the changes to create admin sets using the Hyrax::AdminSetCreateService which is updated to use Valkryie.

NOTE: Indexing will still run all objects via metadata_index_listener which responds to the same messages. Also, still runs `on_object_deleted` since it operates on `id` only and does not attempt to generate a path.

A TODO in the `AdminSetsController` suggested that `:ensure_manager!` is no longer needed. I removed the `before_action`, ran tests, and tested in the UI with the following steps. All look good. prereq * create admin user * create non-admin user 1. login as admin 2. create new admin set 3. add non-admin user as a Manager 4. logout and log back in as non-admin user 5. navigate to Dashboard -> Collections -> Managed Collection (tab) 6. click name of created admin set 7. click Edit 8. modify the description 9. Save 10. navigate to Dashboard -> Collections -> Managed Collection (tab) 11. click name of created admin set 12. confirm that the description was updated Based on these results, it appears that the TODO is correct and this check can be removed.

makes a few additional minor adjustments based on PR feedback

elrayle · 2021-10-27T19:10:21Z

CLOSING - superseded by PR #5224

elrayle added the wings label Aug 23, 2021

elrayle force-pushed the val/admin_set branch 4 times, most recently from c20db89 to 6223a0b Compare August 24, 2021 14:24

no-reply reviewed Aug 24, 2021

View reviewed changes

app/services/hyrax/admin_set_create_service.rb Outdated Show resolved Hide resolved

no-reply reviewed Aug 24, 2021

View reviewed changes

app/services/hyrax/admin_set_create_service.rb Outdated Show resolved Hide resolved

no-reply reviewed Aug 24, 2021

View reviewed changes

lib/tasks/default_admin_set.rake Show resolved Hide resolved

elrayle force-pushed the val/admin_set branch from 6223a0b to 568a916 Compare August 24, 2021 20:26

elrayle mentioned this pull request Aug 24, 2021

provide NullUser for use when real user is not available #5077

Closed

elrayle force-pushed the val/admin_set branch 2 times, most recently from c7f59cc to ea2030e Compare August 24, 2021 21:20

This was referenced Aug 25, 2021

Fix routes for non-work objects #5085

Open

Allow life cycle events to run for all model types #5086

Open

elrayle force-pushed the val/admin_set branch from 2a3c231 to 2aae297 Compare August 25, 2021 17:52

elrayle mentioned this pull request Aug 25, 2021

add system user #5087

Merged

elrayle force-pushed the val/admin_set branch 3 times, most recently from d0f31ec to a84f223 Compare August 25, 2021 20:31

elrayle marked this pull request as ready for review August 25, 2021 20:31

no-reply reviewed Aug 26, 2021

View reviewed changes

elrayle force-pushed the val/admin_set branch from 099ee13 to be9cc37 Compare September 1, 2021 21:25

elrayle added 6 commits September 2, 2021 16:39

replace AdminSet.find_or_create_default_admin_set_id with Hyrax::Admi…

180d91b

…nSetCreateService.find_or_create_default_admin_set.id

replace AdminSet.default_set? with AdminSetCreateService.default_set?

f716966

update admin set controller & abilities to work with Hyrax::Administr…

9e0a9a0

…ativeSet This is required for the UI to work with the changes to create admin sets using the Hyrax::AdminSetCreateService which is updated to use Valkryie.

use a default system user when creating the default admin set

746677f

only run lifecycle listeners for works and file sets

d5ed164

NOTE: Indexing will still run all objects via metadata_index_listener which responds to the same messages. Also, still runs `on_object_deleted` since it operates on `id` only and does not attempt to generate a path.

elrayle added 2 commits September 2, 2021 16:39

make test stub AdminSetCreateService as the service

109c7e7

makes a few additional minor adjustments based on PR feedback

elrayle changed the title ~~update AdminSetCreateService to create valkyrie resources~~ [WIP] update AdminSetCreateService to create valkyrie resources Sep 2, 2021

elrayle force-pushed the val/admin_set branch from be9cc37 to 109c7e7 Compare September 3, 2021 12:46

elrayle mentioned this pull request Sep 16, 2021

ACLs for Hyrax::AdministrativeSets do not fully persist #5108

Closed

elrayle marked this pull request as draft October 8, 2021 15:51

elrayle closed this Oct 27, 2021

elrayle deleted the val/admin_set branch October 27, 2021 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] update AdminSetCreateService to create valkyrie resources #5083

[WIP] update AdminSetCreateService to create valkyrie resources #5083

elrayle commented Aug 23, 2021 •

edited

Loading

no-reply commented Aug 24, 2021

no-reply left a comment

no-reply Aug 26, 2021

elrayle Sep 1, 2021

no-reply Sep 2, 2021

no-reply Aug 26, 2021

elrayle Sep 1, 2021

no-reply Aug 26, 2021

elrayle Sep 1, 2021

elrayle commented Sep 1, 2021

elrayle commented Oct 27, 2021

[WIP] update AdminSetCreateService to create valkyrie resources #5083

[WIP] update AdminSetCreateService to create valkyrie resources #5083

Conversation

elrayle commented Aug 23, 2021 • edited Loading

no-reply commented Aug 24, 2021

no-reply left a comment

Choose a reason for hiding this comment

no-reply Aug 26, 2021

Choose a reason for hiding this comment

elrayle Sep 1, 2021

Choose a reason for hiding this comment

no-reply Sep 2, 2021

Choose a reason for hiding this comment

no-reply Aug 26, 2021

Choose a reason for hiding this comment

elrayle Sep 1, 2021

Choose a reason for hiding this comment

no-reply Aug 26, 2021

Choose a reason for hiding this comment

elrayle Sep 1, 2021

Choose a reason for hiding this comment

elrayle commented Sep 1, 2021

elrayle commented Oct 27, 2021

elrayle commented Aug 23, 2021 •

edited

Loading