Skip to content

Commit

Permalink
Use a partial to render batch sent message
Browse files Browse the repository at this point in the history
I don’t really like that we’re using two flash keys to make this happen.
The @info_request should be available as an InfoRequestBatchTemplate or
something in InfoRequestBatch#show so that we can get the law_used_full.
  • Loading branch information
garethrees committed Jan 9, 2015
1 parent 28a00e5 commit d8e5a1b
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 10 deletions.
10 changes: 3 additions & 7 deletions app/controllers/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,9 @@ def new_batch
:body => params[:outgoing_message][:body],
:public_bodies => @public_bodies,
:user => authenticated_user)
flash[:notice] = _("<p>Your {{law_used_full}} requests will be <strong>sent</strong> shortly!</p>
<p><strong>We will email you</strong> when they have been sent.
We will also email you when there is a response to any of them, or after {{late_number_of_days}} working days if the authorities still haven't
replied by then.</p>
<p>If you write about these requests (for example in a forum or a blog) please link to this page.</p>",
:law_used_full=>@info_request.law_used_full,
:late_number_of_days => AlaveteliConfiguration::reply_late_after_days)

flash[:batch_sent] = true
flash[:batch_law_used_full] = @info_request.law_used_full
redirect_to info_request_batch_path(@info_request_batch)
end

Expand Down
19 changes: 19 additions & 0 deletions app/views/info_request_batch/_batch_sent.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<div id="notice">
<p>
<%= _("Your {{law_used_full}} requests will be <strong>sent</strong> shortly!",
:law_used_full => law_used_full) %>
</p>

<p>
<%= _("<strong>We will email you</strong> when they have been sent. " \
"We will also email you when there is a response to any of them, or " \
"after {{late_number_of_days}} working days if the authorities still" \
"haven't replied by then.",
:late_number_of_days => AlaveteliConfiguration::reply_late_after_days) %>
</p>

<p>
<%= _("If you write about these requests (for example in a forum or a blog) " \
"please link to this page.") %>
</p>
</div>
6 changes: 6 additions & 0 deletions app/views/info_request_batch/show.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
<% @title = _("{{title}} - a batch request", :title => @info_request_batch.title) %>

<% if flash[:batch_sent] %>
<%= render :partial => 'batch_sent',
:locals => { :law_used_full => flash[:batch_law_used_full] } %>
<% end %>

<div class="info_request_batch_intro">
<h1><%= @title %></h1>
<% if @info_request_batch.sent_at %>
Expand Down
11 changes: 8 additions & 3 deletions spec/controllers/request_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2564,10 +2564,15 @@ def make_request
assigns[:existing_batch].should_not be_nil
end

it 'should display a success notice' do
it 'sets the batch_sent flash to true' do
make_request
notice_text = "<p>Your Freedom of Information requests will be <strong>sent</strong> shortly!"
flash[:notice].should match notice_text
expect(flash[:batch_sent]).to be_true
end

it 'sets the batch_law_used_full flash to the law used' do
make_request
expected = assigns[:info_request].law_used_full
expect(flash[:batch_law_used_full]).to eq(expected)
end

end
Expand Down

3 comments on commit d8e5a1b

@crowbot
Copy link
Member

@crowbot crowbot commented on d8e5a1b Jan 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think there are two significant bits of context here. One is that the law_used_full attribute is derived from law_used, which in turn is set on InfoRequest initialisation based on the tags applied to the public body. So different requests in a batch could have different values for law_used_full - some of them might be to bodies that only accept environmental information requests. So using the value from the batch template is not really a good proxy for the whole collection.

The second is that, in any case, the distinction between the two types of request is a UK-specific feature which should be moved to the UK theme (#2085).

Given these two factors, I think the cleanest thing might be to drop law_used_full from this descriptive text, and just have it say "Your requests will be sent shortly", without specifying what law will be used.

@garethrees
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So different requests in a batch could have different values for law_used_full

Ah, I had thought that the existing behaviour was "correct", but looks like it also suffers from this problem.

Given these two factors, I think the cleanest thing might be to drop law_used_full from this descriptive text, and just have it say "Your requests will be sent shortly", without specifying what law will be used.

+1. I think a better way to illustrate the distinction would be to use some sort of tag or icon next to each request in the request list.

@garethrees
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2372441

Please sign in to comment.