From 42f73df872d5731ae3696b64b7ea2a607c167419 Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 11 Jul 2023 12:53:18 +0100 Subject: [PATCH] Update the duplicate journey after deleting logs --- app/controllers/delete_logs_controller.rb | 22 ++++++-- app/controllers/duplicate_logs_controller.rb | 11 ++-- app/helpers/duplicate_logs_helper.rb | 17 +++++++ app/views/duplicate_logs/show.erb | 14 +++--- app/views/logs/delete_duplicates.html.erb | 6 +-- spec/features/lettings_log_spec.rb | 34 +++++++++++-- spec/features/sales_log_spec.rb | 50 +++++++++++++++++++ .../duplicate_logs_controller_spec.rb | 16 +++--- 8 files changed, 141 insertions(+), 29 deletions(-) create mode 100644 app/helpers/duplicate_logs_helper.rb diff --git a/app/controllers/delete_logs_controller.rb b/app/controllers/delete_logs_controller.rb index 75828a2a6..1288007f0 100644 --- a/app/controllers/delete_logs_controller.rb +++ b/app/controllers/delete_logs_controller.rb @@ -27,8 +27,7 @@ class DeleteLogsController < ApplicationController logs = LettingsLog.find(params.require(:ids)) discard logs if request.referer&.include?("delete-duplicates") - log_ids = logs.map { |log| "Log #{log.id}" }.join(", ") - redirect_to lettings_logs_path, notice: I18n.t("notification.duplicate_logs_deleted", count: logs.count, log_ids:) + redirect_to lettings_log_duplicate_logs_path(lettings_log_id: remaining_log_id, original_log_id:), notice: I18n.t("notification.duplicate_logs_deleted", count: logs.count, log_ids: duplicate_log_ids(logs)) else redirect_to lettings_logs_path, notice: I18n.t("notification.logs_deleted", count: logs.count) end @@ -56,8 +55,11 @@ class DeleteLogsController < ApplicationController def discard_sales_logs logs = SalesLog.find(params.require(:ids)) discard logs - - redirect_to sales_logs_path, notice: I18n.t("notification.logs_deleted", count: logs.count) + if request.referer&.include?("delete-duplicates") + redirect_to sales_log_duplicate_logs_path(sales_log_id: remaining_log_id, original_log_id:), notice: I18n.t("notification.duplicate_logs_deleted", count: logs.count, log_ids: duplicate_log_ids(logs)) + else + redirect_to sales_logs_path, notice: I18n.t("notification.logs_deleted", count: logs.count) + end end def delete_lettings_logs_for_organisation @@ -197,4 +199,16 @@ private log.discard! end end + + def duplicate_log_ids(logs) + logs.map { |log| "Log #{log.id}" }.join(", ") + end + + def original_log_id + CGI.parse(URI.parse(request.referer).query)["original_log_id"][0] + end + + def remaining_log_id + request.referer.split("/")[-2] + end end diff --git a/app/controllers/duplicate_logs_controller.rb b/app/controllers/duplicate_logs_controller.rb index f636f8778..546699453 100644 --- a/app/controllers/duplicate_logs_controller.rb +++ b/app/controllers/duplicate_logs_controller.rb @@ -2,14 +2,10 @@ class DuplicateLogsController < ApplicationController before_action :authenticate_user! before_action :find_resource_by_named_id before_action :find_duplicate_logs + before_action :find_original_log_id def show if @log - @duplicate_logs = if @log.lettings? - current_user.lettings_logs.duplicate_logs(@log) - else - current_user.sales_logs.duplicate_logs(@log) - end @all_duplicates = [@log, *@duplicate_logs] @duplicate_check_questions = duplicate_check_question_ids.map { |question_id| question = @log.form.get_question(question_id, @log) @@ -64,4 +60,9 @@ private %w[owning_organisation_id saledate purchid age1 sex1 ecstat1 postcode_full] end end + + def find_original_log_id + query_params = URI.parse(request.url).query + @original_log_id = CGI.parse(query_params)["original_log_id"][0].to_i if query_params + end end diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb new file mode 100644 index 000000000..5c619228a --- /dev/null +++ b/app/helpers/duplicate_logs_helper.rb @@ -0,0 +1,17 @@ +module DuplicateLogsHelper + include GovukLinkHelper + + def duplicate_logs_continue_button(all_duplicates, duplicate_log, original_log) + if all_duplicates.count > 1 + return govuk_button_link_to "Keep this log and delete duplicates", send("#{duplicate_log.class.name.underscore}_delete_duplicates_path", duplicate_log, original_log_id: original_log.id) + end + + if original_log.id == duplicate_log.id + govuk_button_link_to "Continue editing Log #{duplicate_log.id}", send("#{duplicate_log.class.name.underscore}_path", duplicate_log) + else + return govuk_button_link_to "Back to lettings logs", lettings_logs_path if duplicate_log.lettings? + + govuk_button_link_to "Back to sales logs", sales_logs_path + end + end +end diff --git a/app/views/duplicate_logs/show.erb b/app/views/duplicate_logs/show.erb index 8689ebd48..6a96561cb 100644 --- a/app/views/duplicate_logs/show.erb +++ b/app/views/duplicate_logs/show.erb @@ -1,17 +1,19 @@ <% content_for :title, "Check duplicate logs" %>
- <%= govuk_panel( - classes: "app-panel--interruption", - ) do %> -

These logs are duplicates

-

These logs have the same values for the following fields. Choose one to keep or correct the answers.

+ <% if @all_duplicates.count > 1 %> + <%= govuk_panel( + classes: "app-panel--interruption", + ) do %> +

These logs are duplicates

+

These logs have the same values for the following fields. Choose one to keep or correct the answers.

+ <% end %> <% end %> <% @all_duplicates.each_with_index do |log, index| %> <%= render partial: "duplicate_log", locals: { log: log } %> <%= render partial: "duplicate_log_check_answers", locals: { log: log } %> - <%= govuk_button_link_to "Keep this log and delete duplicates", log.lettings? ? lettings_log_delete_duplicates_path(log) : sales_log_delete_duplicates_path(log) %> + <%= duplicate_logs_continue_button(@all_duplicates, log, @log) %> <% if index < @all_duplicates.count - 1 %>
<% end %> diff --git a/app/views/logs/delete_duplicates.html.erb b/app/views/logs/delete_duplicates.html.erb index 60fffc48f..3ee368164 100644 --- a/app/views/logs/delete_duplicates.html.erb +++ b/app/views/logs/delete_duplicates.html.erb @@ -26,12 +26,12 @@
<%= govuk_button_to @duplicate_logs.count == 1 ? "Delete this log" : "Delete these logs", - delete_logs_lettings_logs_path, + send("delete_logs_#{@log.class.name.underscore}s_path"), method: "delete", - params: { ids: @duplicate_logs.map(&:id) } %> + params: { ids: @duplicate_logs.map(&:id), original_log_id: @original_log_id } %> <%= govuk_button_link_to( "Cancel", - @log.lettings? ? lettings_log_duplicate_logs_path(@log) : sales_log_duplicate_logs_path(@log), + send("#{@log.class.name.underscore}_duplicate_logs_path", @log), secondary: true, ) %>
diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index bee9bea9c..020910672 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -390,7 +390,7 @@ RSpec.describe "Lettings Log Features" do end end - context "when there are duplicate logs" do + context "when a log becomes a duplicate" do let(:lettings_log) { create(:lettings_log, :completed, owning_organisation: user.organisation, created_by: user) } let!(:duplicate_log) do duplicate = lettings_log.dup @@ -399,13 +399,41 @@ RSpec.describe "Lettings Log Features" do duplicate end - it "is possible to delete duplicate logs" do - visit lettings_log_delete_duplicates_path(lettings_log) + before do + lettings_log.update!(tenancycode: "different") + visit("/lettings-logs/#{lettings_log.id}/tenant-code") + fill_in("lettings-log-tenancycode-field", with: duplicate_log.tenancycode) + click_button("Save and continue") + end + + it "allows keeping the original log and deleting duplicates" do + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/duplicate-logs") + click_link("Keep this log and delete duplicates", href: "/lettings-logs/#{lettings_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") click_button "Delete this log" duplicate_log.reload expect(duplicate_log.status).to eq("deleted") expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") expect(page).to have_content("Log #{duplicate_log.id} has been deleted") + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + expect(page).not_to have_content("These logs are duplicates") + expect(page).not_to have_link("Keep this log and delete duplicates") + expect(page).to have_link("Continue editing Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") + end + + it "allows keeping the duplicate log and deleting the original one" do + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/duplicate-logs") + click_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_current_path("/lettings-logs/#{duplicate_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + click_button "Delete this log" + lettings_log.reload + expect(lettings_log.status).to eq("deleted") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{lettings_log.id} has been deleted") + expect(page).to have_current_path("/lettings-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + expect(page).not_to have_content("These logs are duplicates") + expect(page).not_to have_link("Keep this log and delete duplicates") + expect(page).to have_link("Back to lettings logs", href: "/lettings-logs") end end end diff --git a/spec/features/sales_log_spec.rb b/spec/features/sales_log_spec.rb index 890c82786..301f8e648 100644 --- a/spec/features/sales_log_spec.rb +++ b/spec/features/sales_log_spec.rb @@ -137,4 +137,54 @@ RSpec.describe "Sales Log Features" do end end end + + context "when a log becomes a duplicate" do + let(:user) { create(:user, :data_coordinator) } + let(:sales_log) { create(:sales_log, :completed, owning_organisation: user.organisation, created_by: user) } + let!(:duplicate_log) do + duplicate = sales_log.dup + duplicate.id = nil + duplicate.save! + duplicate + end + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + sales_log.update!(purchid: "different") + visit("/sales-logs/#{sales_log.id}/purchaser-code") + fill_in("sales-log-purchid-field", with: duplicate_log.purchid) + click_button("Save and continue") + end + + it "allows keeping the original log and deleting duplicates" do + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/duplicate-logs") + click_link("Keep this log and delete duplicates", href: "/sales-logs/#{sales_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + click_button "Delete this log" + duplicate_log.reload + expect(duplicate_log.status).to eq("deleted") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{duplicate_log.id} has been deleted") + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + expect(page).not_to have_content("These logs are duplicates") + expect(page).not_to have_link("Keep this log and delete duplicates") + expect(page).to have_link("Continue editing Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") + end + + it "allows keeping the duplicate log and deleting the original one" do + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/duplicate-logs") + click_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_current_path("/sales-logs/#{duplicate_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + click_button "Delete this log" + sales_log.reload + expect(sales_log.status).to eq("deleted") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{sales_log.id} has been deleted") + expect(page).to have_current_path("/sales-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + expect(page).not_to have_content("These logs are duplicates") + expect(page).not_to have_link("Keep this log and delete duplicates") + expect(page).to have_link("Back to sales logs", href: "/sales-logs") + end + end end diff --git a/spec/requests/duplicate_logs_controller_spec.rb b/spec/requests/duplicate_logs_controller_spec.rb index 7eb4a2461..b9b5c0dd7 100644 --- a/spec/requests/duplicate_logs_controller_spec.rb +++ b/spec/requests/duplicate_logs_controller_spec.rb @@ -75,9 +75,9 @@ RSpec.describe DuplicateLogsController, type: :request do it "displays buttons to delete" do expect(page).to have_link("Keep this log and delete duplicates", count: 3) - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{lettings_log.id}/delete-duplicates") - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.first.id}/delete-duplicates") - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.second.id}/delete-duplicates") + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{lettings_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{lettings_log.id}") end end @@ -107,9 +107,9 @@ RSpec.describe DuplicateLogsController, type: :request do it "displays buttons to delete" do expect(page).to have_link("Keep this log and delete duplicates", count: 3) - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{sales_log.id}/delete-duplicates") - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.first.id}/delete-duplicates") - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.second.id}/delete-duplicates") + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{sales_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{sales_log.id}") end end end @@ -118,7 +118,7 @@ RSpec.describe DuplicateLogsController, type: :request do describe "GET sales delete-duplicates" do let(:headers) { { "Accept" => "text/html" } } let(:id) { sales_log.id } - let(:request) { get "/sales-logs/#{id}/delete-duplicates" } + let(:request) { get "/sales-logs/#{id}/delete-duplicates?original_log_id=#{id}" } before do allow(user).to receive(:need_two_factor_authentication?).and_return(false) @@ -207,7 +207,7 @@ RSpec.describe DuplicateLogsController, type: :request do describe "GET lettings delete-duplicates" do let(:id) { lettings_log.id } - let(:request) { get "/lettings-logs/#{id}/delete-duplicates" } + let(:request) { get "/lettings-logs/#{id}/delete-duplicates?original_log_id=#{id}" } before do allow(user).to receive(:need_two_factor_authentication?).and_return(false)