diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index a6e2c08e58..e38d1bdf0e 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -116,6 +116,7 @@ def set_organisations_answer_options def merge_request_params merge_params = params.fetch(:merge_request, {}).permit( :requesting_organisation_id, + :has_helpdesk_ticket, :helpdesk_ticket, :status, :absorbing_organisation_id, @@ -124,6 +125,7 @@ def merge_request_params ) merge_params[:requesting_organisation_id] = current_user.organisation.id + merge_params[:helpdesk_ticket] = nil if merge_params[:has_helpdesk_ticket] == "false" merge_params end @@ -151,6 +153,14 @@ def validate_response if merge_request_params[:existing_absorbing_organisation].nil? @merge_request.errors.add(:existing_absorbing_organisation, :blank) end + when "helpdesk_ticket" + @merge_request.has_helpdesk_ticket = merge_request_params[:has_helpdesk_ticket] + @merge_request.helpdesk_ticket = merge_request_params[:helpdesk_ticket] + if merge_request_params[:has_helpdesk_ticket].blank? + @merge_request.errors.add(:has_helpdesk_ticket, :blank) + elsif merge_request_params[:has_helpdesk_ticket] == "true" && merge_request_params[:helpdesk_ticket].blank? + @merge_request.errors.add(:helpdesk_ticket, :blank) + end end end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 28c6939351..a8ed721204 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -9,7 +9,7 @@ def display_value_or_placeholder(value, placeholder = "You didn't answer this qu def request_details(merge_request) [ { label: "Requester", value: display_value_or_placeholder(merge_request.requester&.name) }, - { label: "Helpdesk ticket", value: merge_request.helpdesk_ticket.present? ? link_to("#{merge_request.helpdesk_ticket} (opens in a new tab)", "https://mhclgdigital.atlassian.net/browse/#{merge_request.helpdesk_ticket}", target: "_blank", rel: "noopener noreferrer") : display_value_or_placeholder(nil), action: merge_request_action(merge_request, "helpdesk_ticket") }, + { label: "Helpdesk ticket", value: helpdesk_ticket_value(merge_request), action: merge_request_action(merge_request, "helpdesk_ticket") }, { label: "Status", value: status_tag(merge_request.status) }, ] end @@ -280,4 +280,14 @@ def any_organisations_share_logs?(organisations, type) def begin_merge_disabled?(merge_request) merge_request.status != "ready_to_merge" || merge_request.merge_date.future? end + + def helpdesk_ticket_value(merge_request) + if merge_request.helpdesk_ticket.present? + link_to("#{merge_request.helpdesk_ticket} (opens in a new tab)", "https://mhclgdigital.atlassian.net/browse/#{merge_request.helpdesk_ticket}", target: "_blank", rel: "noopener noreferrer") + elsif merge_request.has_helpdesk_ticket == false + "Not reported by a helpdesk ticket" + else + display_value_or_placeholder(nil) + end + end end diff --git a/app/views/merge_requests/helpdesk_ticket.html.erb b/app/views/merge_requests/helpdesk_ticket.html.erb index 4ebd113952..9ebed7a907 100644 --- a/app/views/merge_requests/helpdesk_ticket.html.erb +++ b/app/views/merge_requests/helpdesk_ticket.html.erb @@ -7,14 +7,27 @@ <%= form_with model: @merge_request, url: submit_merge_request_url(request.query_parameters["referrer"]), method: :patch do |f| %> <%= f.govuk_error_summary %> -

Which helpdesk ticket reported this merge?

-

If this merge was reported via a helpdesk ticket, provide the ticket number.
The ticket will be linked to the merge request for reference.

-
- <%= f.govuk_text_field :helpdesk_ticket, caption: { text: "Ticket number", class: "govuk-label govuk-label--s" }, label: { text: "For example, MSD-12345", class: "app-!-colour-muted" } %> + <%= f.govuk_radio_buttons_fieldset :has_helpdesk_ticket, + legend: { text: "Was this merge reported by a helpdesk ticket?", size: "l" } do %> + + <%= f.govuk_radio_button "has_helpdesk_ticket", + true, + label: { text: "Yes" }, + **basic_conditional_html_attributes({ "helpdesk_ticket" => [true] }, "merge_request") do %> + <%= f.govuk_text_field :helpdesk_ticket, + caption: { text: "Ticket number", class: "govuk-label govuk-label--s" }, + label: { text: "For example, MSD-12345", class: "app-!-colour-muted" } %> + <% end %> + + <%= f.govuk_radio_button "has_helpdesk_ticket", + false, + label: { text: "No" } %> + <% end %> + <%= f.hidden_field :page, value: "helpdesk_ticket" %>
<%= f.govuk_submit submit_merge_request_button_text(request.query_parameters["referrer"]) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 6986187176..80711129e5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -188,6 +188,10 @@ en: blank: "You must answer absorbing organisation already active?" merging_organisation_id: part_of_another_merge: "Another merge request records %{organisation} as merging into %{absorbing_organisation} on %{merge_date}. Select another organisation or remove this organisation from the other merge request." + has_helpdesk_ticket: + blank: "You must answer was this merge reported by a helpdesk ticket?" + helpdesk_ticket: + blank: "You must answer the ticket number" notification: attributes: title: diff --git a/db/migrate/20241122154743_add_has_helpdest_ticket.rb b/db/migrate/20241122154743_add_has_helpdest_ticket.rb new file mode 100644 index 0000000000..103611ad86 --- /dev/null +++ b/db/migrate/20241122154743_add_has_helpdest_ticket.rb @@ -0,0 +1,5 @@ +class AddHasHelpdestTicket < ActiveRecord::Migration[7.0] + def change + add_column :merge_requests, :has_helpdesk_ticket, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 9aa744dc2c..c53872020a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_11_18_104046) do +ActiveRecord::Schema[7.0].define(version: 2024_11_22_154743) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -478,6 +478,7 @@ t.boolean "request_merged" t.boolean "processing" t.boolean "existing_absorbing_organisation" + t.boolean "has_helpdesk_ticket" end create_table "notifications", force: :cascade do |t| diff --git a/spec/factories/merge_request.rb b/spec/factories/merge_request.rb index 19020fce1a..4b33e40026 100644 --- a/spec/factories/merge_request.rb +++ b/spec/factories/merge_request.rb @@ -2,6 +2,7 @@ factory :merge_request do status { "incomplete" } merge_date { nil } + has_helpdesk_ticket { true } helpdesk_ticket { "MSD-99999" } association :requesting_organisation, factory: :organisation end diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 2e2488b930..074d2186cb 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -260,7 +260,7 @@ end it "shows the correct content" do - expect(page).to have_content("Which helpdesk ticket reported this merge?") + expect(page).to have_content("Was this merge reported by a helpdesk ticket?") expect(page).to have_link("Back", href: existing_absorbing_organisation_merge_request_path(merge_request)) expect(page).to have_button("Save and continue") end @@ -476,6 +476,82 @@ end end end + + describe "from helpdesk_ticket page" do + context "when not answering the question" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "helpdesk_ticket" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("You must answer was this merge reported by a helpdesk ticket?") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end + end + + context "when has_helpdesk_ticket is true but no ticket is given" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "helpdesk_ticket", has_helpdesk_ticket: true } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("You must answer the ticket number") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end + end + + context "when has_helpdesk_ticket is false" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation, helpdesk_ticket: "123") } + let(:params) do + { merge_request: { page: "helpdesk_ticket", has_helpdesk_ticket: false } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "updates has_helpdesk_ticket and clears helpdesk_ticket" do + request + expect(merge_request.reload.has_helpdesk_ticket).to eq(false) + expect(merge_request.helpdesk_ticket).to eq(nil) + end + end + + context "when has_helpdesk_ticket is true and ticket is given" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "helpdesk_ticket", has_helpdesk_ticket: true, helpdesk_ticket: "321" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "updates has_helpdesk_ticket and clears helpdesk_ticket" do + request + expect(merge_request.reload.has_helpdesk_ticket).to eq(true) + expect(merge_request.helpdesk_ticket).to eq("321") + end + end + end end describe "#merge_start_confirmation" do