From: Tom Hughes <tom@compton.nu>
Date: Tue, 5 Feb 2013 18:08:42 +0000 (+0000)
Subject: Make the API reject changes to closed notes
X-Git-Tag: live~5117^2~24
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/ae27f7adbe561b20203fa639ac1aa50d0408edc0

Make the API reject changes to closed notes
---

diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index a7fa03ac7..0544f8705 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -94,6 +94,7 @@ class NotesController < ApplicationController
     @note = Note.find(id)
     raise OSM::APINotFoundError unless @note
     raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible?
+    raise OSM::APINoteAlreadyClosedError.new(@note) if @note.closed?
 
     # Add a comment to the note
     Note.transaction do
@@ -121,6 +122,7 @@ class NotesController < ApplicationController
     @note = Note.find_by_id(id)
     raise OSM::APINotFoundError unless @note
     raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible?
+    raise OSM::APINoteAlreadyClosedError.new(@note) if @note.closed?
 
     # Close the note and add a comment
     Note.transaction do
diff --git a/app/models/note.rb b/app/models/note.rb
index 801e1f3cb..bb56c5ce0 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -52,6 +52,11 @@ class Note < ActiveRecord::Base
     status != "hidden"
   end
 
+  # Check if a note is closed
+  def closed?
+    not closed_at.nil?
+  end
+
   # Return the author object, derived from the first comment
   def author
     self.comments.first.author
diff --git a/lib/osm.rb b/lib/osm.rb
index 3da9671c1..393011dac 100644
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -281,6 +281,23 @@ module OSM
     end
   end
 
+  # Raised when the note provided is already closed
+  class APINoteAlreadyClosedError < APIError
+    def initialize(note)
+      @note = note
+    end
+
+    attr_reader :note
+
+    def status
+      :conflict
+    end
+
+    def to_s
+      "The note #{@note.id} was closed at #{@note.closed_at}"
+    end
+  end
+
   # Helper methods for going to/from mercator and lat/lng.
   class Mercator
     include Math
diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb
index 1c0ded355..6e6a3c17c 100644
--- a/test/functional/notes_controller_test.rb
+++ b/test/functional/notes_controller_test.rb
@@ -236,6 +236,11 @@ class NotesControllerTest < ActionController::TestCase
       post :comment, {:id => notes(:hidden_note_with_comment).id, :text => "This is an additional comment"}
     end
     assert_response :gone
+
+    assert_no_difference('NoteComment.count') do
+      post :comment, {:id => notes(:closed_note_with_comment).id, :text => "This is an additional comment"}
+    end
+    assert_response :conflict
   end
 
   def test_note_close_success
@@ -273,6 +278,9 @@ class NotesControllerTest < ActionController::TestCase
 
     post :close, {:id => notes(:hidden_note_with_comment).id}
     assert_response :gone
+
+    post :close, {:id => notes(:closed_note_with_comment).id}
+    assert_response :conflict
   end
 
   def test_note_read_success