From 54348f0c7be208889c0d99ca7df9e04febbf26e7 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Sat, 21 Apr 2018 20:41:18 -0400 Subject: [PATCH] Pass along the object's admin_set_id to the param sanitization to ensure that permissions params are returned or stripped based upon the active workflow for the admin set. Without this the batch edit of permissions silently fails because it doesn't see any params to operate on. This commit also fixes the params for the visibility tests based upon browser testing. The line reading the visibility from the bare param could be removed since it isn't used so I marked it as deprecated. Fix visibility and use actor to correctly handle params Make permissions collapsable like description tab --- .rubocop_fixme.yml | 2 + app/assets/javascripts/hyrax/batch_edit.js | 3 +- .../hyrax/batch_edits_controller.rb | 34 +++++++- app/views/hyrax/batch_edits/edit.html.erb | 74 +++++++++++++---- .../hyrax/batch_edits_controller_spec.rb | 83 ++++++++++++++++++- spec/features/batch_edit_spec.rb | 51 +++++++++++- 6 files changed, 219 insertions(+), 28 deletions(-) diff --git a/.rubocop_fixme.yml b/.rubocop_fixme.yml index d8b93c6638..fbf0f039ca 100644 --- a/.rubocop_fixme.yml +++ b/.rubocop_fixme.yml @@ -6,6 +6,7 @@ Metrics/ClassLength: Exclude: - 'app/controllers/hyrax/dashboard/collections_controller.rb' - 'app/controllers/hyrax/admin/admin_sets_controller.rb' + - 'app/controllers/hyrax/batch_edits_controller.rb' - 'app/controllers/hyrax/downloads_controller.rb' - 'app/controllers/hyrax/file_sets_controller.rb' - 'app/forms/hyrax/forms/permission_template_form.rb' @@ -83,6 +84,7 @@ RSpec/ExampleLength: - 'spec/actors/hyrax/actors/file_set_actor_spec.rb' - 'spec/actors/hyrax/actors/generic_work_actor_spec.rb' - 'spec/controllers/hyrax/api/items_controller_spec.rb' + - 'spec/controllers/hyrax/batch_edits_controller_spec.rb' - 'spec/controllers/hyrax/batch_uploads_controller_spec.rb' - 'spec/controllers/hyrax/file_sets_controller_spec.rb' - 'spec/controllers/hyrax/generic_works_controller_spec.rb' diff --git a/app/assets/javascripts/hyrax/batch_edit.js b/app/assets/javascripts/hyrax/batch_edit.js index ccd9347eb9..f076af6980 100644 --- a/app/assets/javascripts/hyrax/batch_edit.js +++ b/app/assets/javascripts/hyrax/batch_edit.js @@ -159,7 +159,8 @@ function batch_edit_init () { setTimeout(ajaxManager.runNow(), 100); } - $("#permissions_save").click(runSave); + $("#permissions_visibility_save").click(runSave); + $("#permissions_roles_save").click(runSave); $(".field-save").click(runSave); } diff --git a/app/controllers/hyrax/batch_edits_controller.rb b/app/controllers/hyrax/batch_edits_controller.rb index a4fccd46b6..b9fb3349c0 100644 --- a/app/controllers/hyrax/batch_edits_controller.rb +++ b/app/controllers/hyrax/batch_edits_controller.rb @@ -46,9 +46,16 @@ def destroy_collection end def update_document(obj) - obj.attributes = work_params + interpret_visiblity_params(obj) + obj.attributes = work_params(admin_set_id: obj.admin_set_id).except(*visibility_params) obj.date_modified = Time.current.ctime - obj.visibility = params[:visibility] + + if params[:visibility] + Deprecation.warn(self, "visibility is not submitted by the Hyrax UI and is deprecated " \ + "for removal in Hyrax 3.0. Please use " \ + "#{form_class.model_name.param_key}[visibility] instead.") + obj.visibility = params[:visibility] + end InheritPermissionsJob.perform_now(obj) VisibilityCopyJob.perform_now(obj) @@ -94,9 +101,28 @@ def terms form_class.terms end - def work_params + def work_params(extra_params = {}) work_params = params[form_class.model_name.param_key] || ActionController::Parameters.new - form_class.model_attributes(work_params) + form_class.model_attributes(work_params.merge(extra_params)) + end + + def interpret_visiblity_params(obj) + stack = ActionDispatch::MiddlewareStack.new.tap do |middleware| + middleware.use Hyrax::Actors::InterpretVisibilityActor + end + env = Hyrax::Actors::Environment.new(obj, current_ability, work_params(admin_set_id: obj.admin_set_id)) + last_actor = Hyrax::Actors::Terminator.new + stack.build(last_actor).update(env) + end + + def visibility_params + ['visibility', + 'lease_expiration_date', + 'visibility_during_lease', + 'visibility_after_lease', + 'embargo_release_date', + 'visibility_during_embargo', + 'visibility_after_embargo'] end def redirect_to_return_controller diff --git a/app/views/hyrax/batch_edits/edit.html.erb b/app/views/hyrax/batch_edits/edit.html.erb index 28a7d06665..34cefe2292 100644 --- a/app/views/hyrax/batch_edits/edit.html.erb +++ b/app/views/hyrax/batch_edits/edit.html.erb @@ -50,25 +50,65 @@
- <%= simple_form_for @form.model, - url: hyrax.batch_edits_path, - method: :put, - remote: true, - html: { id: "form_permissions", class: "ajax-form"}, - data: { 'param-key' => @form.model_name.param_key } do |f| %> - <%= hidden_field_tag('update_type', 'update') %> - <%= hidden_field_tag('key', 'permissions') %> - <%= render "hyrax/base/form_permission", f: f %> - <%= render "hyrax/base/form_share", f: f %> +
+ <%= simple_form_for @form, + url: hyrax.batch_edits_path, + method: :put, + remote: true, + builder: Hyrax::FormBuilder, + html: { id: "form_permissions_visibility", class: "ajax-form"}, + data: { 'param-key' => @form.model_name.param_key } do |f| %> +
+ +
+
+ <%= hidden_field_tag('update_type', 'update') %> + <%= hidden_field_tag('key', 'permissions') %> + <%= render "hyrax/base/form_permission", f: f %> - <% @form.batch_document_ids.each do |batch_id| %> - <%= hidden_field_tag "batch_document_ids[]", batch_id %> + <% @form.batch_document_ids.each do |batch_id| %> + <%= hidden_field_tag "batch_document_ids[]", batch_id %> + <% end %> +
+ <%= f.submit "Save changes", class: 'btn btn-primary field-save', id: "permissions_visibility_save" %> + Cancel +
+
+
<% end %> -
- <%= f.submit "Save changes", class: 'btn btn-primary', id: 'permissions_save' %> -
-
- <% end %> +
+ +
+ <%= simple_form_for @form, + url: hyrax.batch_edits_path, + method: :put, + remote: true, + builder: Hyrax::FormBuilder, + html: { id: "form_permissions", class: "ajax-form"}, + data: { 'param-key' => @form.model_name.param_key } do |f| %> +
+ +
+
+ <%= hidden_field_tag('update_type', 'update') %> + <%= hidden_field_tag('key', 'permissions') %> + <%= render "hyrax/base/form_share", f: f %> + + <% @form.batch_document_ids.each do |batch_id| %> + <%= hidden_field_tag "batch_document_ids[]", batch_id %> + <% end %> +
+ <%= f.submit "Save changes", class: 'btn btn-primary field-save', id: "permissions_sharing_save" %> + Cancel +
+
+
+ <% end %> +
diff --git a/spec/controllers/hyrax/batch_edits_controller_spec.rb b/spec/controllers/hyrax/batch_edits_controller_spec.rb index 15ea9a0d49..846b8e3154 100644 --- a/spec/controllers/hyrax/batch_edits_controller_spec.rb +++ b/spec/controllers/hyrax/batch_edits_controller_spec.rb @@ -32,22 +32,27 @@ describe "update" do let(:user) { build(:user) } + let(:admin_set) { create(:admin_set) } + let(:permission_template) { create(:permission_template, source_id: admin_set.id) } + let!(:workflow) { create(:workflow, allows_access_grant: true, active: true, permission_template_id: permission_template.id) } let!(:one) do - create(:work, creator: ["Fred"], title: ["abc"], language: ['en'], user: user) + create(:work, admin_set_id: admin_set.id, creator: ["Fred"], title: ["abc"], language: ['en'], user: user) end let!(:two) do - create(:work, creator: ["Fred"], title: ["abc"], language: ['en'], user: user) + create(:work, admin_set_id: admin_set.id, creator: ["Fred"], title: ["abc"], language: ['en'], user: user) end let!(:three) do - create(:work, creator: ["Fred"], title: ["abc"], language: ['en']) + create(:work, admin_set_id: admin_set.id, creator: ["Fred"], title: ["abc"], language: ['en']) end let!(:file_set) do create(:file_set, creator: ["Fred"]) end + let(:release_date) { Time.zone.today + 2 } + let(:mycontroller) { "hyrax/my/works" } before do @@ -83,7 +88,7 @@ end it "updates permissions" do - put :update, params: { update_type: "update", visibility: "authenticated" } + put :update, params: { update_type: "update", generic_work: { visibility: "authenticated" } } expect(response).to be_redirect work1 = GenericWork.find(one.id) @@ -93,6 +98,76 @@ expect(GenericWork.find(two.id).visibility).to eq "authenticated" expect(GenericWork.find(three.id).visibility).to eq "restricted" end + + it 'creates leases' do + put :update, params: { update_type: "update", + generic_work: { visibility: "lease", lease_expiration_date: release_date, visibility_during_lease: 'open', visibility_after_lease: 'restricted' } } + expect(response).to be_redirect + + work1 = GenericWork.find(one.id) + expect(work1.visibility).to eq "open" + expect(work1.visibility_during_lease).to eq 'open' + expect(work1.visibility_after_lease).to eq 'restricted' + expect(work1.lease_expiration_date).to eq release_date + expect(work1.file_sets.map(&:visibility)).to eq ['open'] + expect(work1.file_sets.map(&:visibility_during_lease)).to eq ['open'] + expect(work1.file_sets.map(&:visibility_after_lease)).to eq ['restricted'] + expect(work1.file_sets.map(&:lease_expiration_date)).to eq [release_date] + + work2 = GenericWork.find(two.id) + expect(work2.visibility).to eq 'open' + expect(work2.visibility_during_lease).to eq 'open' + expect(work2.visibility_after_lease).to eq 'restricted' + expect(work2.lease_expiration_date).to eq release_date + + work3 = GenericWork.find(three.id) + expect(work3.visibility).to eq 'restricted' + expect(work3.visibility_during_lease).to be_nil + expect(work3.visibility_after_lease).to be_nil + expect(work3.lease_expiration_date).to be_nil + end + + it 'creates embargoes' do + put :update, params: { update_type: "update", + generic_work: { visibility: "embargo", embargo_release_date: release_date, visibility_during_embargo: 'authenticated', visibility_after_embargo: 'open' } } + expect(response).to be_redirect + + work1 = GenericWork.find(one.id) + expect(work1.visibility).to eq "authenticated" + expect(work1.visibility_during_embargo).to eq 'authenticated' + expect(work1.visibility_after_embargo).to eq 'open' + expect(work1.embargo_release_date).to eq release_date + expect(work1.file_sets.map(&:visibility)).to eq ['authenticated'] + expect(work1.file_sets.map(&:visibility_during_embargo)).to eq ['authenticated'] + expect(work1.file_sets.map(&:visibility_after_embargo)).to eq ['open'] + expect(work1.file_sets.map(&:embargo_release_date)).to eq [release_date] + + work2 = GenericWork.find(two.id) + expect(work2.visibility).to eq 'authenticated' + expect(work2.visibility_during_embargo).to eq 'authenticated' + expect(work2.visibility_after_embargo).to eq 'open' + expect(work2.embargo_release_date).to eq release_date + + work3 = GenericWork.find(three.id) + expect(work3.visibility).to eq 'restricted' + expect(work3.visibility_during_embargo).to be_nil + expect(work3.visibility_after_embargo).to be_nil + expect(work3.embargo_release_date).to be_nil + end + + context 'with roles' do + it 'updates roles' do + put :update, params: { update_type: "update", generic_work: { permissions_attributes: [{ type: 'person', access: 'read', name: 'foo@bar.com' }] } } + expect(response).to be_redirect + + work1 = GenericWork.find(one.id) + expect(work1.read_users).to include "foo@bar.com" + expect(work1.file_sets.map(&:read_users)).to eq [["foo@bar.com"]] + + expect(GenericWork.find(two.id).read_users).to eq ["foo@bar.com"] + expect(GenericWork.find(three.id).read_users).to eq [] + end + end end describe "#destroy_collection" do diff --git a/spec/features/batch_edit_spec.rb b/spec/features/batch_edit_spec.rb index b50ec72e40..5d0ce8df9b 100644 --- a/spec/features/batch_edit_spec.rb +++ b/spec/features/batch_edit_spec.rb @@ -2,10 +2,16 @@ RSpec.describe 'batch', type: :feature, clean_repo: true, js: true do let(:current_user) { create(:user) } - let!(:work1) { create(:public_work, user: current_user) } - let!(:work2) { create(:public_work, user: current_user) } + let(:admin_set) { create(:admin_set) } + let(:permission_template) { create(:permission_template, source_id: admin_set.id) } + let!(:workflow) { create(:workflow, allows_access_grant: true, active: true, permission_template_id: permission_template.id) } + + let!(:work1) { create(:public_work, admin_set_id: admin_set.id, user: current_user, members: [file_set]) } + let!(:work2) { create(:public_work, admin_set_id: admin_set.id, user: current_user) } + let!(:file_set) { create(:file_set) } before do + RoleMapper.byname[current_user.user_key] << 'donor' sign_in current_user visit '/dashboard/my/works' check 'check_all' @@ -60,6 +66,47 @@ page.find("input#generic_work_related_url[value='NEW related_url']") end end + + it 'updates permissions and roles' do + click_on 'batch-edit' + find('#edit_permissions_link').click + expect(page).to have_content('Batch Edit Descriptions') + + # Set visibility to private + within "#form_permissions_visibility" do + batch_edit_expand('permissions_visibility') + find('#generic_work_visibility_authenticated').click + find('#permissions_visibility_save').click + # This was `expect(page).to have_content 'Changes Saved'`, however in debugging, + # the `have_content` check was ignoring the `within` scoping and finding + # "Changes Saved" for other field areas + find('.status', text: 'Changes Saved') + end + + within "#form_permissions" do + batch_edit_expand('permissions_sharing') + page.select('donor', from: 'new_group_name_skel') + page.select('View/Download', from: 'new_group_permission_skel') + page.find('#add_new_group_skel').click + find('#collapse_permissions_sharing table', text: 'View/Download') + find('#collapse_permissions_sharing table', text: 'donor') + find('#permissions_sharing_save').click + # This was `expect(page).to have_content 'Changes Saved'`, however in debugging, + # the `have_content` check was ignoring the `within` scoping and finding + # "Changes Saved" for other field areas + find('.status', text: 'Changes Saved') + end + + # Visit work permissions and verify + visit "/concern/generic_works/#{work1.id}/edit#share" + page.find('#generic_work_visibility_authenticated:checked') + page.find('#share table', text: 'donor') + + # Visit file permissions and verify + visit "concern/file_sets/#{work1.file_sets.first.id}/edit#permissions_display" + page.find('#file_set_visibility_authenticated:checked') + page.find('#permissions_display table', text: 'donor') + end end describe 'deleting' do