From: Tom Hughes <tom@compton.nu>
Date: Wed, 28 Aug 2019 16:23:10 +0000 (+0100)
Subject: Merge remote-tracking branch 'upstream/pull/1926'
X-Git-Tag: live~2967
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/783b5e3729228908d7404ae7404af1023501a906?hp=a7092491b069de43bb8d8c30d3526e7095c5cc98

Merge remote-tracking branch 'upstream/pull/1926'
---

diff --git a/app/controllers/api/notes_controller.rb b/app/controllers/api/notes_controller.rb
index 20a24ce99..fc9167eb3 100644
--- a/app/controllers/api/notes_controller.rb
+++ b/app/controllers/api/notes_controller.rb
@@ -4,7 +4,7 @@ module Api
 
     before_action :check_api_readable
     before_action :setup_user_auth, :only => [:create, :comment, :show]
-    before_action :authorize, :only => [:close, :reopen, :destroy]
+    before_action :authorize, :only => [:close, :reopen, :destroy, :comment]
 
     authorize_resource
 
diff --git a/app/views/browse/note.html.erb b/app/views/browse/note.html.erb
index c7989d789..f68dfbe2e 100644
--- a/app/views/browse/note.html.erb
+++ b/app/views/browse/note.html.erb
@@ -41,18 +41,18 @@
   <% end %>
 
   <% if @note.status == "open" %>
-    <form action="#">
-      <textarea class="comment" name="text" cols="40" rows="5" maxlength="2000"></textarea>
-      <div class="buttons clearfix">
-        <% if current_user and current_user.moderator? -%>
-          <input type="submit" name="hide" value="<%= t("javascripts.notes.show.hide") %>" class="deemphasize" data-note-id="<%= @note.id %>" data-method="DELETE" data-url="<%= note_url(@note, "json") %>">
-        <% end -%>
-        <% if current_user -%>
+    <% if current_user -%>
+      <form action="#">
+        <textarea class="comment" name="text" cols="40" rows="5" maxlength="2000"></textarea>
+        <div class="buttons clearfix">
+          <% if current_user.moderator? -%>
+            <input type="submit" name="hide" value="<%= t("javascripts.notes.show.hide") %>" class="deemphasize" data-note-id="<%= @note.id %>" data-method="DELETE" data-url="<%= note_url(@note, "json") %>">
+          <% end -%>
           <input type="submit" name="close" value="<%= t("javascripts.notes.show.resolve") %>" data-note-id="<%= @note.id %>" data-method="POST" data-url="<%= close_note_url(@note, "json") %>">
-        <% end -%>
-        <input type="submit" name="comment" value="<%= t("javascripts.notes.show.comment") %>" data-note-id="<%= @note.id %>" data-method="POST" data-url="<%= comment_note_url(@note, "json") %>" disabled="1">
-      </div>
-    </form>
+          <input type="submit" name="comment" value="<%= t("javascripts.notes.show.comment") %>" data-note-id="<%= @note.id %>" data-method="POST" data-url="<%= comment_note_url(@note, "json") %>" disabled="1">
+        </div>
+      </form>
+    <% end -%>
   <% else %>
     <form action="#">
       <input type="hidden" name="text" value="">
diff --git a/test/controllers/api/notes_controller_test.rb b/test/controllers/api/notes_controller_test.rb
index 1254c4fb5..5f449f0d6 100644
--- a/test/controllers/api/notes_controller_test.rb
+++ b/test/controllers/api/notes_controller_test.rb
@@ -222,6 +222,8 @@ module Api
 
     def test_comment_success
       open_note_with_comment = create(:note_with_comments)
+      user = create(:user)
+      basic_authorization user.email, "test"
       assert_difference "NoteComment.count", 1 do
         assert_no_difference "ActionMailer::Base.deliveries.size" do
           perform_enqueued_jobs do
@@ -238,7 +240,7 @@ module Api
       assert_equal 2, js["properties"]["comments"].count
       assert_equal "commented", js["properties"]["comments"].last["action"]
       assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
-      assert_nil js["properties"]["comments"].last["user"]
+      assert_equal user.display_name, js["properties"]["comments"].last["user"]
 
       get :show, :params => { :id => open_note_with_comment.id, :format => "json" }
       assert_response :success
@@ -250,7 +252,7 @@ module Api
       assert_equal 2, js["properties"]["comments"].count
       assert_equal "commented", js["properties"]["comments"].last["action"]
       assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
-      assert_nil js["properties"]["comments"].last["user"]
+      assert_equal user.display_name, js["properties"]["comments"].last["user"]
 
       # Ensure that emails are sent to users
       first_user = create(:user)
@@ -261,47 +263,6 @@ module Api
         create(:note_comment, :note => note, :author => first_user)
         create(:note_comment, :note => note, :author => second_user)
       end
-      assert_difference "NoteComment.count", 1 do
-        assert_difference "ActionMailer::Base.deliveries.size", 2 do
-          perform_enqueued_jobs do
-            post :comment, :params => { :id => note_with_comments_by_users.id, :text => "This is an additional comment", :format => "json" }
-          end
-        end
-      end
-      assert_response :success
-      js = ActiveSupport::JSON.decode(@response.body)
-      assert_not_nil js
-      assert_equal "Feature", js["type"]
-      assert_equal note_with_comments_by_users.id, js["properties"]["id"]
-      assert_equal "open", js["properties"]["status"]
-      assert_equal 3, js["properties"]["comments"].count
-      assert_equal "commented", js["properties"]["comments"].last["action"]
-      assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
-      assert_nil js["properties"]["comments"].last["user"]
-
-      email = ActionMailer::Base.deliveries.find { |e| e.to.first == first_user.email }
-      assert_not_nil email
-      assert_equal 1, email.to.length
-      assert_equal "[OpenStreetMap] An anonymous user has commented on one of your notes", email.subject
-
-      email = ActionMailer::Base.deliveries.find { |e| e.to.first == second_user.email }
-      assert_not_nil email
-      assert_equal 1, email.to.length
-      assert_equal "[OpenStreetMap] An anonymous user has commented on a note you are interested in", email.subject
-
-      get :show, :params => { :id => note_with_comments_by_users.id, :format => "json" }
-      assert_response :success
-      js = ActiveSupport::JSON.decode(@response.body)
-      assert_not_nil js
-      assert_equal "Feature", js["type"]
-      assert_equal note_with_comments_by_users.id, js["properties"]["id"]
-      assert_equal "open", js["properties"]["status"]
-      assert_equal 3, js["properties"]["comments"].count
-      assert_equal "commented", js["properties"]["comments"].last["action"]
-      assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
-      assert_nil js["properties"]["comments"].last["user"]
-
-      ActionMailer::Base.deliveries.clear
 
       basic_authorization third_user.email, "test"
 
@@ -318,7 +279,7 @@ module Api
       assert_equal "Feature", js["type"]
       assert_equal note_with_comments_by_users.id, js["properties"]["id"]
       assert_equal "open", js["properties"]["status"]
-      assert_equal 4, js["properties"]["comments"].count
+      assert_equal 3, js["properties"]["comments"].count
       assert_equal "commented", js["properties"]["comments"].last["action"]
       assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
       assert_equal third_user.display_name, js["properties"]["comments"].last["user"]
@@ -341,7 +302,7 @@ module Api
       assert_equal "Feature", js["type"]
       assert_equal note_with_comments_by_users.id, js["properties"]["id"]
       assert_equal "open", js["properties"]["status"]
-      assert_equal 4, js["properties"]["comments"].count
+      assert_equal 3, js["properties"]["comments"].count
       assert_equal "commented", js["properties"]["comments"].last["action"]
       assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
       assert_equal third_user.display_name, js["properties"]["comments"].last["user"]
@@ -352,6 +313,15 @@ module Api
     def test_comment_fail
       open_note_with_comment = create(:note_with_comments)
 
+      user = create(:user)
+
+      assert_no_difference "NoteComment.count" do
+        post :comment, :params => { :text => "This is an additional comment" }
+        assert_response :unauthorized
+      end
+
+      basic_authorization user.email, "test"
+
       assert_no_difference "NoteComment.count" do
         post :comment, :params => { :text => "This is an additional comment" }
       end