From 8eb484218bfae7ae372182e7a7edfb28b063f3f1 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Mon, 11 Sep 2023 17:02:10 +0100 Subject: [PATCH 1/2] AdminIncomingMessageController#redeliver style Minor style fixes: * Line spacing * Line length * Quote style * Interpolation * Variable assignment --- .../admin_incoming_message_controller.rb | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin_incoming_message_controller.rb b/app/controllers/admin_incoming_message_controller.rb index e70d3f9990..d45697eb72 100644 --- a/app/controllers/admin_incoming_message_controller.rb +++ b/app/controllers/admin_incoming_message_controller.rb @@ -78,30 +78,34 @@ def bulk_destroy end def redeliver - - message_ids = params[:url_title].split(",").each(&:strip) + message_ids = params[:url_title].split(',').each(&:strip) previous_request = @incoming_message.info_request destination_request = nil if message_ids.empty? - flash[:error] = "You must supply at least one request to redeliver the message to." + flash[:error] = + 'You must supply at least one request to redeliver the message to.' + return redirect_to admin_request_url(previous_request) end ActiveRecord::Base.transaction do message_ids.each do |m| - if m.match(/^[0-9]+$/) - destination_request = InfoRequest.find_by_id(m.to_i) - else - destination_request = InfoRequest.find_by_url_title!(m) - end + destination_request = + if m.match(/^[0-9]+$/) + InfoRequest.find_by_id(m.to_i) + else + InfoRequest.find_by_url_title!(m) + end + if destination_request.nil? - flash[:error] = "Failed to find destination request '" + m + "'" + flash[:error] = "Failed to find destination request '#{m}'" return redirect_to admin_request_url(previous_request) end raw_email_data = @incoming_message.raw_email.data mail = MailHandler.mail_from_raw_email(raw_email_data) + destination_request. receive(mail, raw_email_data, @@ -114,12 +118,15 @@ def redeliver deleted_incoming_message_id: @incoming_message.id ) - flash[:notice] = "Message has been moved to request(s). Showing the last one:" + flash[:notice] = + 'Message has been moved to request(s). Showing the last one:' end + # expire cached files previous_request.expire @incoming_message.destroy end + redirect_to admin_request_url(destination_request) end From f23d05188d666285f49f0ec93dded2b2a8045098 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Mon, 11 Sep 2023 17:11:08 +0100 Subject: [PATCH 2/2] Fix minor bug redelivering to multiple MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When passing multiple destinations as a CSV [1], we're expecting to strip these values down to an Array of two stripped ID numbers [2]. This wasn't happening as the `each` call wasn't returning the stripped values. As such, we're left with some invalid ID numbers [3] that don't get found later on. This fixes this issue by using `map`, which returns the stripped values. I've also added some specs. They're not the totally complete, but they do fail prior to using `map` and pass afterwards, so good enough for this specific issue. [1] e.g. `"105, 106"` [2] i.e. `["105", "106"]` [3] i.e. `["105", " 106"]` – note the preceding space --- .../admin_incoming_message_controller.rb | 2 +- .../admin_incoming_message_controller_spec.rb | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin_incoming_message_controller.rb b/app/controllers/admin_incoming_message_controller.rb index d45697eb72..d6a0443105 100644 --- a/app/controllers/admin_incoming_message_controller.rb +++ b/app/controllers/admin_incoming_message_controller.rb @@ -78,7 +78,7 @@ def bulk_destroy end def redeliver - message_ids = params[:url_title].split(',').each(&:strip) + message_ids = params[:url_title].split(',').map(&:strip) previous_request = @incoming_message.info_request destination_request = nil diff --git a/spec/controllers/admin_incoming_message_controller_spec.rb b/spec/controllers/admin_incoming_message_controller_spec.rb index d5452060f8..631ab20f9d 100644 --- a/spec/controllers/admin_incoming_message_controller_spec.rb +++ b/spec/controllers/admin_incoming_message_controller_spec.rb @@ -74,6 +74,7 @@ FactoryBot.create(:incoming_message, info_request: previous_info_request) end let(:destination_info_request) { FactoryBot.create(:info_request) } + let(:destination_info_request_2) { FactoryBot.create(:info_request) } it 'expires the file cache for the previous request' do allow(IncomingMessage).to receive(:find).and_return(incoming_message) @@ -107,6 +108,29 @@ expect(response).to redirect_to admin_request_url(incoming_message.info_request) end + context 'when redelivering to multiple requests' do + before do + destination_params = + [destination_info_request, destination_info_request_2]. + map(&:id). + join(', ') + + post :redeliver, + params: { id: incoming_message.id, + url_title: destination_params } + end + + it 'renders a message' do + msg = 'Message has been moved to request(s). Showing the last one:' + expect(flash[:notice]).to eq(msg) + end + + it 'redirects to the last given destination request' do + expect(response). + to redirect_to admin_request_path(destination_info_request_2) + end + end + context 'if the request is embargoed', feature: :alaveteli_pro do before do incoming_message.info_request.create_embargo