Have a link to the annotation form, rather than the form being on the request.

(Partly to reduce clutter, partly to not overemphasise annotation vs.
sending followups. Not sure if this is right, but will try it a bit.)

5 files changed, 72 insertions, 65 deletions

diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb
index 8ac6d3850..ff49b2a79 100644
--- a/app/controllers/comment_controller.rb
+++ b/app/controllers/comment_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2008 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: comment_controller.rb,v 1.5 2008-08-29 12:03:40 francis Exp $
+# $Id: comment_controller.rb,v 1.6 2008-09-02 14:57:31 francis Exp $
 
 class CommentController < ApplicationController
     
@@ -12,19 +12,26 @@ class CommentController < ApplicationController
         if params[:type] == 'request'
             @info_request = InfoRequest.find_by_url_title(params[:url_title])
             @track_thing = TrackThing.create_track_for_request(@info_request)
-            @comment = Comment.new(params[:comment].merge({
-                :comment_type => 'request', 
-                :user => @user
-            }))
+            if params[:comment]
+                @comment = Comment.new(params[:comment].merge({
+                    :comment_type => 'request', 
+                    :user => @user
+                }))
+            end
         else
             raise "Unknown type " + params[:type]
         end
 
-        # XXX this check should theoretically be a validation rule in the model
-        @existing_comment = Comment.find_by_existing_comment(@info_request.id, params[:comment][:body])
+        if params[:comment]
+            # XXX this check should theoretically be a validation rule in the model
+            @existing_comment = Comment.find_by_existing_comment(@info_request.id, params[:comment][:body])
+        else
+            # Default to subscribing to request when first viewing form
+            params[:subscribe_to_request] = true
+        end
         
         # See if values were valid or not
-        if !@existing_comment.nil? || !@comment.valid? || params[:reedit]
+        if !params[:comment] || !@existing_comment.nil? || !@comment.valid? || params[:reedit]
             render :action => 'new'
             return
         end
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 19a602571..ac025b94a 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: request_controller.rb,v 1.102 2008-08-31 23:43:52 francis Exp $
+# $Id: request_controller.rb,v 1.103 2008-09-02 14:57:31 francis Exp $
 
 class RequestController < ApplicationController
     
@@ -39,9 +39,6 @@ class RequestController < ApplicationController
         # Track corresponding to this page
         @track_thing = TrackThing.create_track_for_request(@info_request)
         @feed_autodetect = [ { :url => do_track_url(@track_thing, 'feed'), :title => @track_thing.params[:title_in_rss] } ]
-
-        # Default to subscribing to requests
-        params[:subscribe_to_request] = true
     end
 
     # Requests similar to this one
diff --git a/app/views/comment/new.rhtml b/app/views/comment/new.rhtml
index 3109af360..056d5f7ba 100644
--- a/app/views/comment/new.rhtml
+++ b/app/views/comment/new.rhtml
@@ -11,6 +11,58 @@
 
 <%= foi_error_messages_for :comment %>
 
-<h1>Add an annotation</h1>
+<h1>Add an annotation to '<%=request_link(@info_request)%>'</h1>
+
+
+<% if true %>
+        <p>
+        Annotations will be posted publically here, and are
+        <strong>not</strong> sent to the authority.
+        </p>
+
+        <p>
+        Help the requester with their request.
+        </p>
+
+        <ul>
+
+        <% if [ 'waiting_clarification' ].include?(@info_request.described_state) %>
+            <li> Advise on how to <strong>best clarify</strong> the request.</li>
+        <% end %>
+
+        <% if @info_request.awaiting_description %>
+        <% end %>
+
+        <% if not [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
+            <li> Link to the information requested, if it is <strong>already available</strong> on the Internet. </li>
+            <li> Suggest <strong>where else</strong> the requester might find the information. </li>
+            <li> Offer better ways of <strong>wording the request</strong> to get the information. </li>
+        <% end %>
+
+        <% if [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
+            <li> <strong>Summarise</strong> the content of any information returned. </li>
+            <li> Say how you've <strong>used the information</strong>, with links if possible. </li>
+            <li> <strong>Thank</strong> the public authority or <%=h @info_request.user.name %>. </li>
+        <% end %>
+        <% if [ 'partially_successful' ].include?(@info_request.described_state) %>
+            <li> Suggest how the requester can find the <strong>rest of the information</strong>. </li>
+        <% end %>
+        <% if [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
+            <li> Point to <strong>related information</strong>, campaigns or forums which may be useful. </li>
+        <% end %>
+
+        <% if [ 'not_held' ].include?(@info_request.described_state) %>
+            <li> Ideas on what <strong>other documents to request</strong> which the authority may hold. </li>
+        <% end %>
+        <% if [ 'rejected' ].include?(@info_request.described_state) %>
+            <li> Advise on whether the <strong>rejection is legal</strong>, and how to complain about if not. </li>
+        <% end %>
+
+        <% if [ 'requires_admin' ].include?(@info_request.described_state) %>
+            <li> Your thoughts on what the WhatDoTheyKnow <strong>administrators</strong> should do about the request. </li>
+        <% end %>
+
+        </ul>
+<% end %>
 
 <%= render :partial => 'comment/comment_form', :locals => { :track_thing => @track_thing } %>
diff --git a/app/views/request/show.rhtml b/app/views/request/show.rhtml
index 87f9e3757..2e0d79e18 100644
--- a/app/views/request/show.rhtml
+++ b/app/views/request/show.rhtml
@@ -108,58 +108,8 @@
     <% end %>
 
     <div id="add_annotation">
-        <h2>Add an annotation</h2>
-
-        <p>
-        Annotations will be posted publically here, and are
-        <strong>not</strong> sent to the authority.
-        </p>
-
-        <p>
-        Help the requester with their request.
-        </p>
-
-        <ul>
-
-        <% if [ 'waiting_clarification' ].include?(@info_request.described_state) %>
-            <li> Advise on how to <strong>best clarify</strong> the request.</li>
-        <% end %>
-
-        <% if @info_request.awaiting_description %>
-        <% end %>
-
-        <% if not [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
-            <li> Link to the information requested, if it is <strong>already available</strong> on the Internet. </li>
-            <li> Suggest <strong>where else</strong> the requester might find the information. </li>
-            <li> Offer better ways of <strong>wording the request</strong> to get the information. </li>
-        <% end %>
-
-        <% if [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
-            <li> <strong>Summarise</strong> the content of any information returned. </li>
-            <li> Say how you've <strong>used the information</strong>, with links if possible. </li>
-            <li> <strong>Thank</strong> the public authority or <%=h @info_request.user.name %>. </li>
-        <% end %>
-        <% if [ 'partially_successful' ].include?(@info_request.described_state) %>
-            <li> Suggest how the requester can find the <strong>rest of the information</strong>. </li>
-        <% end %>
-        <% if [ 'successful', 'partially_successful' ].include?(@info_request.described_state) %>
-            <li> Point to <strong>related information</strong>, campaigns or forums which may be useful. </li>
-        <% end %>
-
-        <% if [ 'not_held' ].include?(@info_request.described_state) %>
-            <li> Ideas on what <strong>other documents to request</strong> which the authority may hold. </li>
-        <% end %>
-        <% if [ 'rejected' ].include?(@info_request.described_state) %>
-            <li> Advise on whether the <strong>rejection is legal</strong>, and how to complain about if not. </li>
-        <% end %>
-
-        <% if [ 'requires_admin' ].include?(@info_request.described_state) %>
-            <li> Your thoughts on what the WhatDoTheyKnow <strong>administrators</strong> should do about the request. </li>
-        <% end %>
-
-        </ul>
-
-        <%= render :partial => 'comment/comment_form', :locals => { :track_thing => @track_thing } %>
+        <%= link_to "Add an annotation", new_comment_url(:url_title => @info_request.url_title) %> (to help
+        the requester or others with this request)
     </div>
 </div>
 
diff --git a/todo.txt b/todo.txt
index e7447f02d..2abf33421 100644
--- a/todo.txt
+++ b/todo.txt
@@ -191,6 +191,7 @@ Consider removing login links from notifications of new responses, now we have r
  thanks :) I won't do it now as there are more important things, I was just accidentally impressed
 
 Properly escape all name_and_email functions
+    call our new TMail.Address.encode_quoted_string monkeypatch function
 
 Including 'Request for information' in the subject line of your e-mail will assist us in prioritising your request.
 http://www.parliament.uk/parliamentary_publications_and_archives/freedom_of_information_in_the_house_of_lords/lords__foi___how_to_obtain_information.cfm