Merge remote-tracking branch 'openaustralia_github/more_info_on_report_request' into rails-3-develop

9 files changed, 240 insertions, 113 deletions

diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb
new file mode 100644
index 000000000..a1dd53125
--- /dev/null
+++ b/app/controllers/reports_controller.rb
@@ -0,0 +1,31 @@
+class ReportsController < ApplicationController
+    def create
+        @info_request = InfoRequest.find_by_url_title!(params[:request_id])
+        @reason = params[:reason]
+        @message = params[:message]
+        if @reason.empty?
+            flash[:error] = _("Please choose a reason")
+            render "new"
+            return
+        end
+
+        if !authenticated_user
+            flash[:notice] = _("You need to be logged in to report a request for administrator attention")
+        elsif @info_request.attention_requested
+            flash[:notice] = _("This request has already been reported for administrator attention")
+        else
+            @info_request.report!(@reason, @message, @user)
+            flash[:notice] = _("This request has been reported for administrator attention")
+        end
+        redirect_to request_url(@info_request)
+    end
+
+    def new
+        @info_request = InfoRequest.find_by_url_title!(params[:request_id])
+        if authenticated?(
+            :web => _("To report this request"),
+            :email => _("Then you can report the request '{{title}}'", :title => @info_request.title),
+            :email_subject => _("Report an offensive or unsuitable request"))
+        end
+    end
+end
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index e8547f72f..6ca4e9f82 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -680,25 +680,6 @@ class RequestController < ApplicationController
         end
     end
 
-    def report_request
-        info_request = InfoRequest.find_by_url_title!(params[:url_title])
-        return if !authenticated?(
-                :web => _("To report this FOI request"),
-                :email => _("Then you can report the request '{{title}}'", :title => info_request.title),
-                :email_subject => _("Report an offensive or unsuitable request")
-            )
-
-        if !info_request.attention_requested
-            info_request.set_described_state('attention_requested', @user)
-            info_request.attention_requested = true # tells us if attention has ever been requested
-            info_request.save!
-            flash[:notice] = _("This request has been reported for administrator attention")
-        else
-            flash[:notice] = _("This request has already been reported for administrator attention")
-        end
-        redirect_to request_url(info_request)
-    end
-
     # special caching code so mime types are handled right
     around_filter :cache_attachments, :only => [ :get_attachment, :get_attachment_as_html ]
     def cache_attachments
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 553bb2436..aaf171c4c 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -108,6 +108,12 @@ class InfoRequest < ActiveRecord::Base
         states
     end
 
+    # Possible reasons that a request could be reported for administrator attention
+    def report_reasons
+        ["Contains defamatory material", "Not a valid request", "Request for personal information",
+            "Contains personal information", "Vexatious", "Other"]
+    end
+
     def must_be_valid_state
         errors.add(:described_state, "is not a valid state") if
             !InfoRequest.enumerate_states.include? described_state
@@ -543,6 +549,15 @@ public
         ['requires_admin', 'error_message', 'attention_requested'].include?(described_state)
     end
 
+    # Report this request for administrator attention
+    def report!(reason, message, user)
+        ActiveRecord::Base.transaction do
+            set_described_state('attention_requested', user, "Reason: #{reason}\n\n#{message}")
+            self.attention_requested = true # tells us if attention has ever been requested
+            save!
+        end
+    end
+
     # change status, including for last event for later historical purposes
     def set_described_state(new_state, set_by = nil, message = "")
         old_described_state = described_state
diff --git a/app/views/reports/new.html.erb b/app/views/reports/new.html.erb
new file mode 100644
index 000000000..7d558ab4e
--- /dev/null
+++ b/app/views/reports/new.html.erb
@@ -0,0 +1,26 @@
+<h1>Report request: <%= @info_request.title %></h1>
+
+<% if @info_request.attention_requested %>
+  <p><%= _("This request has already been reported for administrator attention") %></p>
+<% else %>
+  <p>
+    Reporting a request notifies the site administrators. They will respond as soon as possible.
+  </p>
+  <p>Why specifically do you consider this request unsuitable?</p>
+
+  <%= form_tag request_report_path(:request_id => @info_request.url_title) do %>
+    <p>
+      <label class="form_label" for="reason">Reason:</label>
+      <%= select_tag :reason, options_for_select(@info_request.report_reasons, @reason), :prompt => "Choose a reason" %>
+    </p>
+    <p>
+      <label class="form_label" for="message">Please tell us more:</label>
+      <%= text_area_tag :message, @message, :rows => 10, :cols => 60 %>
+    </p>
+
+    <div class="form_button">
+        <%= submit_tag _("Report request") %>
+    </div>
+
+  <% end %>
+<% end %>
diff --git a/app/views/request/_sidebar.html.erb b/app/views/request/_sidebar.html.erb
index 4bc8826fd..aba5c2fb3 100644
--- a/app/views/request/_sidebar.html.erb
+++ b/app/views/request/_sidebar.html.erb
@@ -30,7 +30,7 @@
      <% else %>
       <p><%= _('Requests for personal information and vexatious requests are not considered valid for FOI purposes (<a href="/help/about">read more</a>).') %></p>
        <p><%= _('If you believe this request is not suitable, you can report it for attention by the site administrators') %></p>
-       <%= button_to _("Report this request"), report_path(:url_title => @info_request.url_title), :class => "link_button_green" %>
+       <%= link_to _("Report this request"), new_request_report_path(:request_id => @info_request.url_title) %>
       <% end %>
     <% end %>
     <h2><%= _("Act on what you've learnt") %></h2>
diff --git a/config/routes.rb b/config/routes.rb
index bc03d91e1..56be975c3 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -57,15 +57,12 @@ Alaveteli::Application.routes.draw do
 
     match '/upload/request/:url_title' => 'request#upload_response', :as => :upload_response
     match '/request/:url_title/download' => 'request#download_entire_request', :as => :download_entire_request
-
-    # It would be nice to add :conditions => { :method => :post } to this next one,
-    # because it ought not really to be available as a GET request since it changes
-    # the server state. Unfortunately this doesn’t play well with the PostRedirect
-    # mechanism, which assumes all post-login actions are available via GET, so we
-    # refrain.
-    match '/request/:url_title/report' => 'request#report_request', :as => :report
     ####
 
+    resources :request, :only => [] do
+        resource :report, :only => [:new, :create]
+    end
+
     #### User controller
     # Use /profile for things to do with the currently signed in user.
     # Use /user/XXXX for things that anyone can see about that user.
diff --git a/spec/controllers/reports_controller_spec.rb b/spec/controllers/reports_controller_spec.rb
new file mode 100644
index 000000000..fa8c72eaa
--- /dev/null
+++ b/spec/controllers/reports_controller_spec.rb
@@ -0,0 +1,104 @@
+require 'spec_helper'
+
+describe ReportsController, "when reporting a request when not logged in" do
+    it "should only allow logged-in users to report requests" do
+        post :create, :request_id => info_requests(:badger_request).url_title, :reason => "my reason"
+
+        flash[:notice].should =~ /You need to be logged in/
+        response.should redirect_to show_request_path(:url_title => info_requests(:badger_request).url_title)
+    end
+end
+
+describe ReportsController, "when reporting a request (logged in)" do
+    render_views
+
+    before do
+        @user = users(:robin_user)
+        session[:user_id] = @user.id
+    end
+
+    it "should 404 for non-existent requests" do
+      lambda {
+        post :create, :request_id => "hjksfdhjk_louytu_qqxxx"
+      }.should raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it "should mark a request as having been reported" do
+        ir = info_requests(:badger_request)
+        title = ir.url_title
+        ir.attention_requested.should == false
+
+        post :create, :request_id => title, :reason => "my reason"
+        response.should redirect_to show_request_path(:url_title => title)
+
+        ir.reload
+        ir.attention_requested.should == true
+        ir.described_state.should == "attention_requested"
+    end
+
+    it "should pass on the reason and message" do
+        info_request = mock_model(InfoRequest, :url_title => "foo", :attention_requested= => nil, :save! => nil)
+        InfoRequest.should_receive(:find_by_url_title!).with("foo").and_return(info_request)
+        info_request.should_receive(:report!).with("Not valid request", "It's just not", @user)
+        post :create, :request_id => "foo", :reason => "Not valid request", :message => "It's just not"
+    end
+
+    it "should not allow a request to be reported twice" do
+        title = info_requests(:badger_request).url_title
+
+        post :create, :request_id => title, :reason => "my reason"
+        response.should redirect_to show_request_url(:url_title => title)
+
+        post :create, :request_id => title, :reason => "my reason"
+        response.should redirect_to show_request_url(:url_title => title)
+        flash[:notice].should =~ /has already been reported/
+    end
+
+    it "should send an email from the reporter to admins" do
+        ir = info_requests(:badger_request)
+        title = ir.url_title
+        post :create, :request_id => title, :reason => "my reason"
+        deliveries = ActionMailer::Base.deliveries
+        deliveries.size.should == 1
+        mail = deliveries[0]
+        mail.subject.should =~ /attention_requested/
+        mail.from.should include(@user.email)
+        mail.body.should include(@user.name)
+    end
+
+    it "should force the user to pick a reason" do
+        info_request = mock_model(InfoRequest, :report! => nil, :url_title => "foo",
+            :report_reasons => ["Not FOIish enough"])
+        InfoRequest.should_receive(:find_by_url_title!).with("foo").and_return(info_request)
+
+        post :create, :request_id => "foo", :reason => ""
+        response.should render_template("new")
+        flash[:error].should == "Please choose a reason"
+    end
+end
+
+describe ReportsController, "#new_report_request" do
+    let(:info_request) { mock_model(InfoRequest, :url_title => "foo") }
+    before :each do
+        InfoRequest.should_receive(:find_by_url_title!).with("foo").and_return(info_request)
+    end
+
+    context "not logged in" do
+        it "should require the user to be logged in" do
+            get :new, :request_id => "foo"
+            response.should_not render_template("new")
+        end
+    end
+
+    context "logged in" do
+        before :each do
+            session[:user_id] = users(:bob_smith_user).id
+        end
+        it "should show the form" do
+            get :new, :request_id => "foo"
+            response.should render_template("new")
+        end
+    end
+end
+
+
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 122584c7d..5d10c88ad 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -241,6 +241,36 @@ describe RequestController, "when showing one request" do
       end
     end
 
+    context "when the request has not yet been reported" do
+        it "should allow the user to report" do
+            title = info_requests(:badger_request).url_title
+            get :show, :url_title => title
+            response.should_not contain("This request has been reported")
+            response.should contain("Offensive?")
+        end
+    end
+
+    context "when the request has been reported for admin attention" do
+        before :each do
+            info_requests(:fancy_dog_request).report!("", "", nil)
+        end
+        it "should inform the user" do
+            get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
+            response.should contain("This request has been reported")
+            response.should_not contain("Offensive?")
+        end
+
+        context "and then deemed okay and left to complete" do
+            before :each do
+                info_requests(:fancy_dog_request).set_described_state("successful")
+            end
+            it "should let the user know that the administrators have not hidden this request" do
+                get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
+                response.body.should =~ (/the site administrators.*have not hidden it/)
+            end
+        end
+    end
+
     describe 'when the request is being viewed by an admin' do
 
         describe 'if the request is awaiting description' do
@@ -2319,91 +2349,6 @@ describe RequestController, "when showing similar requests" do
 
 end
 
-
-describe RequestController, "when reporting a request when not logged in" do
-    it "should only allow logged-in users to report requests" do
-        get :report_request, :url_title => info_requests(:badger_request).url_title
-        post_redirect = PostRedirect.get_last_post_redirect
-        response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
-    end
-end
-
-describe RequestController, "when reporting a request (logged in)" do
-    render_views
-
-    before do
-        @user = users(:robin_user)
-        session[:user_id] = @user.id
-    end
-
-    it "should 404 for non-existent requests" do
-      lambda {
-        post :report_request, :url_title => "hjksfdhjk_louytu_qqxxx"
-      }.should raise_error(ActiveRecord::RecordNotFound)
-    end
-
-    it "should mark a request as having been reported" do
-        ir = info_requests(:badger_request)
-        title = ir.url_title
-        get :show, :url_title => title
-        assigns[:info_request].attention_requested.should == false
-
-        post :report_request, :url_title => title
-        response.should redirect_to(:action => :show, :url_title => title)
-
-        get :show, :url_title => title
-        response.should be_success
-        assigns[:info_request].attention_requested.should == true
-        assigns[:info_request].described_state.should == "attention_requested"
-    end
-
-    it "should not allow a request to be reported twice" do
-        title = info_requests(:badger_request).url_title
-
-        post :report_request, :url_title => title
-        response.should redirect_to(:action => :show, :url_title => title)
-        get :show, :url_title => title
-        response.should be_success
-        response.body.should include("has been reported")
-
-        post :report_request, :url_title => title
-        response.should redirect_to(:action => :show, :url_title => title)
-        get :show, :url_title => title
-        response.should be_success
-        response.body.should include("has already been reported")
-    end
-
-    it "should let users know a request has been reported" do
-        title = info_requests(:badger_request).url_title
-        get :show, :url_title => title
-        response.body.should include("Offensive?")
-
-        post :report_request, :url_title => title
-        response.should redirect_to(:action => :show, :url_title => title)
-
-        get :show, :url_title => title
-        response.body.should_not include("Offensive?")
-        response.body.should include("This request has been reported")
-
-        info_requests(:badger_request).set_described_state("successful")
-        get :show, :url_title => title
-        response.body.should_not include("This request has been reported")
-        response.body.should =~ (/the site administrators.*have not hidden it/)
-    end
-
-    it "should send an email from the reporter to admins" do
-        ir = info_requests(:badger_request)
-        title = ir.url_title
-        post :report_request, :url_title => title
-        deliveries = ActionMailer::Base.deliveries
-        deliveries.size.should == 1
-        mail = deliveries[0]
-        mail.subject.should =~ /attention_requested/
-        mail.from.should include(@user.email)
-        mail.body.should include(@user.name)
-    end
-end
-
 describe RequestController, "when caching fragments" do
     it "should not fail with long filenames" do
         long_name = "blahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblahblah.txt"
@@ -2431,4 +2376,3 @@ describe RequestController, "when caching fragments" do
 
 end
 
-
diff --git a/spec/views/reports/new.erb_spec.rb b/spec/views/reports/new.erb_spec.rb
new file mode 100644
index 000000000..66b738261
--- /dev/null
+++ b/spec/views/reports/new.erb_spec.rb
@@ -0,0 +1,29 @@
+require File.expand_path(File.join('..', '..', '..', 'spec_helper'), __FILE__)
+
+describe 'reports/new.html.erb' do
+  let(:info_request) { mock_model(InfoRequest, :url_title => "foo", :report_reasons => ["Weird"]) }
+  before :each do
+    assign(:info_request, info_request)
+  end
+
+  it "should show a form" do
+    render
+    rendered.should have_selector("form")
+  end
+
+  context "request has already been reported" do
+    before :each do
+      info_request.stub!(:attention_requested).and_return(true)
+    end
+
+    it "should not show a form" do
+      render
+      rendered.should_not have_selector("form")
+    end
+
+    it "should say it's already been reported" do
+      render
+      rendered.should contain("This request has already been reported")
+    end
+  end
+end