Add batch actions to all/managed works page. #2434

jonathandixon · 2017-12-21T19:39:37Z

Adds ability to batch select works on the All/Managed page (dashboard/works) and perform batch operations: delete, add to a collection, and edit.

@samvera/hyrax-code-reviewers

elrayle · 2017-12-22T18:31:52Z

This addresses issue #2280 - Make read access works available to add to collections

jonathandixon · 2018-01-03T15:35:34Z

Added suggested tests: batch edit/delete that includes read-only works updates/deletes the edit access works and not the read access works.

jcoyne

Can you remove DashboardHelperBehavior#on_my_works? now? It doesn't appear to be used anymore.

jcoyne · 2018-01-08T19:39:55Z

spec/views/hyrax/my/_search_header.html.erb_spec.rb

@@ -12,6 +12,7 @@

  context "on my works page" do
    before do
+      view.lookup_context.prefixes.push "hyrax/my/works"
      allow(view).to receive(:on_my_works?).and_return(true)


jcoyne · 2018-01-08T19:40:07Z

spec/views/hyrax/my/_search_header.html.erb_spec.rb

    before do
+      view.lookup_context.prefixes.push "hyrax/my/collections"
      allow(view).to receive(:on_my_works?).and_return(false)


jonathandixon · 2018-01-09T13:46:47Z

@jcoyne I'm fine with removing DashboardHelperBehavior#on_my_works?. Should this be done in a separate issue? I worry that removing it here will get lost and that downstream applications that use it in their templates will have problems on upgrade.

jcoyne · 2018-01-09T21:17:41Z

@jonathandixon I think you should just remove it here because we have no other uses of it in hyrax. If you don't want to remove it here out of an abundance of caution, you should at least deprecate it.

jcoyne · 2018-01-09T21:25:51Z

app/views/hyrax/my/works/_tabs.html.erb

@@ -1,7 +1,7 @@
 <ul class="nav nav-tabs" id="my_nav" role="navigation">
  <span class="sr-only">You are currently listing your works.  You have <%= @response.docs.count %> <%= 'work'.pluralize(@response.docs.count)%> </span>
  <li<%= ' class="active"'.html_safe if current_page?(hyrax.dashboard_works_path(locale: nil)) %>>
-    <%= link_to t("hyrax.dashboard.#{current_ability.admin? ? 'all' : 'managed'}.works"), hyrax.dashboard_works_path %>
+    <%= link_to t("hyrax.dashboard.#{current_ability.admin? ? 'all' : 'managed'}.works"), hyrax.dashboard_works_path, data: { turbolinks: false } %>


Why are you doing turbolinks: false here?

This relates to issue #1191. Without it, you end up with an ActionController::InvalidAuthenticityToken error.

jcoyne · 2018-01-09T21:27:56Z

app/views/hyrax/my/works/_batch_actions.html.erb

+  <div class="batch-toggle">
+    <% session[:batch_edit_state] = "on" %>
+    <div class="button_to-inline">
+      <%= button_to "Edit Selected", hyrax.edit_batch_edits_path, method: :get, class: "btn btn-update btn-primary hidden submits-batches", data: { behavior: 'batch-edit' }, id: 'batch-edit' %>


Can you add this string to i18n? and on line 10?

jcoyne · 2018-01-09T21:29:53Z

spec/controllers/hyrax/batch_edits_controller_spec.rb

    end

    it "is successful" do
      put :update, params: { update_type: "delete_all" }
      expect(response).to redirect_to(dashboard_path(locale: 'en'))
      expect { GenericWork.find(one.id) }.to raise_error(Ldp::Gone)
      expect { GenericWork.find(two.id) }.to raise_error(Ldp::Gone)
+      expect(GenericWork.find(three.id).id).to eq(three.id)


This should be expect(GenericWork).to exist(three.id) (or alternately expect(GenericWork.exists?(three.id).to be true)

jcoyne · 2018-01-09T21:31:01Z

spec/controllers/hyrax/batch_edits_controller_spec.rb

@@ -61,13 +70,15 @@
      expect(response).to be_redirect
      expect(GenericWork.find(one.id).subject).to eq ["zzz"]
      expect(GenericWork.find(two.id).subject).to eq ["zzz"]
+      expect(GenericWork.find(three.id).subject).not_to eq ["zzz"]


Negative assertions are not nearly as specific as positive assertions. Is there a reason you can't do expect(GenericWork.find(three.id).subject).to eq ["whavever value it initially had"]?

jcoyne · 2018-01-09T21:31:23Z

spec/controllers/hyrax/batch_edits_controller_spec.rb

    end

    it "updates permissions" do
      put :update, params: { update_type: "update", visibility: "authenticated" }
      expect(response).to be_redirect
      expect(GenericWork.find(one.id).visibility).to eq "authenticated"
      expect(GenericWork.find(two.id).visibility).to eq "authenticated"
+      expect(GenericWork.find(three.id).visibility).not_to eq "authenticated"


Can we assert what we expect it to be? restricted?

jcoyne · 2018-01-09T21:31:56Z

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -74,7 +74,7 @@

      it "adds docs to the collection and adds the collection id to the documents in the collection" do
        post :create, params: {
-          batch_document_ids: [asset1.id],
+          batch_document_ids: [asset1.id, unowned_asset.id],


Why are you doing this change? It seems odd that no assertions are changing.

This is to verify that only works the user has access to are added to the collection. This passes an ID of an unowned work with the request but that shouldn't change the original expectation that only asset1 is added to the collection.

jcoyne · 2018-01-09T21:33:19Z

spec/controllers/hyrax/batch_edits_controller_spec.rb

@@ -35,20 +37,27 @@
    let!(:two) do
      create(:generic_work, creator: ["Fred"], title: ["abc"], language: ['en'], user: user)
    end
+
+    let!(:three) do
+      create(:generic_work, creator: ["Fred"], title: ["abc"], language: ['en'])


On line 13 above you are using create(:work,...) can we stick with that for consistency?

jcoyne · 2018-01-09T21:34:35Z

app/views/hyrax/dashboard/works/_list_works.html.erb

@@ -1,4 +1,8 @@
 <tr id="document_<%= document.id %>">
+  <td>
+    <label for="batch_document_<%= document.id %>" class="sr-only"><%=t("hyrax.dashboard.my.sr.batch_checkbox")%></label>
+    <%= render 'hyrax/batch_select/add_button', document: document %>&nbsp;


What's the point of the &nbsp here?

This was a copy and paste. It appears the   is used to manipulate the vertical position of the checkbox inside the table cell (without it the checkbox renders below verticle center).

elrayle

This looks good to me. @jcoyne Are you questions answered? This looks ready to merge to me.

Basic changes look good, but there are test failures.

aeschylus added the in progress label Dec 21, 2017

jcoyne added the Collection impacts the Collection part of PCDM Model label Jan 8, 2018

jcoyne requested changes Jan 8, 2018

View reviewed changes

jcoyne previously requested changes Jan 9, 2018

View reviewed changes

elrayle previously approved these changes Jan 11, 2018

View reviewed changes

elrayle previously approved these changes Jan 12, 2018

View reviewed changes

Add batch actions to all/managed works page.

c39dd24

jonathandixon dismissed elrayle’s stale review via c39dd24 January 12, 2018 18:47

elrayle approved these changes Jan 13, 2018

View reviewed changes

elrayle merged commit e5618c0 into samvera:collections-sprint Jan 13, 2018

elrayle removed the in progress label Jan 13, 2018

elrayle mentioned this pull request Jan 30, 2018

make read access works available to add to collections #2280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add batch actions to all/managed works page. #2434

Add batch actions to all/managed works page. #2434

jonathandixon commented Dec 21, 2017

elrayle commented Dec 22, 2017

jonathandixon commented Jan 3, 2018

jcoyne left a comment

jcoyne Jan 8, 2018

jcoyne Jan 8, 2018

jonathandixon commented Jan 9, 2018

jcoyne commented Jan 9, 2018

jcoyne Jan 9, 2018

jonathandixon Jan 10, 2018

jcoyne Jan 9, 2018

jcoyne Jan 9, 2018

jcoyne Jan 9, 2018

jcoyne Jan 9, 2018

jcoyne Jan 9, 2018

jonathandixon Jan 10, 2018

jcoyne Jan 9, 2018

jcoyne Jan 9, 2018

jonathandixon Jan 10, 2018

elrayle left a comment

Add batch actions to all/managed works page. #2434

Add batch actions to all/managed works page. #2434

Conversation

jonathandixon commented Dec 21, 2017

elrayle commented Dec 22, 2017

jonathandixon commented Jan 3, 2018

jcoyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathandixon commented Jan 9, 2018

jcoyne commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment