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

app/assets/javascripts/index/changeset.js

@@ -26,14 +26,14 @@ OSM.Changeset = function (map) {
     });
   }
 
-  function updateChangeset(form, method, url, include_data) {
+  function updateChangeset(method, url, include_data) {
     var data;
 
-    $(form).find("#comment-error").prop("hidden", true);
+    content.find("#comment-error").prop("hidden", true);
-    $(form).find("input[type=submit]").prop("disabled", true);
+    content.find("button[data-method][data-url]").prop("disabled", true);
 
     if (include_data) {
-      data = { text: $(form.text).val() };
+      data = { text: content.find("textarea").val() };
     } else {
       data = {};
     }
@@ -47,24 +47,21 @@ OSM.Changeset = function (map) {
         OSM.loadSidebarContent(window.location.pathname, page.load);
       },
       error: function (xhr) {
-        $(form).find("#comment-error").text(xhr.responseText);
+        content.find("button[data-method][data-url]").prop("disabled", false);
-        $(form).find("#comment-error").prop("hidden", false);
+        content.find("#comment-error")
-        $(form).find("input[type=submit]").prop("disabled", false);
+          .text(xhr.responseText)
+          .prop("hidden", false)
+          .get(0).scrollIntoView({ block: "nearest" });
       }
     });
   }
 
   function initialize() {
-    content.find("input[name=comment]").on("click", function (e) {
+    content.find("button[data-method][data-url]").on("click", function (e) {
       e.preventDefault();
       var data = $(e.target).data();
-      updateChangeset(e.target.form, data.method, data.url, true);
+      var include_data = e.target.name === "comment";
-    });
+      updateChangeset(data.method, data.url, include_data);
-
-    content.find(".action-button").on("click", function (e) {
-      e.preventDefault();
-      var data = $(e.target).data();
-      updateChangeset(e.target.form, data.method, data.url);
     });
 
     content.find("textarea").on("input", function (e) {

app/assets/stylesheets/common.scss

@@ -647,11 +647,6 @@ tr.turn:hover {
     }
   }
 
-  span.action-button:hover {
-    cursor: pointer;
-    text-decoration: underline;
-  }
-
   .note-description {
     overflow: hidden;
     margin: 0 0 10px 10px;

app/views/browse/changeset.html.erb

@@ -18,54 +18,41 @@
     <% if current_user %>
       <div class="col-auto">
         <% if @changeset.subscribers.exists?(current_user.id) %>
-          <button class="action-button btn btn-sm btn-primary" name="unsubscribe" data-method="POST" data-url="<%= changeset_unsubscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.unsubscribe") %></button>
+          <button class="btn btn-sm btn-primary" name="unsubscribe" data-method="POST" data-url="<%= changeset_unsubscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.unsubscribe") %></button>
         <% else %>
-          <button class="action-button btn btn-sm btn-primary" name="subscribe" data-method="POST" data-url="<%= changeset_subscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.subscribe") %></button>
+          <button class="btn btn-sm btn-primary" name="subscribe" data-method="POST" data-url="<%= changeset_subscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.subscribe") %></button>
         <% end %>
       </div>
     <% end %>
   </div>
 
   <% if @comments.length > 0 %>
-    <div class='changeset-comments'>
+    <ul class="list-unstyled">
-      <form action="#">
+      <% @comments.each do |comment| %>
-        <ul class="list-unstyled">
+        <% next unless comment.visible || current_user&.moderator? %>
-          <% @comments.each do |comment| %>
+        <li id="c<%= comment.id %>">
-            <% if comment.visible %>
+          <small class='text-muted'>
-              <li id="c<%= comment.id %>">
+            <%= t comment.visible ? ".comment_by_html" : ".hidden_comment_by_html",
-                <small class='text-muted'>
+                  :time_ago => friendly_date_ago(comment.created_at),
-                  <%= t(".comment_by_html",
+                  :user => link_to(comment.author.display_name, user_path(comment.author)) %>
-                        :time_ago => friendly_date_ago(comment.created_at),
+            <% if current_user&.moderator? %>
-                        :user => link_to(comment.author.display_name, user_path(comment.author))) %>
+              —
-                  <% if current_user and current_user.moderator? %>
+              <%= tag.button t("javascripts.changesets.show.#{comment.visible ? 'hide' : 'unhide'}_comment"),
-                    — <span class="action-button" data-comment-id="<%= comment.id %>" data-method="POST" data-url="<%= changeset_comment_hide_url(comment.id) %>"><%= t("javascripts.changesets.show.hide_comment") %></span>
+                             :class => "btn btn-sm small btn-link link-secondary p-0 align-baseline",
-                  <% end %>
+                             :data => { :method => "POST",
-                </small>
+                                        :url => comment.visible ? changeset_comment_hide_url(comment) : changeset_comment_unhide_url(comment) } %>
-                <div class="mx-2">
-                  <%= comment.body.to_html %>
-                </div>
-              </li>
-            <% elsif current_user and current_user.moderator? %>
-              <li id="c<%= comment.id %>">
-                <small class='text-muted'>
-                  <%= t(".hidden_comment_by_html",
-                        :time_ago => friendly_date_ago(comment.created_at),
-                        :user => link_to(comment.author.display_name, user_path(comment.author))) %>
-                  — <span class="action-button text-muted" data-comment-id="<%= comment.id %>" data-method="POST" data-url="<%= changeset_comment_unhide_url(comment.id) %>"><%= t("javascripts.changesets.show.unhide_comment") %></span>
-                 </small>
-                <div class="mx-2">
-                  <%= comment.body.to_html %>
-                </div>
-              </li>
             <% end %>
-          <% end %>
+          </small>
-        </ul>
+          <div class="mx-2">
-      </form>
+            <%= comment.body.to_html %>
-    </div>
+          </div>
+        </li>
+      <% end %>
+    </ul>
   <% end %>
 
   <% unless current_user %>
-    <p class="notice">
+    <p>
       <%= link_to(t(".join_discussion"), login_path(:referer => request.fullpath)) %>
     </p>
   <% end %>
@@ -79,11 +66,11 @@
         <div id="comment-error" class="alert alert-danger p-2 mb-3" hidden>
         </div>
         <div>
-          <input type="submit" name="comment" value="<%= t("javascripts.changesets.show.comment") %>" data-changeset-id="<%= @changeset.id %>" data-method="POST" data-url="<%= changeset_comment_url(@changeset) %>" disabled="1" class="btn btn-sm btn-primary" />
+          <button name="comment" data-method="POST" data-url="<%= changeset_comment_url(@changeset) %>" disabled class="btn btn-sm btn-primary"><%= t("javascripts.changesets.show.comment") %></button>
         </div>
       </form>
     <% else %>
-      <p class="notice">
+      <p>
         <%= t(".still_open") %>
       </p>
     <% end %>

test/application_system_test_case.rb

@@ -33,6 +33,11 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
     end
   end
 
+  def sign_out
+    visit logout_path
+    click_on "Logout", :match => :first
+  end
+
   def within_sidebar(&block)
     within "#sidebar_content", &block
   end

test/controllers/browse_controller_test.rb

@@ -142,20 +142,6 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
     browse_check :changeset_path, changeset.id, "browse/changeset"
   end
 
-  def test_read_changeset_hidden_comments
-    changeset = create(:changeset)
-    create_list(:changeset_comment, 3, :changeset => changeset)
-    create(:changeset_comment, :visible => false, :changeset => changeset)
-
-    browse_check :changeset_path, changeset.id, "browse/changeset"
-    assert_select "div.changeset-comments ul li", :count => 3
-
-    session_for(create(:moderator_user))
-
-    browse_check :changeset_path, changeset.id, "browse/changeset"
-    assert_select "div.changeset-comments ul li", :count => 4
-  end
-
   def test_read_changeset_element_links
     changeset = create(:changeset)
     node = create(:node, :with_history, :changeset => changeset)

test/integration/user_changeset_comments_test.rb

@@ -1,55 +0,0 @@
-require "test_helper"
-
-class UserChangesetCommentsTest < ActionDispatch::IntegrationTest
-  # Test 'log in to comment' message for nonlogged in user
-  def test_log_in_message
-    changeset = create(:changeset, :closed)
-
-    get "/changeset/#{changeset.id}"
-    assert_response :success
-
-    assert_select "div#content" do
-      assert_select "div#sidebar" do
-        assert_select "div#sidebar_content" do
-          assert_select "div" do
-            assert_select "p.notice" do
-              assert_select "a[href='/login?referer=%2Fchangeset%2F#{changeset.id}']", :text => I18n.t("browse.changeset.join_discussion"), :count => 1
-            end
-          end
-        end
-      end
-    end
-  end
-
-  # Test if the form is shown
-  def test_displaying_form
-    user = create(:user)
-    changeset = create(:changeset, :closed)
-
-    get "/login"
-    follow_redirect!
-    # We should now be at the login page
-    assert_response :success
-    assert_template "sessions/new"
-    # We can now login
-    post "/login", :params => { "username" => user.email, "password" => "test" }
-    assert_response :redirect
-
-    get "/changeset/#{changeset.id}"
-
-    assert_response :success
-    assert_template "browse/changeset"
-
-    assert_select "div#content" do
-      assert_select "div#sidebar" do
-        assert_select "div#sidebar_content" do
-          assert_select "div" do
-            assert_select "form[action='#']" do
-              assert_select "textarea[name=text]"
-            end
-          end
-        end
-      end
-    end
-  end
-end

test/system/changeset_comments_test.rb

@@ -0,0 +1,162 @@
+require "application_system_test_case"
+
+class ChangesetCommentsTest < ApplicationSystemTestCase
+  test "open changeset has a still open notice" do
+    changeset = create(:changeset)
+    sign_in_as(create(:user))
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_no_button "Comment"
+      assert_text "Changeset still open"
+    end
+  end
+
+  test "changeset has a login notice" do
+    changeset = create(:changeset, :closed)
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_no_button "Subscribe"
+      assert_no_button "Comment"
+      assert_link "Log in to join the discussion", :href => login_path(:referer => changeset_path(changeset))
+    end
+  end
+
+  test "can add a comment to a changeset" do
+    changeset = create(:changeset, :closed)
+    user = create(:user)
+    sign_in_as(user)
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_no_content "Comment from #{user.display_name}"
+      assert_no_content "Some newly added changeset comment"
+      assert_button "Comment", :disabled => true
+
+      fill_in "text", :with => "Some newly added changeset comment"
+
+      assert_button "Comment", :disabled => false
+
+      click_on "Comment"
+
+      assert_content "Comment from #{user.display_name}"
+      assert_content "Some newly added changeset comment"
+    end
+  end
+
+  test "regular users can't hide comments" do
+    changeset = create(:changeset, :closed)
+    create(:changeset_comment, :changeset => changeset, :body => "Unwanted comment")
+    sign_in_as(create(:user))
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_text "Unwanted comment"
+      assert_no_button "hide"
+    end
+  end
+
+  test "moderators can hide comments" do
+    changeset = create(:changeset, :closed)
+    create(:changeset_comment, :changeset => changeset, :body => "Unwanted comment")
+
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_text "Unwanted comment"
+    end
+
+    sign_in_as(create(:moderator_user))
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_text "Unwanted comment"
+      assert_button "hide", :exact => true
+      assert_no_button "unhide", :exact => true
+
+      click_on "hide", :exact => true
+
+      assert_text "Unwanted comment"
+      assert_no_button "hide", :exact => true
+      assert_button "unhide", :exact => true
+    end
+
+    sign_out
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_no_text "Unwanted comment"
+    end
+  end
+
+  test "moderators can unhide comments" do
+    changeset = create(:changeset, :closed)
+    create(:changeset_comment, :changeset => changeset, :body => "Wanted comment", :visible => false)
+
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_no_text "Wanted comment"
+    end
+
+    sign_in_as(create(:moderator_user))
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_text "Wanted comment"
+      assert_no_button "hide", :exact => true
+      assert_button "unhide", :exact => true
+
+      click_on "unhide", :exact => true
+
+      assert_text "Wanted comment"
+      assert_button "hide", :exact => true
+      assert_no_button "unhide", :exact => true
+    end
+
+    sign_out
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_text "Wanted comment"
+    end
+  end
+
+  test "can subscribe" do
+    changeset = create(:changeset, :closed)
+    user = create(:user)
+    sign_in_as(user)
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_button "Subscribe"
+      assert_no_button "Unsubscribe"
+
+      click_on "Subscribe"
+
+      assert_no_button "Subscribe"
+      assert_button "Unsubscribe"
+    end
+  end
+
+  test "can't subscribe when blocked" do
+    changeset = create(:changeset, :closed)
+    user = create(:user)
+    sign_in_as(user)
+    visit changeset_path(changeset)
+    create(:user_block, :user => user)
+
+    within_sidebar do
+      assert_no_text "Your access to the API has been blocked"
+      assert_button "Subscribe"
+      assert_no_button "Unsubscribe"
+
+      click_on "Subscribe"
+
+      assert_text "Your access to the API has been blocked"
+      assert_button "Subscribe"
+      assert_no_button "Unsubscribe"
+    end
+  end
+end