From: Tom Hughes <tom@compton.nu>
Date: Sun, 22 May 2011 16:02:48 +0000 (+0100)
Subject: Tidy up the note controller
X-Git-Tag: live~5626^2~147
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/83821816359187fa2b877a29b7ab7d9f974c17b5?ds=sidebyside

Tidy up the note controller

Go through most of the note controller, tidying things up, fixing
a few bugs, and making sure we have tests for everything.
---

diff --git a/app/controllers/note_controller.rb b/app/controllers/note_controller.rb
index 486ce38d3..c46145045 100644
--- a/app/controllers/note_controller.rb
+++ b/app/controllers/note_controller.rb
@@ -5,7 +5,6 @@ class NoteController < ApplicationController
   before_filter :check_api_readable
   before_filter :authorize_web, :only => [:create, :close, :update, :delete, :mine]
   before_filter :check_api_writable, :only => [:create, :close, :update, :delete]
-  before_filter :require_moderator, :only => [:delete]
   before_filter :set_locale, :only => [:mine]
   after_filter :compress_output
   around_filter :api_call_handle_error, :api_call_timeout
@@ -13,57 +12,68 @@ class NoteController < ApplicationController
   # Help methods for checking boundary sanity and area size
   include MapBoundary
 
+  ##
+  # Return a list of notes in a given area
   def list
-    # Figure out the bbox
-    bbox = params['bbox']
+    # Figure out the bbox - we prefer a bbox argument but also
+    # support the old, deprecated, method with four arguments
+    if params[:bbox]
+      raise OSM::APIBadUserInput.new("Invalid bbox") unless params[:bbox].count(",") == 3
 
-    if bbox and bbox.count(',') == 3
-      bbox = bbox.split(',')
-      @min_lon, @min_lat, @max_lon, @max_lat = sanitise_boundaries(bbox)
+      bbox = params[:bbox].split(",")
     else
-      #Fallback to old style, this is deprecated and should not be used
-      raise OSM::APIBadUserInput.new("No l was given") unless params['l']
-      raise OSM::APIBadUserInput.new("No r was given") unless params['r']
-      raise OSM::APIBadUserInput.new("No b was given") unless params['b']
-      raise OSM::APIBadUserInput.new("No t was given") unless params['t']
-
-      @min_lon = params['l'].to_f
-      @max_lon = params['r'].to_f
-      @min_lat = params['b'].to_f
-      @max_lat = params['t'].to_f
+      raise OSM::APIBadUserInput.new("No l was given") unless params[:l]
+      raise OSM::APIBadUserInput.new("No r was given") unless params[:r]
+      raise OSM::APIBadUserInput.new("No b was given") unless params[:b]
+      raise OSM::APIBadUserInput.new("No t was given") unless params[:t]
+
+      bbox = [ params[:l], params[:b], params[:r], params[:t] ]
     end
 
-    limit = getLimit
-    conditions = closedCondition
-	
+    # Get the sanitised boundaries
+    @min_lon, @min_lat, @max_lon, @max_lat = sanitise_boundaries(bbox)
+
+    # Get any conditions that need to be applied
+    conditions = closed_condition
+
+    # Check that the boundaries are valid
     check_boundaries(@min_lon, @min_lat, @max_lon, @max_lat, MAX_NOTE_REQUEST_AREA)
 
-    @notes = Note.find_by_area(@min_lat, @min_lon, @max_lat, @max_lon, :include => :comments, :order => "updated_at DESC", :limit => limit, :conditions => conditions)
+    # Find the notes we want to return
+    @notes = Note.find_by_area(@min_lat, @min_lon, @max_lat, @max_lon,
+                               :include => :comments, 
+                               :conditions => conditions,
+                               :order => "updated_at DESC", 
+                               :limit => result_limit)
 
+    # Render the result
     respond_to do |format|
-      format.html {render :template => 'note/list.rjs', :content_type => "text/javascript"}
-      format.rss {render :template => 'note/list.rss'}
+      format.html { render :format => :rjs, :content_type => "text/javascript" }
+      format.rss
       format.js
-      format.xml {render :template => 'note/list.xml'}
-      format.json { render :json => @notes.to_json(:methods => [:lat, :lon], :only => [:id, :status, :created_at], :include => { :comments => { :only => [:author_name, :created_at, :body]}}) }	  
-      format.gpx {render :template => 'note/list.gpx'}
+      format.xml
+      format.json { render :json => @notes.to_json }
+      format.gpx
     end
   end
 
+  ##
+  # Create a new note
   def create
-    raise OSM::APIBadUserInput.new("No lat was given") unless params['lat']
-    raise OSM::APIBadUserInput.new("No lon was given") unless params['lon']
-    raise OSM::APIBadUserInput.new("No text was given") unless params['text']
-
-    lon = params['lon'].to_f
-    lat = params['lat'].to_f
-    comment = params['text']
-
-    name = "NoName"
-    name = params['name'] if params['name']
-
-    #Include in a transaction to ensure that there is always a note_comment for every note
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No lat was given") unless params[:lat]
+    raise OSM::APIBadUserInput.new("No lon was given") unless params[:lon]
+    raise OSM::APIBadUserInput.new("No text was given") unless params[:text]
+
+    # Extract the arguments
+    lon = params[:lon].to_f
+    lat = params[:lat].to_f
+    comment = params[:text]
+    name = params[:name]
+
+    # Include in a transaction to ensure that there is always a note_comment for every note
     Note.transaction do
+      # Create the note
       @note = Note.create(:lat => lat, :lon => lon)
       raise OSM::APIBadUserInput.new("The note is outside this world") unless @note.in_world?
 
@@ -81,116 +91,173 @@ class NoteController < ApplicationController
         @note.nearby_place = "unknown"
       end
 
+      # Save the note
       @note.save
 
+      # Add a comment to the note
       add_comment(@note, comment, name, "opened")
     end
- 
+
+    # Send an OK response
     render_ok
   end
 
+  ##
+  # Add a comment to an existing note
   def update
-    raise OSM::APIBadUserInput.new("No id was given") unless params['id']
-    raise OSM::APIBadUserInput.new("No text was given") unless params['text']
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No id was given") unless params[:id]
+    raise OSM::APIBadUserInput.new("No text was given") unless params[:text]
 
-    name = "NoName"
-    name = params['name'] if params['name']
-	
-    id = params['id'].to_i
+    # Extract the arguments
+    id = params[:id].to_i
+    comment = params[:text]
+    name = params[:name] or "NoName"
 
+    # Find the note and check it is valid
     note = Note.find(id)
     raise OSM::APINotFoundError unless note
     raise OSM::APIAlreadyDeletedError unless note.visible?
 
+    # Add a comment to the note
     Note.transaction do
-      add_comment(note, params['text'], name, "commented")
+      add_comment(note, comment, name, "commented")
     end
 
+    # Send an OK response
     render_ok
   end
 
+  ##
+  # Close a note
   def close
-    raise OSM::APIBadUserInput.new("No id was given") unless params['id']
-	
-    id = params['id'].to_i
-    name = "NoName"
-    name = params['name'] if params['name']
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No id was given") unless params[:id]
 
+    # Extract the arguments
+    id = params[:id].to_i
+    name = params[:name]
+
+    # Find the note and check it is valid
     note = Note.find_by_id(id)
     raise OSM::APINotFoundError unless note
     raise OSM::APIAlreadyDeletedError unless note.visible?
 
+    # Close the note and add a comment
     Note.transaction do
       note.close
-      add_comment(note, :nil, name, "closed")
+
+      add_comment(note, nil, name, "closed")
     end
 
+    # Send an OK response
     render_ok
   end 
 
+  ##
+  # Get a feed of recent notes and comments
   def rss
-    limit = getLimit
-    conditions = closedCondition
+    # Get any conditions that need to be applied
+    conditions = closed_condition
 
-    # Figure out the bbox
-    bbox = params['bbox']
+    # Process any bbox
+    if params[:bbox]
+      raise OSM::APIBadUserInput.new("Invalid bbox") unless params[:bbox].count(",") == 3
 
-    if bbox and bbox.count(',') == 3
-      bbox = bbox.split(',')
-      @min_lon, @min_lat, @max_lon, @max_lat = sanitise_boundaries(bbox)
+      @min_lon, @min_lat, @max_lon, @max_lat = sanitise_boundaries(params[:bbox].split(','))
 
       check_boundaries(@min_lon, @min_lat, @max_lon, @max_lat, MAX_NOTE_REQUEST_AREA)
 
-      conditions = cond_merge conditions, [OSM.sql_for_area(@min_lat, @min_lon, @max_lat, @max_lon)]
+      conditions = cond_merge conditions, [OSM.sql_for_area(@min_lat, @min_lon, @max_lat, @max_lon, "notes.")]
     end
 
-    @comments = NoteComment.find(:all, :limit => limit, :order => "created_at DESC", :joins => :note, :include => :note, :conditions => conditions)
-    render :template => 'note/rss.rss'
+    # Find the comments we want to return
+    @comments = NoteComment.find(:all, 
+                                 :conditions => conditions,
+                                 :order => "created_at DESC",
+                                 :limit => result_limit,
+                                 :joins => :note, 
+                                 :include => :note)
+
+    # Render the result
+    respond_to do |format|
+      format.rss
+    end
   end
 
+  ##
+  # Read a note
   def read
-    @note = Note.find(params['id'])
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No id was given") unless params[:id]
+
+    # Find the note and check it is valid
+    @note = Note.find(params[:id])
     raise OSM::APINotFoundError unless @note
     raise OSM::APIAlreadyDeletedError unless @note.visible?
     
+    # Render the result
     respond_to do |format|
-      format.rss
       format.xml
-      format.json { render :json => @note.to_json(:methods => [:lat, :lon], :only => [:id, :status, :created_at], :include => { :comments => { :only => [:author_name, :created_at, :body]}}) }	  
+      format.rss
+      format.json { render :json => @note.to_json }
       format.gpx
     end
   end
 
+  ##
+  # Delete (hide) a note
   def delete
-    note = note.find(params['id'])
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No id was given") unless params[:id]
+
+    # Extract the arguments
+    id = params[:id].to_i
+    name = params[:name]
+
+    # Find the note and check it is valid
+    note = Note.find(id)
     raise OSM::APINotFoundError unless note
     raise OSM::APIAlreadyDeletedError unless note.visible?
 
+    # Mark the note as hidden
     Note.transaction do
       note.status = "hidden"
       note.save
-      add_comment(note, :nil, name, "hidden")
+
+      add_comment(note, nil, name, "hidden")
     end
 
+    # Render the result
     render :text => "ok\n", :content_type => "text/html" 
   end
 
+  ##
+  # Return a list of notes matching a given string
   def search
-    raise OSM::APIBadUserInput.new("No query string was given") unless params['q']
-    limit = getLimit
-    conditions = closedCondition
-    conditions = cond_merge conditions, ['note_comments.body ~ ?', params['q']]
+    # Check the arguments are sane
+    raise OSM::APIBadUserInput.new("No query string was given") unless params[:q]
+
+    # Get any conditions that need to be applied
+    conditions = closed_condition
+    conditions = cond_merge conditions, ['note_comments.body ~ ?', params[:q]]
 	
-    #TODO: There should be a better way to do this.   CloseConditions are ignored at the moment
+    # Find the notes we want to return
+    @notes = Note.find(:all, 
+                       :conditions => conditions,
+                       :order => "updated_at DESC",
+                       :limit => result_limit,
+                       :joins => :comments,
+                       :include => :comments)
 
-    @notes = Note.find(:all, :limit => limit, :order => "updated_at DESC", :joins => :comments, :include => :comments, :conditions => conditions).uniq
+    # Render the result
     respond_to do |format|
-      format.html {render :template => 'note/list.rjs', :content_type => "text/javascript"}
-      format.rss {render :template => 'note/list.rss'}
+      format.html { render :action => :list, :format => :rjs, :content_type => "text/javascript"}
+      format.rss { render :action => :list }
       format.js
-      format.xml {render :template => 'note/list.xml'}
-      format.json { render :json => @notes.to_json(:methods => [:lat, :lon], :only => [:id, :status, :created_at], :include => { :comments => { :only => [:author_name, :created_at, :body]}}) }
-      format.gpx {render :template => 'note/list.gpx'}
+      format.xml { render :action => :list }
+      format.json { render :json => @notes.to_json }
+      format.gpx { render :action => :list }
     end
   end
 
@@ -252,32 +319,41 @@ private
     end 
   end 
 
+  ##
+  # Render an OK response
   def render_ok
-    output_js = :false
-    output_js = :true if params['format'] == "js"
-
-    if output_js == :true
+    if params[:format] == "js"
       render :text => "osbResponse();", :content_type => "text/javascript" 
     else
-      render :text => "ok " + @note.id.to_s + "\n", :content_type => "text/html" if @note
-      render :text => "ok\n", :content_type => "text/html" unless @note
+      render :text => "ok " + @note.id.to_s + "\n", :content_type => "text/plain" if @note
+      render :text => "ok\n", :content_type => "text/plain" unless @note
     end
   end
 
-  def getLimit
-    limit = 100
-    limit = params['limit'] if ((params['limit']) && (params['limit'].to_i < 10000) && (params['limit'].to_i > 0))
-    return limit
+  ##
+  # Get the maximum number of results to return
+  def result_limit
+    if params[:limit] and params[:limit].to_i > 0 and params[:limit].to_i < 10000
+      params[:limit].to_i
+    else
+      100
+    end
   end
 
-  def closedCondition
-    closed_since = 7 unless params['closed']
-    closed_since = params['closed'].to_i if params['closed']
+  ##
+  # Generate a condition to choose which bugs we want based
+  # on their status and the user's request parameters
+  def closed_condition
+    if params[:closed]
+      closed_since = params[:closed].to_i
+    else
+      closed_since = 7
+    end
 	
     if closed_since < 0
       conditions = ["status != 'hidden'"]
     elsif closed_since > 0
-      conditions = ["((status = 'open') OR ((status = 'closed' ) AND (closed_at > '" + (Time.now - closed_since.days).to_s + "')))"]
+      conditions = ["(status = 'open' OR (status = 'closed' AND closed_at > '#{Time.now - closed_since.days}'))"]
     else
       conditions = ["status = 'open'"]
     end
@@ -285,26 +361,26 @@ private
     return conditions
   end
 
-  def add_comment(note, text, name, event) 
-    comment = note.comments.create(:visible => true, :event => event)
-    comment.body = text unless text == :nil
+  ##
+  # Add a comment to a note
+  def add_comment(note, text, name, event)
+    name = "NoName" if name.nil?
+
+    attributes = { :visible => true, :event => event, :body => text }
+
     if @user  
-      comment.author_id = @user.id
-      comment.author_name = @user.display_name
+      attributes[:author_id] = @user.id
+      attributes[:author_name] = @user.display_name
     else  
-      comment.author_ip = request.remote_ip
-      comment.author_name = name + " (a)"
+      attributes[:author_ip] = request.remote_ip
+      attributes[:author_name] = name + " (a)"
     end
-    comment.save 
-    note.save
-
-    sent_to = Set.new
-    note.comments.each do | cmt |
-      if cmt.author
-        unless sent_to.include?(cmt.author)
-          Notifier.deliver_note_comment_notification(note_comment, cmt.author) unless cmt.author == @user
-          sent_to.add(cmt.author)
-        end
+
+    note.comments.create(attributes)
+
+    note.comments.map { |c| c.author }.uniq.each do |user|
+      if user and user != @user
+        Notifier.deliver_note_comment_notification(comment, user)
       end
     end
   end
diff --git a/app/models/note.rb b/app/models/note.rb
index 0512639e1..892ada1aa 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -4,7 +4,7 @@ class Note < ActiveRecord::Base
   has_many :comments, :class_name => "NoteComment",
                       :foreign_key => :note_id,
                       :order => :created_at,
-                      :conditions => "visible = true AND body IS NOT NULL"
+                      :conditions => { :visible => true }
 
   validates_presence_of :id, :on => :update
   validates_uniqueness_of :id
@@ -72,4 +72,17 @@ class Note < ActiveRecord::Base
   def author_name
     self.comments.first.author_name
   end
+
+  # Custom JSON output routine for notes
+  def to_json(options = {})
+    super options.reverse_merge(
+      :methods => [ :lat, :lon ], 
+      :only => [ :id, :status, :created_at ],
+      :include => {
+         :comments => {
+           :only => [ :event, :author_name, :created_at, :body ]
+         }
+      }
+    )
+  end
 end
diff --git a/test/functional/note_controller_test.rb b/test/functional/note_controller_test.rb
index 955f1c24c..2e4a01b3e 100644
--- a/test/functional/note_controller_test.rb
+++ b/test/functional/note_controller_test.rb
@@ -3,7 +3,7 @@ require File.dirname(__FILE__) + '/../test_helper'
 class NoteControllerTest < ActionController::TestCase
   fixtures :users, :notes, :note_comments
 
-  def test_map_note_create_success
+  def test_note_create_success
     assert_difference('Note.count') do
       assert_difference('NoteComment.count') do
         post :create, {:lat => -1.0, :lon => -1.0, :name => "new_tester", :text => "This is a comment"}
@@ -11,122 +11,304 @@ class NoteControllerTest < ActionController::TestCase
     end
     assert_response :success
     id = @response.body.sub(/ok/,"").to_i
+
     get :read, {:id => id, :format => 'json'}
     assert_response :success
-    js = @response.body
-    assert_match "\"status\":\"open\"", js
-    assert_match "\"body\":\"This is a comment\"", js
-    assert_match "\"author_name\":\"new_tester (a)\"", js
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal id, js["note"]["id"]
+    assert_equal "open", js["note"]["status"]
+    assert_equal "opened", js["note"]["comments"].last["event"]
+    assert_equal "This is a comment", js["note"]["comments"].last["body"]
+    assert_equal "new_tester (a)", js["note"]["comments"].last["author_name"]
   end
 
-  def test_map_note_comment_create_success
+  def test_note_create_fail
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lon => -1.0, :name => "new_tester", :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => -1.0, :name => "new_tester", :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => -1.0, :lon => -1.0, :name => "new_tester"}
+      end
+    end
+    assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => -100.0, :lon => -1.0, :name => "new_tester", :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => -1.0, :lon => -200.0, :name => "new_tester", :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
+  end
+
+  def test_note_comment_create_success
     assert_difference('NoteComment.count') do
-      post :update, {:id => 2, :name => "new_tester2", :text => "This is an additional comment"}
+      post :update, {:id => notes(:open_note_with_comment).id, :name => "new_tester2", :text => "This is an additional comment"}
     end
     assert_response :success
 
-    get :read, {:id => 2, :format => 'json'}
+    get :read, {:id => notes(:open_note_with_comment).id, :format => 'json'}
     assert_response :success
-    js = @response.body
-    assert_match "\"id\":2", js
-    assert_match "\"status\":\"open\"", js
-    assert_match "\"body\":\"This is an additional comment\"", js
-    assert_match "\"author_name\":\"new_tester2 (a)\"", js
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal notes(:open_note_with_comment).id, js["note"]["id"]
+    assert_equal "open", js["note"]["status"]
+    assert_equal "commented", js["note"]["comments"].last["event"]
+    assert_equal "This is an additional comment", js["note"]["comments"].last["body"]
+    assert_equal "new_tester2 (a)", js["note"]["comments"].last["author_name"]
   end
 
-  def test_map_note_read_success
-    get :read, {:id => 1}
+  def test_note_comment_create_fail
+    assert_no_difference('NoteComment.count') do
+      post :update, {:name => "new_tester2", :text => "This is an additional comment"}
+    end
+    assert_response :bad_request
+
+    assert_no_difference('NoteComment.count') do
+      post :update, {:id => notes(:open_note_with_comment).id, :name => "new_tester2"}
+    end
+    assert_response :bad_request
+
+    assert_no_difference('NoteComment.count') do
+      post :update, {:id => 12345, :name => "new_tester2", :text => "This is an additional comment"}
+    end
+    assert_response :not_found
+
+    assert_no_difference('NoteComment.count') do
+      post :update, {:id => notes(:hidden_note_with_comment).id, :name => "new_tester2", :text => "This is an additional comment"}
+    end
+    assert_response :gone
+  end
+
+  def test_note_close_success
+    post :close, {:id => notes(:open_note_with_comment).id}
+    assert_response :success
+
+    get :read, {:id => notes(:open_note_with_comment).id, :format => 'json'}
+    assert_response :success
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal notes(:open_note_with_comment).id, js["note"]["id"]
+    assert_equal "closed", js["note"]["status"]
+    assert_equal "closed", js["note"]["comments"].last["event"]
+    assert_equal "NoName (a)", js["note"]["comments"].last["author_name"]
+  end
+
+  def test_note_close_fail
+    post :close
+    assert_response :bad_request
+
+    post :close, {:id => 12345}
+    assert_response :not_found
+
+    post :close, {:id => notes(:hidden_note_with_comment).id}
+    assert_response :gone
+  end
+
+  def test_note_read_success
+    get :read, {:id => notes(:open_note).id}
     assert_response :success      
+    assert_equal "application/xml", @response.content_type
 
-    get :read, {:id => 1,  :format => 'xml'}
+    get :read, {:id => notes(:open_note).id, :format => "xml"}
     assert_response :success
+    assert_equal "application/xml", @response.content_type
 
-    get :read, {:id => 1,  :format => 'rss'}
+    get :read, {:id => notes(:open_note).id, :format => "rss"}
     assert_response :success
+    assert_equal "application/rss+xml", @response.content_type
 
-    get :read, {:id => 1,  :format => 'json'}
+    get :read, {:id => notes(:open_note).id, :format => "json"}
     assert_response :success
+    assert_equal "application/json", @response.content_type
 
-    get :read, {:id => 1,  :format => 'gpx'}
+    get :read, {:id => notes(:open_note).id, :format => "gpx"}
     assert_response :success
+    assert_equal "application/gpx+xml", @response.content_type
   end
 
-  def test_map_note_close_success
-    post :close, {:id => 2}
+  def test_note_read_hidden_comment
+    get :read, {:id => notes(:note_with_hidden_comment).id, :format => 'json'}
     assert_response :success
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal notes(:note_with_hidden_comment).id, js["note"]["id"]
+    assert_equal 2, js["note"]["comments"].count
+    assert_equal "Valid comment for note 5", js["note"]["comments"][0]["body"]
+    assert_equal "Another valid comment for note 5", js["note"]["comments"][1]["body"]
+  end
 
-    get :read, {:id => 2, :format => 'json'}
-    js = @response.body
-    assert_match "\"id\":2", js
-    assert_match "\"status\":\"closed\"", js
+  def test_note_read_fail
+    post :read
+    assert_response :bad_request
+
+    get :read, {:id => 12345}
+    assert_response :not_found
+
+    get :read, {:id => notes(:hidden_note_with_comment).id}
+    assert_response :gone
+  end
+
+  def test_note_delete_success
+    delete :delete, {:id => notes(:open_note_with_comment).id}
+    assert_response :success
+
+    get :read, {:id => notes(:open_note_with_comment).id, :format => 'json'}
+    assert_response :gone
+  end
+
+  def test_note_delete_fail
+    delete :delete
+    assert_response :bad_request
+
+    delete :delete, {:id => 12345}
+    assert_response :not_found
+
+    delete :delete, {:id => notes(:hidden_note_with_comment).id}
+    assert_response :gone
   end
 
   def test_get_notes_success
-    get :list, {:bbox=>'1,1,1.2,1.2'}
+    get :list, {:bbox => '1,1,1.2,1.2'}
     assert_response :success
+    assert_equal "text/javascript", @response.content_type
 
-    get :list, {:bbox=>'1,1,1.2,1.2', :format => 'rss'}
+    get :list, {:bbox => '1,1,1.2,1.2', :format => 'rss'}
     assert_response :success
+    assert_equal "application/rss+xml", @response.content_type
 
-    get :list, {:bbox=>'1,1,1.2,1.2', :format => 'json'}
+    get :list, {:bbox => '1,1,1.2,1.2', :format => 'json'}
     assert_response :success
+    assert_equal "application/json", @response.content_type
 
-    get :list, {:bbox=>'1,1,1.2,1.2', :format => 'xml'}
+    get :list, {:bbox => '1,1,1.2,1.2', :format => 'xml'}
     assert_response :success
+    assert_equal "application/xml", @response.content_type
 
-    get :list, {:bbox=>'1,1,1.2,1.2', :format => 'gpx'}
+    get :list, {:bbox => '1,1,1.2,1.2', :format => 'gpx'}
     assert_response :success
+    assert_equal "application/gpx+xml", @response.content_type
   end
 
-  def test_get_notes_large_area_success
-    get :list, {:bbox=>'-2.5,-2.5,2.5,2.5'}
+  def test_get_notes_large_area
+    get :list, {:bbox => '-2.5,-2.5,2.5,2.5'}
     assert_response :success
-  end
 
-  def test_get_notes_large_area_bad_request
-    get :list, {:bbox=>'-10,-10,12,12'}
+    get :list, {:l => '-2.5', :b => '-2.5', :r => '2.5', :t => '2.5'}
+    assert_response :success
+
+    get :list, {:bbox => '-10,-10,12,12'}
+    assert_response :bad_request
+
+    get :list, {:l => '-10', :b => '-10', :r => '12', :t => '12'}
     assert_response :bad_request
   end
 
-  def test_get_notes_closed_7_success
-    get :list, {:bbox=>'1,1,1.2,1.2', :closed => '7'}
+  def test_get_notes_closed
+    get :list, {:bbox=>'1,1,1.7,1.7', :closed => '7', :format => 'json'}
     assert_response :success
-  end
+    assert_equal "application/json", @response.content_type
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal 4, js.count
 
-  def test_get_notes_closed_0_success
-    get :list, {:bbox=>'1,1,1.2,1.2', :closed => '0'}
+    get :list, {:bbox=>'1,1,1.7,1.7', :closed => '0', :format => 'json'}
     assert_response :success
-  end
+    assert_equal "application/json", @response.content_type
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal 4, js.count
 
-  def test_get_notes_closed_n1_success
-    get :list, {:bbox=>'1,1,1.2,1.2', :closed => '-1'}
+    get :list, {:bbox=>'1,1,1.7,1.7', :closed => '-1', :format => 'json'}
     assert_response :success
+    assert_equal "application/json", @response.content_type
+    js = ActiveSupport::JSON.decode(@response.body)
+    assert_not_nil js
+    assert_equal 6, js.count
   end
 
+  def test_get_notes_bad_params
+    get :list, {:bbox => '-2.5,-2.5,2.5'}
+    assert_response :bad_request
+
+    get :list, {:bbox => '-2.5,-2.5,2.5,2.5,2.5'}
+    assert_response :bad_request
+
+    get :list, {:b => '-2.5', :r => '2.5', :t => '2.5'}
+    assert_response :bad_request
+
+    get :list, {:l => '-2.5', :r => '2.5', :t => '2.5'}
+    assert_response :bad_request
+
+    get :list, {:l => '-2.5', :b => '-2.5', :t => '2.5'}
+    assert_response :bad_request
+
+    get :list, {:l => '-2.5', :b => '-2.5', :r => '2.5'}
+    assert_response :bad_request
+  end
 
   def test_search_success
-    get :search, {:bbox=>'1,1,1.2,1.2', :q => 'note 1'}
+    get :search, {:q => 'note 1'}
     assert_response :success
+    assert_equal "text/javascript", @response.content_type
 
-    get :search, {:bbox=>'1,1,1.2,1.2', :q => 'note 1', :format => 'xml'}
+    get :search, {:q => 'note 1', :format => 'xml'}
     assert_response :success
+    assert_equal "application/xml", @response.content_type
 
-    get :search, {:bbox=>'1,1,1.2,1.2', :q => 'note 1', :format => 'json'}
+    get :search, {:q => 'note 1', :format => 'json'}
     assert_response :success
+    assert_equal "application/json", @response.content_type
 
-    get :search, {:bbox=>'1,1,1.2,1.2', :q => 'note 1', :format => 'rss'}
+    get :search, {:q => 'note 1', :format => 'rss'}
     assert_response :success
+    assert_equal "application/rss+xml", @response.content_type
 
-    get :search, {:bbox=>'1,1,1.2,1.2', :q => 'note 1', :format => 'gpx'}
+    get :search, {:q => 'note 1', :format => 'gpx'}
     assert_response :success
+    assert_equal "application/gpx+xml", @response.content_type
+  end
+
+  def test_search_bad_params
+    get :search
+    assert_response :bad_request
   end
 
   def test_rss_success
-    get :rss, {:bbox=>'1,1,1.2,1.2'}
-    assert_response :success
-	
     get :rss
     assert_response :success
+    assert_equal "application/rss+xml", @response.content_type
+
+    get :rss, {:bbox=>'1,1,1.2,1.2'}
+    assert_response :success	
+    assert_equal "application/rss+xml", @response.content_type
+  end
+
+  def test_rss_fail
+    get :rss, {:bbox=>'1,1,1.2'}
+    assert_response :bad_request
+
+    get :rss, {:bbox=>'1,1,1.2,1.2,1.2'}
+    assert_response :bad_request
   end
 
   def test_user_notes_success
@@ -139,36 +321,4 @@ class NoteControllerTest < ActionController::TestCase
     get :mine, {:display_name=>'non-existent'}
     assert_response :not_found	
   end
-
-  def test_map_note_comment_create_not_found
-    assert_no_difference('NoteComment.count') do
-      post :update, {:id => 12345, :name => "new_tester", :text => "This is an additional comment"}
-    end
-    assert_response :not_found
-  end
-
-  def test_map_note_close_not_found
-    post :close, {:id => 12345}
-    assert_response :not_found
-  end
-
-  def test_map_note_read_not_found
-    get :read, {:id => 12345}
-    assert_response :not_found
-  end
-
-  def test_map_note_read_gone
-    get :read, {:id => 4}
-    assert_response :gone
-  end
-
-  def test_map_note_hidden_comment
-    get :read, {:id => 5, :format => 'json'}
-    assert_response :success
-    js = @response.body
-    assert_match "\"id\":5", js
-    assert_match "\"body\":\"Valid comment for note 5\"", js
-    assert_match "\"body\":\"Another valid comment for note 5\"", js
-    assert_no_match /\"body\":\"Spam for note 5\"/, js
-  end
 end