From 9f6ecd844c8c6d00be81e561f32f0e768994e25d Mon Sep 17 00:00:00 2001 From: Kat Date: Fri, 9 Aug 2024 10:17:55 +0100 Subject: [PATCH] Update user permissions and absorbing org question order --- app/controllers/merge_requests_controller.rb | 15 +- .../merge_requests_controller_spec.rb | 197 +++++++++++------- 2 files changed, 125 insertions(+), 87 deletions(-) diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 5d1e2550e..4c674c46b 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -13,7 +13,7 @@ class MergeRequestsController < ApplicationController merge_date ] before_action :authenticate_user! - before_action :authenticate_scope!, except: [:create] + before_action :authenticate_scope! def absorbing_organisation; end def confirm_telephone_number; end @@ -26,9 +26,8 @@ class MergeRequestsController < ApplicationController def create ActiveRecord::Base.transaction do @merge_request = MergeRequest.create!(merge_request_params.merge(status: :incomplete)) - MergeRequestOrganisation.create!({ merge_request: @merge_request, merging_organisation: @merge_request.requesting_organisation }) end - redirect_to organisations_merge_request_path(@merge_request) + redirect_to absorbing_organisation_merge_request_path(@merge_request) rescue ActiveRecord::RecordInvalid render_not_found end @@ -72,13 +71,13 @@ private def next_page_path case page when "absorbing_organisation" + organisations_merge_request_path(@merge_request) + when "organisations" if create_new_organisation? new_organisation_name_merge_request_path(@merge_request) else confirm_telephone_number_merge_request_path(@merge_request) end - when "organisations" - absorbing_organisation_merge_request_path(@merge_request) when "confirm_telephone_number" merge_date_merge_request_path(@merge_request) when "new_organisation_name" @@ -122,9 +121,7 @@ private :new_organisation_telephone_number, ) - if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?) - merge_params[:requesting_organisation_id] = current_user.organisation.id - end + merge_params[:requesting_organisation_id] = current_user.organisation.id if merge_params[:absorbing_organisation_id].present? if create_new_organisation? @@ -172,7 +169,7 @@ private end def authenticate_scope! - if current_user.organisation != @merge_request.requesting_organisation && !current_user.support? + unless current_user.support? render_not_found end end diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index fe9f1b16e..03addfca4 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -10,11 +10,14 @@ RSpec.describe MergeRequestsController, type: :request do let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:other_merge_request) { MergeRequest.create!(requesting_organisation: other_organisation) } - context "when user is signed in with a data coordinator user" do - before { sign_in user } + context "when user is signed in with a support user" do + before do + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in support_user + end describe "#organisations" do - let(:params) { { merge_request: { requesting_organisation_id: "9", status: "incomplete" } } } + let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id, status: "incomplete" } } } context "when creating a new merge request" do before do @@ -23,9 +26,8 @@ RSpec.describe MergeRequestsController, type: :request do it "creates merge request with requesting organisation" do follow_redirect! - expect(page).to have_content("Which organisations are merging?") - expect(page).to have_content(organisation.name) - expect(page).not_to have_link("Remove") + expect(page).to have_content("Which organisation is absorbing the others?") + expect(page).to have_content(support_user.organisation.name) end context "when passing a different requesting organisation id" do @@ -34,9 +36,8 @@ RSpec.describe MergeRequestsController, type: :request do it "creates merge request with current user organisation" do follow_redirect! expect(MergeRequest.count).to eq(1) - expect(MergeRequest.first.requesting_organisation_id).to eq(organisation.id) - expect(MergeRequest.first.merging_organisations.count).to eq(1) - expect(MergeRequest.first.merging_organisations.first.id).to eq(organisation.id) + expect(MergeRequest.first.requesting_organisation_id).to eq(support_user.organisation_id) + expect(MergeRequest.first.merging_organisations.count).to eq(0) end end end @@ -51,16 +52,6 @@ RSpec.describe MergeRequestsController, type: :request do expect(page).to have_content(organisation.name) end end - - context "when viewing existing merge request of a different (unauthorised) organisation" do - before do - get "/merge-request/#{other_merge_request.id}/organisations", headers:, params: - end - - it "shows merge request with requesting organisation" do - expect(response).to have_http_status(:not_found) - end - end end describe "#update_organisations" do @@ -266,32 +257,6 @@ RSpec.describe MergeRequestsController, type: :request do end describe "#update" do - before { sign_in user } - - describe "#other_merging_organisations" do - let(:other_merging_organisations) { "A list of other merging organisations" } - let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } } - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - context "when adding other merging organisations" do - before do - MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) - end - - it "updates the merge request" do - expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations) - end - - it "redirects telephone number path" do - request - - expect(response).to redirect_to(absorbing_organisation_merge_request_path(merge_request)) - end - end - end - describe "from absorbing_organisation page" do context "when not answering the question" do let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } @@ -313,38 +278,46 @@ RSpec.describe MergeRequestsController, type: :request do end end - context "when absorbing_organisation_id set to other" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + context "when absorbing_organisation_id set to id" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } let(:params) do - { merge_request: { absorbing_organisation_id: "other", page: "absorbing_organisation" } } + { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } end + let(:request) do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "redirects to new org path" do + it "redirects to merging organisations path" do request - expect(response).to redirect_to(new_organisation_name_merge_request_path(merge_request)) + expect(response).to redirect_to(organisations_merge_request_path(merge_request)) end - it "resets absorbing_organisation and sets new_absorbing_organisation to true" do + it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do expect { request }.to change { merge_request.reload.absorbing_organisation - }.from(other_organisation).to(nil).and change { + }.from(nil).to(other_organisation).and change { merge_request.reload.new_absorbing_organisation - }.from(nil).to(true) + }.from(true).to(false) end end + end - context "when absorbing_organisation_id set to id" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } - let(:params) do - { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } + describe "#other_merging_organisations" do + let(:other_merging_organisations) { "A list of other merging organisations" } + let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } } + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + context "when adding other merging organisations" do + before do + MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) end - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: + it "updates the merge request" do + expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations) end it "redirects telephone number path" do @@ -352,14 +325,6 @@ RSpec.describe MergeRequestsController, type: :request do expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request)) end - - it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do - expect { request }.to change { - merge_request.reload.absorbing_organisation - }.from(nil).to(other_organisation).and change { - merge_request.reload.new_absorbing_organisation - }.from(true).to(false) - end end end @@ -374,7 +339,7 @@ RSpec.describe MergeRequestsController, type: :request do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "redirects telephone number path" do + it "redirects merge date path" do request expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) @@ -399,7 +364,7 @@ RSpec.describe MergeRequestsController, type: :request do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "redirects telephone number path" do + it "redirects merge date path" do request expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) @@ -694,23 +659,99 @@ RSpec.describe MergeRequestsController, type: :request do end end - context "when user is signed in as a support user" do + context "when user is signed in with a data coordinator user" do before do - allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) - sign_in support_user + sign_in user end describe "#organisations" do let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } - before do - post "/merge-request", headers:, params: + context "when creating a new merge request" do + before do + post "/merge-request", headers:, params: + end + + it "does not allow creating a new merge request" do + expect(response).to have_http_status(:not_found) + end + end + + context "when viewing existing merge request" do + before do + get "/merge-request/#{merge_request.id}/organisations", headers:, params: + end + + it "does not allow viewing a merge request" do + expect(response).to have_http_status(:not_found) + end end + end - it "creates merge request with requesting organisation" do - follow_redirect! - expect(MergeRequest.count).to eq(1) - expect(MergeRequest.first.requesting_organisation_id).to eq(other_organisation.id) + describe "#update_organisations" do + let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } + + context "when updating a merge request with a new organisation" do + before do + patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + end + + it "does not allow updaing a merge request" do + expect(response).to have_http_status(:not_found) + end + end + end + + describe "#remove_organisation" do + let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } + + context "when removing an organisation from merge request" do + before do + MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) + get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params: + end + + it "does not allow removing an organisation" do + expect(response).to have_http_status(:not_found) + end + end + end + + describe "#update" do + describe "from absorbing_organisation page" do + context "when absorbing_organisation_id set to id" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:params) do + { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } + end + + before do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "does not allow updating absorbing organisation" do + expect(response).to have_http_status(:not_found) + end + end + end + + describe "#other_merging_organisations" do + let(:other_merging_organisations) { "A list of other merging organisations" } + let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } } + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + context "when adding other merging organisations" do + before do + MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) + request + end + + it "does not allow updating merging organisations" do + expect(response).to have_http_status(:not_found) + end + end end end end