From: Andy Allan Date: Sun, 25 Feb 2024 10:32:15 +0000 (+0100) Subject: Merge pull request #4535 from tomhughes/rails-tokens X-Git-Tag: live~1197 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/52f755cb27dd7dad2fe65a3deb5d4591c6675dec?hp=4dff06a6293971c3e17f8508859a1d80717a23f6 Merge pull request #4535 from tomhughes/rails-tokens Use rails generated tokens for emails --- diff --git a/app/assets/javascripts/index/changeset.js b/app/assets/javascripts/index/changeset.js index c6e77bc71..75a1f7b4d 100644 --- a/app/assets/javascripts/index/changeset.js +++ b/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); - $(form).find("input[type=submit]").prop("disabled", true); + content.find("#comment-error").prop("hidden", 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); - $(form).find("#comment-error").prop("hidden", false); - $(form).find("input[type=submit]").prop("disabled", false); + content.find("button[data-method][data-url]").prop("disabled", false); + content.find("#comment-error") + .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); - }); - - content.find(".action-button").on("click", function (e) { - e.preventDefault(); - var data = $(e.target).data(); - updateChangeset(e.target.form, data.method, data.url); + var include_data = e.target.name === "comment"; + updateChangeset(data.method, data.url, include_data); }); content.find("textarea").on("input", function (e) { diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index b5bd5adec..1f7c45db5 100644 --- a/app/assets/stylesheets/common.scss +++ b/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; diff --git a/app/controllers/api/traces_controller.rb b/app/controllers/api/traces_controller.rb index b66aead38..956bcde6e 100644 --- a/app/controllers/api/traces_controller.rb +++ b/app/controllers/api/traces_controller.rb @@ -35,7 +35,7 @@ module Api trace = do_create(params[:file], tags, description, visibility) if trace.id - TraceImporterJob.perform_later(trace) + trace.schedule_import render :plain => trace.id.to_s elsif trace.valid? head :internal_server_error @@ -66,7 +66,7 @@ module Api if trace.user == current_user trace.visible = false trace.save! - TraceDestroyerJob.perform_later(trace) + trace.schedule_destruction head :ok else diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index 242f8113c..df6337147 100644 --- a/app/controllers/traces_controller.rb +++ b/app/controllers/traces_controller.rb @@ -126,7 +126,7 @@ class TracesController < ApplicationController flash[:notice] = t ".trace_uploaded" flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4 - TraceImporterJob.perform_later(@trace) + @trace.schedule_import redirect_to :action => :index, :display_name => current_user.display_name else flash[:error] = t(".upload_failed") if @trace.valid? @@ -176,7 +176,7 @@ class TracesController < ApplicationController trace.visible = false trace.save flash[:notice] = t ".scheduled_for_deletion" - TraceDestroyerJob.perform_later(trace) + trace.schedule_destruction redirect_to :action => :index, :display_name => trace.user.display_name end rescue ActiveRecord::RecordNotFound diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 429fa47a4..fbf49ecbe 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -247,7 +247,7 @@ class UsersController < ApplicationController when "openid" uid.match(%r{https://www.google.com/accounts/o8/id?(.*)}) || uid.match(%r{https://me.yahoo.com/(.*)}) - when "google", "facebook", "microsoft" + when "google", "facebook", "microsoft", "github", "wikipedia" true else false diff --git a/app/models/trace.rb b/app/models/trace.rb index 2411fb9b7..1e8f25a83 100644 --- a/app/models/trace.rb +++ b/app/models/trace.rb @@ -267,6 +267,14 @@ class Trace < ApplicationRecord end end + def schedule_import + TraceImporterJob.new(self).enqueue(:priority => user.traces.where(:inserted => false).count) + end + + def schedule_destruction + TraceDestroyerJob.perform_later(self) + end + private def content_type(file) diff --git a/app/views/browse/changeset.html.erb b/app/views/browse/changeset.html.erb index 501712717..ac5382b14 100644 --- a/app/views/browse/changeset.html.erb +++ b/app/views/browse/changeset.html.erb @@ -18,54 +18,41 @@ <% if current_user %>
<% if @changeset.subscribers.exists?(current_user.id) %> - + <% else %> - + <% end %>
<% end %> <% if @comments.length > 0 %> -
-
-
    - <% @comments.each do |comment| %> - <% if comment.visible %> -
  • - - <%= t(".comment_by_html", - :time_ago => friendly_date_ago(comment.created_at), - :user => link_to(comment.author.display_name, user_path(comment.author))) %> - <% if current_user and current_user.moderator? %> - — <%= t("javascripts.changesets.show.hide_comment") %> - <% end %> - -
    - <%= comment.body.to_html %> -
    -
    • - <% elsif current_user and current_user.moderator? %> -
  • - - <%= t(".hidden_comment_by_html", - :time_ago => friendly_date_ago(comment.created_at), - :user => link_to(comment.author.display_name, user_path(comment.author))) %> - — <%= t("javascripts.changesets.show.unhide_comment") %> - -
    - <%= comment.body.to_html %> -
    -
    • +
      + <% @comments.each do |comment| %> + <% next unless comment.visible || current_user&.moderator? %> +
    • + + <%= t comment.visible ? ".comment_by_html" : ".hidden_comment_by_html", + :time_ago => friendly_date_ago(comment.created_at), + :user => link_to(comment.author.display_name, user_path(comment.author)) %> + <% if current_user&.moderator? %> + — + <%= tag.button t("javascripts.changesets.show.#{comment.visible ? 'hide' : 'unhide'}_comment"), + :class => "btn btn-sm small btn-link link-secondary p-0 align-baseline", + :data => { :method => "POST", + :url => comment.visible ? changeset_comment_hide_url(comment) : changeset_comment_unhide_url(comment) } %> <% end %> - <% end %> -
    - -
+ +
+ <%= comment.body.to_html %> +
+ + <% end %> + <% end %> <% unless current_user %> -

+

<%= link_to(t(".join_discussion"), login_path(:referer => request.fullpath)) %>

<% end %> @@ -79,11 +66,11 @@
- " data-changeset-id="<%= @changeset.id %>" data-method="POST" data-url="<%= changeset_comment_url(@changeset) %>" disabled="1" class="btn btn-sm btn-primary" /> +
<% else %> -

+

<%= t(".still_open") %>

<% end %> diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb index 7931546d4..ef8f0e371 100644 --- a/test/application_system_test_case.rb +++ b/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 diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb index 1023d76ae..2bb743636 100644 --- a/test/controllers/browse_controller_test.rb +++ b/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) diff --git a/test/integration/user_changeset_comments_test.rb b/test/integration/user_changeset_comments_test.rb deleted file mode 100644 index 2b95094fe..000000000 --- a/test/integration/user_changeset_comments_test.rb +++ /dev/null @@ -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 diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 35f98d17d..59efeaabb 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -852,7 +852,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest OmniAuth.config.add_mock(:github, :uid => "123454321", :info => { "email" => new_email }) assert_difference("User.count") do - assert_difference("ActionMailer::Base.deliveries.size", 1) do + assert_no_difference("ActionMailer::Base.deliveries.size") do perform_enqueued_jobs do post "/user/new", :params => { :user => { :email => new_email, @@ -880,7 +880,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest :read_ct => 1, :read_tou => 1 } assert_response :redirect - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name + assert_redirected_to welcome_path follow_redirect! end end @@ -888,7 +888,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest # Check the page assert_response :success - assert_template "confirmations/confirm" + assert_template "site/welcome" ActionMailer::Base.deliveries.clear end @@ -1006,7 +1006,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest OmniAuth.config.add_mock(:wikipedia, :uid => "123454321", :info => { "email" => new_email }) assert_difference("User.count") do - assert_difference("ActionMailer::Base.deliveries.size", 1) do + assert_no_difference("ActionMailer::Base.deliveries.size") do perform_enqueued_jobs do post "/user/new", :params => { :user => { :email => new_email, @@ -1034,7 +1034,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest :read_ct => 1, :read_tou => 1 } assert_response :redirect - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name + assert_redirected_to welcome_path follow_redirect! end end @@ -1042,7 +1042,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest # Check the page assert_response :success - assert_template "confirmations/confirm" + assert_template "site/welcome" ActionMailer::Base.deliveries.clear end diff --git a/test/system/changeset_comments_test.rb b/test/system/changeset_comments_test.rb new file mode 100644 index 000000000..b12aab5ee --- /dev/null +++ b/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