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

Redirect back to edit page after using new creator/collection button #2677

Merged
merged 5 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/controllers/collections_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
redirect_to session[:return_after_new] + "?new_collection=#{@collection.to_param}", notice: t(".success")
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

session[:return_after_new] = nil
else
redirect_to collections_path, notice: t(".success")
end
end

def update
Expand Down
7 changes: 6 additions & 1 deletion app/controllers/creators_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
redirect_to session[:return_after_new] + "?new_creator=#{@creator.to_param}", notice: t(".success")
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

session[:return_after_new] = nil
else
redirect_to creators_path, notice: t(".success")
end
end

def update
Expand Down
16 changes: 16 additions & 0 deletions app/controllers/models_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down Expand Up @@ -204,4 +206,18 @@ def get_creators_and_collections
@creators = policy_scope(Creator)
@collections = policy_scope(Collection)
end

def set_returnable
Copy link

Choose a reason for hiding this comment

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

Method set_returnable has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

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
session[:return_after_new] = nil
end
end
4 changes: 2 additions & 2 deletions app/views/models/_bulk_fields.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<%= form.label :creator_id, class: "col-sm-2 col-form-label" %>
<div class="col-sm-10">
<div class="input-group">
<%= 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? %>
</div>
</div>
Expand All @@ -12,7 +12,7 @@
<%= form.label :collection_id, class: "col-sm-2 col-form-label" %>
<div class="col-sm-10">
<div class="input-group">
<%= 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? %>
</div>
</div>
Expand Down
13 changes: 8 additions & 5 deletions spec/requests/collections_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: 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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion spec/requests/creators_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: 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
Expand Down
56 changes: 45 additions & 11 deletions spec/requests/models_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand Down Expand Up @@ -110,12 +120,25 @@
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
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 = {}
Expand All @@ -131,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|
Expand All @@ -145,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|
Expand All @@ -161,9 +177,17 @@
end
end

it "clears returnable session param", :as_moderator do
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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
Loading