Skip to content

Commit

Permalink
Fix minor bug redelivering to multiple
Browse files Browse the repository at this point in the history
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
  • Loading branch information
garethrees committed Sep 11, 2023
1 parent f19736a commit 0be1946
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
2 changes: 1 addition & 1 deletion app/controllers/admin_incoming_message_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
25 changes: 25 additions & 0 deletions spec/controllers/admin_incoming_message_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,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)
Expand Down Expand Up @@ -106,6 +107,30 @@
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,

Check warning on line 118 in spec/controllers/admin_incoming_message_controller_spec.rb

View workflow job for this annotation

GitHub Actions / build

[rubocop] reported by reviewdog 🐶 [Correctable] Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is. Raw Output: spec/controllers/admin_incoming_message_controller_spec.rb:118:28: C: [Correctable] Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is. id: incoming_message.id, ^^^^^^^^^^^^^^^^^^^^^^^
url_title: destination_params
}

Check warning on line 120 in spec/controllers/admin_incoming_message_controller_spec.rb

View workflow job for this annotation

GitHub Actions / build

[rubocop] reported by reviewdog 🐶 [Correctable] Layout/FirstHashElementIndentation: Indent the right brace the same as the start of the line where the left brace is. Raw Output: spec/controllers/admin_incoming_message_controller_spec.rb:120:26: C: [Correctable] Layout/FirstHashElementIndentation: Indent the right brace the same as the start of the line where the left brace is. } ^
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
Expand Down

0 comments on commit 0be1946

Please sign in to comment.