From 005fb333a0646ba985cac575bb543c567725148d Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 13 Sep 2024 12:38:48 +0100 Subject: [PATCH 1/5] return to URL from session after creation --- app/controllers/collections_controller.rb | 7 ++++++- app/controllers/creators_controller.rb | 7 ++++++- spec/requests/collections_spec.rb | 13 ++++++++----- spec/requests/creators_spec.rb | 9 ++++++++- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/app/controllers/collections_controller.rb b/app/controllers/collections_controller.rb index efd3fd886..c5ba4cc26 100644 --- a/app/controllers/collections_controller.rb +++ b/app/controllers/collections_controller.rb @@ -54,7 +54,12 @@ def edit def create authorize Collection @collection = Collection.create(collection_params) - redirect_to collections_path, notice: t(".success") + if session[:return_after_new_collection] + redirect_to session[:return_after_new_collection] + "?new_collection=#{@collection.to_param}", notice: t(".success") + session[:return_after_new_collection] = nil + else + redirect_to collections_path, notice: t(".success") + end end def update diff --git a/app/controllers/creators_controller.rb b/app/controllers/creators_controller.rb index 557b2b8bf..faea03318 100644 --- a/app/controllers/creators_controller.rb +++ b/app/controllers/creators_controller.rb @@ -52,7 +52,12 @@ def edit def create authorize Creator @creator = Creator.create(creator_params) - redirect_to creators_path, notice: t(".success") + if session[:return_after_new_creator] + redirect_to session[:return_after_new_creator] + "?new_creator=#{@creator.to_param}", notice: t(".success") + session[:return_after_new_creator] = nil + else + redirect_to creators_path, notice: t(".success") + end end def update diff --git a/spec/requests/collections_spec.rb b/spec/requests/collections_spec.rb index 5241f836f..6f3f28b5a 100644 --- a/spec/requests/collections_spec.rb +++ b/spec/requests/collections_spec.rb @@ -34,11 +34,18 @@ end describe "POST /collections" do - it "creates a new collection", :as_contributor do + it "creates a new collection and redirects to list", :as_contributor do post "/collections", params: {collection: {name: "newname"}} expect(response).to redirect_to("/collections") end + it "creates a new collection and redirects to return location if set", :as_contributor do + model = Model.first + allow_any_instance_of(CollectionsController).to receive(:session).and_return({return_after_new_collection: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance + post "/collections", params: {collection: {name: "newname"}} + expect(response).to redirect_to("/models/#{model.to_param}/edit?new_collection=#{Collection.last.to_param}") + end + it "denies member permission", :as_member do expect { post "/collections", params: {collection: {name: "newname"}} }.to raise_error(Pundit::NotAuthorizedError) end @@ -78,10 +85,6 @@ patch "/collections/#{collection.to_param}", params: {collection: {name: "newname"}} expect(response).to redirect_to("/collections") end - - it "is denied to non-moderators", :as_contributor do - expect { patch "/collections/#{collection.to_param}", params: {collection: {name: "newname"}} }.to raise_error(Pundit::NotAuthorizedError) - end end describe "DELETE /collections/:id" do diff --git a/spec/requests/creators_spec.rb b/spec/requests/creators_spec.rb index 1650723ba..ec6d6ca16 100644 --- a/spec/requests/creators_spec.rb +++ b/spec/requests/creators_spec.rb @@ -34,11 +34,18 @@ end describe "POST /creators" do - it "creates a new creator", :as_contributor do + it "creates a new creator and redirects to list", :as_contributor do post "/creators", params: {creator: {name: "newname"}} expect(response).to redirect_to("/creators") end + it "creates a new creator and redirects to return location if set", :as_contributor do + model = Model.first + allow_any_instance_of(CreatorsController).to receive(:session).and_return({return_after_new_creator: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance + post "/creators", params: {creator: {name: "newname"}} + expect(response).to redirect_to("/models/#{model.to_param}/edit?new_creator=#{Creator.last.to_param}") + end + it "denies member permission", :as_member do expect { post "/creators", params: {creator: {name: "newname"}} }.to raise_error(Pundit::NotAuthorizedError) end From 7e4287a662a546310c5a6812afb4ca81348d1483 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 13 Sep 2024 12:58:29 +0100 Subject: [PATCH 2/5] rename return_to option --- app/controllers/collections_controller.rb | 6 +++--- app/controllers/creators_controller.rb | 6 +++--- spec/requests/collections_spec.rb | 2 +- spec/requests/creators_spec.rb | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/collections_controller.rb b/app/controllers/collections_controller.rb index c5ba4cc26..b1a636c19 100644 --- a/app/controllers/collections_controller.rb +++ b/app/controllers/collections_controller.rb @@ -54,9 +54,9 @@ def edit def create authorize Collection @collection = Collection.create(collection_params) - if session[:return_after_new_collection] - redirect_to session[:return_after_new_collection] + "?new_collection=#{@collection.to_param}", notice: t(".success") - session[:return_after_new_collection] = nil + if session[:return_after_new] + redirect_to session[:return_after_new] + "?new_collection=#{@collection.to_param}", notice: t(".success") + session[:return_after_new] = nil else redirect_to collections_path, notice: t(".success") end diff --git a/app/controllers/creators_controller.rb b/app/controllers/creators_controller.rb index faea03318..be4282ec6 100644 --- a/app/controllers/creators_controller.rb +++ b/app/controllers/creators_controller.rb @@ -52,9 +52,9 @@ def edit def create authorize Creator @creator = Creator.create(creator_params) - if session[:return_after_new_creator] - redirect_to session[:return_after_new_creator] + "?new_creator=#{@creator.to_param}", notice: t(".success") - session[:return_after_new_creator] = nil + if session[:return_after_new] + redirect_to session[:return_after_new] + "?new_creator=#{@creator.to_param}", notice: t(".success") + session[:return_after_new] = nil else redirect_to creators_path, notice: t(".success") end diff --git a/spec/requests/collections_spec.rb b/spec/requests/collections_spec.rb index 6f3f28b5a..83a5332c2 100644 --- a/spec/requests/collections_spec.rb +++ b/spec/requests/collections_spec.rb @@ -41,7 +41,7 @@ it "creates a new collection and redirects to return location if set", :as_contributor do model = Model.first - allow_any_instance_of(CollectionsController).to receive(:session).and_return({return_after_new_collection: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionsController).to receive(:session).and_return({return_after_new: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance post "/collections", params: {collection: {name: "newname"}} expect(response).to redirect_to("/models/#{model.to_param}/edit?new_collection=#{Collection.last.to_param}") end diff --git a/spec/requests/creators_spec.rb b/spec/requests/creators_spec.rb index ec6d6ca16..ad5eb9252 100644 --- a/spec/requests/creators_spec.rb +++ b/spec/requests/creators_spec.rb @@ -41,7 +41,7 @@ it "creates a new creator and redirects to return location if set", :as_contributor do model = Model.first - allow_any_instance_of(CreatorsController).to receive(:session).and_return({return_after_new_creator: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CreatorsController).to receive(:session).and_return({return_after_new: edit_model_path(model)}) # rubocop:disable RSpec/AnyInstance post "/creators", params: {creator: {name: "newname"}} expect(response).to redirect_to("/models/#{model.to_param}/edit?new_creator=#{Creator.last.to_param}") end From 6253f5279cd850cc8eef1e1c5e7d39c0e37fcc68 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 13 Sep 2024 12:58:52 +0100 Subject: [PATCH 3/5] set return session param on model editing pages --- app/controllers/models_controller.rb | 10 ++++++++ spec/requests/models_spec.rb | 36 +++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/app/controllers/models_controller.rb b/app/controllers/models_controller.rb index b6f5477b0..37bb7d0d9 100644 --- a/app/controllers/models_controller.rb +++ b/app/controllers/models_controller.rb @@ -7,6 +7,8 @@ class ModelsController < ApplicationController before_action :get_model, except: [:bulk_edit, :bulk_update, :index, :new, :create] before_action :get_creators_and_collections, only: [:new, :edit, :bulk_edit] + before_action :set_returnable, only: [:bulk_edit, :edit, :new] + before_action :clear_returnable, only: [:bulk_update, :update, :create] after_action :verify_policy_scoped, only: [:bulk_edit, :bulk_update] @@ -204,4 +206,12 @@ def get_creators_and_collections @creators = policy_scope(Creator) @collections = policy_scope(Collection) end + + def set_returnable + session[:return_after_new] = request.fullpath + end + + def clear_returnable + session[:return_after_new] = nil + end end diff --git a/spec/requests/models_spec.rb b/spec/requests/models_spec.rb index 716f62670..6f96425b5 100644 --- a/spec/requests/models_spec.rb +++ b/spec/requests/models_spec.rb @@ -43,6 +43,11 @@ expect(response).to have_http_status(:success) end + it "sets returnable session param", :as_moderator do + get "/models/#{library.models.first.to_param}/edit" + expect(session[:return_after_new]).to eq "/models/#{library.models.first.to_param}/edit" + end + it "is denied to non-moderators", :as_contributor do expect { get "/models/#{library.models.first.to_param}/edit" }.to raise_error(Pundit::NotAuthorizedError) end @@ -59,6 +64,11 @@ expect(tags[2]).to eq "c" end + it "clears returnable session param", :as_moderator do + put "/models/#{library.models.first.to_param}", params: {model: {tag_list: ["a", "b", "c"]}} + expect(session[:return_after_new]).to be_nil + end + it "removes tags from a model", :as_moderator do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations first = library.models.first first.tag_list = "a, b, c" @@ -110,12 +120,17 @@ expect(response).to have_http_status(:success) end + it "sets returnable session param", :as_moderator do + get "/models/edit" + expect(session[:return_after_new]).to eq "/models/edit" + end + it "is denied to non-moderators", :as_contributor do expect { get "/models/edit" }.to raise_error(Pundit::NotAuthorizedError) end end - describe "PATCH /models/edit" do + describe "PATCH /models/update" do it "updates models creator", :as_moderator do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations models = library.models.take(2) update = {} @@ -161,6 +176,15 @@ end end + it "clears returnable session param", :as_moderator do + update = {} + library.models.take(2).each do |model| + update[model.to_param] = 1 + end + patch "/models/update", params: {models: update, remove_tags: ["a", "b"]} + expect(session[:return_after_new]).to be_nil + end + it "is denied to non-moderators", :as_contributor do update = {} expect { patch "/models/update", params: {models: update, remove_tags: ["a", "b"]} }.to raise_error(Pundit::NotAuthorizedError) @@ -227,6 +251,11 @@ expect(response).to have_http_status(:success) end + it "sets returnable session param", :as_contributor do + get "/models/new" + expect(session[:return_after_new]).to eq "/models/new" + end + it "denies member permission", :as_member do expect { get "/models/new" }.to raise_error(Pundit::NotAuthorizedError) end @@ -238,6 +267,11 @@ expect(response).to redirect_to("/libraries") end + it "clears returnable session param", :as_contributor do + post "/models", params: {library: library.to_param, scan: "1", uploads: "{}"} + expect(session[:return_after_new]).to be_nil + end + it "denies member permission", :as_member do expect { post "/models", params: {post: {library_pick: library.to_param, scan_after_upload: "1"}, upload: {datafiles: []}} }.to raise_error(Pundit::NotAuthorizedError) end From f31c4fe7f90f66fc78448dd86b68c74301e82245 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 13 Sep 2024 13:13:54 +0100 Subject: [PATCH 4/5] select new creator or collection in dropdown --- app/controllers/models_controller.rb | 8 +++++++- app/views/models/_bulk_fields.html.erb | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/controllers/models_controller.rb b/app/controllers/models_controller.rb index 37bb7d0d9..bd25f734a 100644 --- a/app/controllers/models_controller.rb +++ b/app/controllers/models_controller.rb @@ -208,7 +208,13 @@ def get_creators_and_collections end def set_returnable - session[:return_after_new] = request.fullpath + session[:return_after_new] = request.fullpath.split("?")[0] + @new_collection = Collection.find_by(public_id: params[:new_collection]) if params[:new_collection] + @new_creator = Creator.find_by(public_id: params[:new_creator]) if params[:new_creator] + if @model + @model.collection = @new_collection if @new_collection + @model.collection = @new_creator if @new_creator + end end def clear_returnable diff --git a/app/views/models/_bulk_fields.html.erb b/app/views/models/_bulk_fields.html.erb index 7e010c349..0d759d3bd 100644 --- a/app/views/models/_bulk_fields.html.erb +++ b/app/views/models/_bulk_fields.html.erb @@ -2,7 +2,7 @@ <%= form.label :creator_id, class: "col-sm-2 col-form-label" %>
- <%= form.collection_select :creator_id, @creators, :id, :name, {include_blank: true}, {class: "form-control col-auto form-select"} %> + <%= form.collection_select :creator_id, @creators, :id, :name, {include_blank: true, selected: @new_creator&.id}.compact, {class: "form-control col-auto form-select"} %> <%= link_to t("creators.general.new"), new_creator_path, class: "btn btn-outline-secondary col-auto" if policy(:creator).new? %>
@@ -12,7 +12,7 @@ <%= form.label :collection_id, class: "col-sm-2 col-form-label" %>
- <%= form.collection_select :collection_id, @collections, :id, :name, {include_blank: true}, {class: "form-control col-auto form-select"} %> + <%= form.collection_select :collection_id, @collections, :id, :name, {include_blank: true, selected: @new_collection&.id}.compact, {class: "form-control col-auto form-select"} %> <%= link_to t("collections.general.new"), new_collection_path, class: "btn btn-outline-secondary col-auto" if policy(:collection).new? %>
From 761e9af50b16938dc06fa858b50ba3ce398187c4 Mon Sep 17 00:00:00 2001 From: James Smith Date: Fri, 13 Sep 2024 13:24:01 +0100 Subject: [PATCH 5/5] DRY up tests to make linter happy --- spec/requests/models_spec.rb | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/spec/requests/models_spec.rb b/spec/requests/models_spec.rb index 6f96425b5..41379bef5 100644 --- a/spec/requests/models_spec.rb +++ b/spec/requests/models_spec.rb @@ -131,6 +131,14 @@ end describe "PATCH /models/update" do + let(:model_params) { + model_params = {} + library.models.each do |model| + model_params[model.to_param] = 1 + end + model_params + } + it "updates models creator", :as_moderator do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations models = library.models.take(2) update = {} @@ -146,12 +154,7 @@ end it "adds tags to models", :as_moderator do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations - update = {} - library.models.take(2).each do |model| - update[model.to_param] = 1 - end - - patch "/models/update", params: {models: update, add_tags: ["a", "b", "c"]} + patch "/models/update", params: {models: model_params, add_tags: ["a", "b", "c"]} expect(response).to have_http_status(:redirect) library.models.take(2).each do |model| @@ -160,14 +163,12 @@ end it "removes tags from models", :as_moderator do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations - update = {} library.models.take(2).each do |model| model.tag_list = "a, b, c" model.save - update[model.to_param] = 1 end - patch "/models/update", params: {models: update, remove_tags: ["a", "b"]} + patch "/models/update", params: {models: model_params, remove_tags: ["a", "b"]} expect(response).to have_http_status(:redirect) library.models.take(2).each do |model| @@ -177,17 +178,16 @@ end it "clears returnable session param", :as_moderator do - update = {} - library.models.take(2).each do |model| - update[model.to_param] = 1 - end - patch "/models/update", params: {models: update, remove_tags: ["a", "b"]} + patch "/models/update", params: {models: model_params, remove_tags: ["a", "b"]} expect(session[:return_after_new]).to be_nil end it "is denied to non-moderators", :as_contributor do update = {} - expect { patch "/models/update", params: {models: update, remove_tags: ["a", "b"]} }.to raise_error(Pundit::NotAuthorizedError) + library.models.take(2).each do |model| + update[model.to_param] = 1 + end + expect { patch "/models/update", params: {models: model_params, remove_tags: ["a", "b"]} }.to raise_error(Pundit::NotAuthorizedError) end end