diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 5d1e2550e..6d7738204 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -1,21 +1,11 @@ class MergeRequestsController < ApplicationController - before_action :find_resource, only: %i[ - update - organisations - update_organisations - remove_merging_organisation - absorbing_organisation - confirm_telephone_number - new_organisation_name - new_organisation_address - new_organisation_telephone_number - new_organisation_type - merge_date - ] + before_action :find_resource, exclude: %i[create new] before_action :authenticate_user! - before_action :authenticate_scope!, except: [:create] + before_action :authenticate_scope! + before_action :set_organisations_answer_options, only: %i[organisations absorbing_organisation update_organisations remove_merging_organisation update] def absorbing_organisation; end + def organisations; end def confirm_telephone_number; end def new_organisation_name; end def new_organisation_address; end @@ -26,17 +16,12 @@ 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 - def organisations - @answer_options = organisations_answer_options - end - def update validate_response @@ -49,7 +34,6 @@ class MergeRequestsController < ApplicationController def update_organisations merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params) - @answer_options = organisations_answer_options if merge_request_organisation.save render :organisations else @@ -59,7 +43,6 @@ class MergeRequestsController < ApplicationController def remove_merging_organisation MergeRequestOrganisation.find_by(merge_request_organisation_params)&.destroy! - @answer_options = organisations_answer_options render :organisations end @@ -72,13 +55,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" @@ -98,13 +81,16 @@ private params.dig(:merge_request, :absorbing_organisation_id) == "other" end - def organisations_answer_options + def set_organisations_answer_options answer_options = { "" => "Select an option" } - Organisation.all.pluck(:id, :name).each do |organisation| - answer_options[organisation[0]] = organisation[1] + if current_user.support? + Organisation.all.pluck(:id, :name).each do |organisation| + answer_options[organisation[0]] = organisation[1] + end end - answer_options + + @answer_options = answer_options end def merge_request_params @@ -122,9 +108,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? @@ -168,11 +152,13 @@ private end def find_resource + return if params[:id].blank? + @merge_request = MergeRequest.find(params[:id]) 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/app/views/merge_requests/absorbing_organisation.html.erb b/app/views/merge_requests/absorbing_organisation.html.erb index e63154bac..ea54de32a 100644 --- a/app/views/merge_requests/absorbing_organisation.html.erb +++ b/app/views/merge_requests/absorbing_organisation.html.erb @@ -1,32 +1,28 @@ <% content_for :before_content do %> <% title = "Tell us if your organisation is merging" %> <% content_for :title, title %> - <%= govuk_back_link href: organisations_merge_request_path(id: @merge_request) %> + <%= govuk_back_link href: organisations_path(anchor: "merge-requests") %> <% end %> <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> <%= f.govuk_error_summary %> -

Which organisation is absorbing the others?

+

Which organisation is absorbing the others?

-

Select the organisation that the other organisations are merging into.

+

If organisations are merging into a new organisation, <%= govuk_link_to "create the new organisation", new_organisation_path %> first and then select it here.

- <%= f.govuk_radio_buttons_fieldset( - :absorbing_organisation_id, - hint: { text: "For example, if Skype and Yammer merged into Microsoft, you would select Microsoft." }, - legend: nil, - ) do %> - <% @merge_request.merging_organisations.order(:name).each do |org| %> - <%= f.govuk_radio_button( - :absorbing_organisation_id, - org.id, - label: { text: org.name }, - ) %> + <%= f.govuk_select(:absorbing_organisation_id, + label: { text: "Select organisation name", class: "govuk-label--m" }, + "data-controller": "accessible-autocomplete") do %> + <% @answer_options.map { |id, name| OpenStruct.new(id:, name:) }.each do |answer| %> + + <% end %> <% end %> - <%= f.govuk_radio_divider %> - <%= f.govuk_radio_button :absorbing_organisation_id, "other", checked: @merge_request.new_absorbing_organisation?, label: { text: "These organisations are merging into a new one" } %> - <% end %> <%= f.hidden_field :page, value: "absorbing_organisation" %> diff --git a/app/views/organisations/index.html.erb b/app/views/organisations/index.html.erb index 4701e67fd..d95516eba 100644 --- a/app/views/organisations/index.html.erb +++ b/app/views/organisations/index.html.erb @@ -15,7 +15,7 @@ <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "organisations" } %> <% end %> <% c.with_tab(label: "Merge requests") do %> - <%= govuk_button_link_to "Create new merge request", "/organisations/#{current_user.organisation_id}/merge-request", html: { method: :get } %> + <%= govuk_button_to "Create new merge request", merge_requests_path, html: { method: :post } %> <%= render partial: "merge_request_list", locals: { merge_requests: @merge_requests, title: "Merge requests", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> <% end %> <% end %> diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index fe9f1b16e..2ae5dce42 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 @@ -265,33 +256,22 @@ RSpec.describe MergeRequestsController, type: :request do end end - describe "#update" do - before { sign_in user } + describe "#absorbing_organisation" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: 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 + before { get "/merge-request/#{merge_request.id}/absorbing-organisation", headers: } - 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 + it "asks for the absorbing organisation" do + expect(page).to have_content("Which organisation is absorbing the others?") + expect(page).to have_content("Select organisation name") + end - expect(response).to redirect_to(absorbing_organisation_merge_request_path(merge_request)) - end - end + it "has the correct back button" do + expect(page).to have_link("Back", href: organisations_path(anchor: "merge-requests")) end + end + describe "#update" do 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 +293,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 +340,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 +354,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 +379,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 +674,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 diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 6c4bc6202..76ef51749 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1022,7 +1022,7 @@ RSpec.describe OrganisationsController, type: :request do end it "has a create new merge request button" do - expect(page).to have_link("Create new merge request") + expect(page).to have_button("Create new merge request") end it "displays 'No merge requests' when @merge_requests is empty" do