diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 13 | ||||
| -rw-r--r-- | app/models/info_request_batch.rb | 12 | ||||
| -rw-r--r-- | app/views/request/new.html.erb | 7 | ||||
| -rw-r--r-- | db/migrate/20131024152540_add_body_to_info_request_batches.rb | 11 | ||||
| -rw-r--r-- | spec/controllers/request_controller_spec.rb | 11 | ||||
| -rw-r--r-- | spec/factories.rb | 1 | ||||
| -rw-r--r-- | spec/models/info_request_batch_spec.rb | 45 |
7 files changed, 95 insertions, 5 deletions
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 047fc7acf..ccf824e75 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -188,13 +188,9 @@ class RequestController < ApplicationController redirect_to select_authorities_path and return end - # TODO: I do think we should probably check for double submission of batch - # requests as we do in 'new' for ordinary requests with find_existing - # TODO: Decide if we make batch requesters describe their undescribed requests # before being able to make a new batch request - if !authenticated_user.can_file_requests? @details = authenticated_user.can_fail_html render :template => 'user/banned' and return @@ -205,12 +201,18 @@ class RequestController < ApplicationController return render_new_compose(batch=true) end + # Check for double submission of batch + @existing_batch = InfoRequestBatch.find_existing(authenticated_user, + params[:info_request][:title], + params[:outgoing_message][:body], + params[:public_body_ids]) + @info_request = InfoRequest.create_from_attributes(params[:info_request], params[:outgoing_message], authenticated_user) @outgoing_message = @info_request.outgoing_messages.first @info_request.is_batch_request_template = true - if !@info_request.valid? + if !@existing_batch.nil? || !@info_request.valid? # We don't want the error "Outgoing messages is invalid", as in this # case the list of errors will also contain a more specific error # describing the reason it is invalid. @@ -227,6 +229,7 @@ class RequestController < ApplicationController # TODO: give messages about bodies # that are no longer requestable @info_request_batch = InfoRequestBatch.create!(:title => params[:info_request][:title], + :body => params[:outgoing_message][:body], :user => authenticated_user) @public_bodies = PublicBody.where({:id => params[:public_body_ids]}).all @public_bodies.each do |public_body| diff --git a/app/models/info_request_batch.rb b/app/models/info_request_batch.rb index af6956095..05175a889 100644 --- a/app/models/info_request_batch.rb +++ b/app/models/info_request_batch.rb @@ -16,5 +16,17 @@ class InfoRequestBatch < ActiveRecord::Base validates_presence_of :user validates_presence_of :title + validates_presence_of :body + + # When constructing a new batch, use this to check user hasn't double submitted. + def InfoRequestBatch.find_existing(user, title, body, public_body_ids) + find(:first, :conditions => ['info_request_batches.user_id = ? + AND info_request_batches.title = ? + AND body = ? + AND info_requests.public_body_id in (?)', + user, title, body, public_body_ids], + :include => :info_requests) + end + end diff --git a/app/views/request/new.html.erb b/app/views/request/new.html.erb index 91171da3e..8534bf0da 100644 --- a/app/views/request/new.html.erb +++ b/app/views/request/new.html.erb @@ -33,6 +33,13 @@ </li> </ul></div> <% end %> + <% if @existing_batch %> + <div class="errorExplanation" id="errorExplanation"><ul> + <li> + <%= _('You already created the same batch of requests on {{date}}. You can either view the <a href="{{existing_batch}}">existing batch</a>, or edit the details below to make a new but similar batch of requests.', :date=>simple_date(@existing_batch.created_at), :existing_batch=>info_request_batch_path(@existing_batch)) %> + </li> + </ul></div> + <% end %> <%= foi_error_messages_for :info_request, :outgoing_message %> diff --git a/db/migrate/20131024152540_add_body_to_info_request_batches.rb b/db/migrate/20131024152540_add_body_to_info_request_batches.rb new file mode 100644 index 000000000..5f9b3af10 --- /dev/null +++ b/db/migrate/20131024152540_add_body_to_info_request_batches.rb @@ -0,0 +1,11 @@ +class AddBodyToInfoRequestBatches < ActiveRecord::Migration + def up + add_column :info_request_batches, :body, :text + add_index :info_request_batches, [:user_id, :body, :title] + end + + def down + remove_column :info_request_batches, :body + end + +end diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index 6652d8f1f..c61e0c780 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -2573,6 +2573,17 @@ describe RequestController, "#new_batch", :focus => true do response.should redirect_to(info_request_batch_path(new_info_request_batch)) end + it 'should prevent double submission of a batch request' do + params = @default_post_params.merge(:preview => 0) + post :new_batch, params, { :user_id => @user.id } + new_info_request_batch = assigns[:info_request_batch] + response.should redirect_to(info_request_batch_path(new_info_request_batch)) + post :new_batch, params, { :user_id => @user.id } + response.should render_template('new') + assigns[:existing_batch].should_not be_nil + end + + context "when the user is banned" do before do diff --git a/spec/factories.rb b/spec/factories.rb index 66388af6e..3ea943030 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -148,5 +148,6 @@ FactoryGirl.define do factory :info_request_batch do title "Example title" user + body "Some text" end end diff --git a/spec/models/info_request_batch_spec.rb b/spec/models/info_request_batch_spec.rb index f6d9bea0c..787b812cf 100644 --- a/spec/models/info_request_batch_spec.rb +++ b/spec/models/info_request_batch_spec.rb @@ -18,4 +18,49 @@ describe InfoRequestBatch, "when validating" do @info_request_batch.errors.full_messages.should == ["Title can't be blank"] end + it 'should require a body' do + @info_request_batch.body = nil + @info_request_batch.valid?.should be_false + @info_request_batch.errors.full_messages.should == ["Body can't be blank"] + end + +end + +describe InfoRequestBatch, "when finding an existing batch" do + + before do + @info_request_batch = FactoryGirl.create(:info_request_batch, :title => 'Matched title', + :body => 'Matched body') + @first_request = FactoryGirl.create(:info_request, :info_request_batch => @info_request_batch) + @second_request = FactoryGirl.create(:info_request, :info_request_batch => @info_request_batch) + end + + it 'should return a batch with the same user, title and body sent to one of the same public bodies' do + InfoRequestBatch.find_existing(@info_request_batch.user, + @info_request_batch.title, + @info_request_batch.body, + [@first_request.public_body_id]).should_not be_nil + end + + it 'should not return a batch with the same title and body sent to another public body' do + InfoRequestBatch.find_existing(@info_request_batch.user, + @info_request_batch.title, + @info_request_batch.body, + [FactoryGirl.create(:public_body).id]).should be_nil + end + + it 'should not return a batch sent the same public bodies with a different title and body' do + InfoRequestBatch.find_existing(@info_request_batch.user, + 'Other title', + 'Other body', + [@first_request.public_body_id]).should be_nil + end + + it 'should not return a batch sent to one of the same public bodies with the same title and body by + a different user' do + InfoRequestBatch.find_existing(FactoryGirl.create(:user), + @info_request_batch.title, + @info_request_batch.body, + [@first_request.public_body_id]).should be_nil + end end |