From: Andy Allan Date: Sun, 25 Feb 2024 10:35:20 +0000 (+0100) Subject: Merge pull request #4536 from tomhughes/trace-size-limit X-Git-Tag: live~1187 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/2dfe6f3f2e911101f4e4e3b90a00e2ae97725d04?hp=741ed5883816460a6498174cea46f43951a438cc Merge pull request #4536 from tomhughes/trace-size-limit Add a limit on the number of points in a GPS trace --- diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9874aa379..6c917e218 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -66,7 +66,7 @@ Metrics/BlockNesting: # Offense count: 26 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 305 + Max: 313 # Offense count: 59 # Configuration parameters: AllowedMethods, AllowedPatterns. 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/concerns/session_methods.rb b/app/controllers/concerns/session_methods.rb index fca851eeb..cebe932fc 100644 --- a/app/controllers/concerns/session_methods.rb +++ b/app/controllers/concerns/session_methods.rb @@ -62,9 +62,10 @@ module SessionMethods ## # def unconfirmed_login(user) - session[:token] = user.tokens.create.token + session[:pending_user] = user.id - redirect_to :controller => "confirmations", :action => "confirm", :display_name => user.display_name + redirect_to :controller => "confirmations", :action => "confirm", + :display_name => user.display_name, :referer => session[:referer] session.delete(:remember_me) session.delete(:referer) diff --git a/app/controllers/concerns/user_methods.rb b/app/controllers/concerns/user_methods.rb index eb7d38988..8cba09827 100644 --- a/app/controllers/concerns/user_methods.rb +++ b/app/controllers/concerns/user_methods.rb @@ -51,7 +51,7 @@ module UserMethods flash[:notice] = t "accounts.update.success_confirm_needed" begin - UserMailer.email_confirm(user, user.tokens.create).deliver_later + UserMailer.email_confirm(user, user.generate_token_for(:new_email)).deliver_later rescue StandardError # Ignore errors sending email end diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 65f560571..48b8dabf2 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -15,41 +15,37 @@ class ConfirmationsController < ApplicationController def confirm if request.post? - token = UserToken.find_by(:token => params[:confirm_string]) - if token&.user&.active? - flash[:error] = t(".already active") - redirect_to login_path - elsif !token || token.expired? + token = params[:confirm_string] + + user = User.find_by_token_for(:new_user, token) || + UserToken.unexpired.find_by(:token => token)&.user + + if !user flash[:error] = t(".unknown token") redirect_to :action => "confirm" - elsif !token.user.visible? - render_unknown_user token.user.display_name + elsif user.active? + flash[:error] = t(".already active") + redirect_to login_path + elsif !user.visible? + render_unknown_user user.display_name else - user = token.user user.activate user.email_valid = true flash[:notice] = gravatar_status_message(user) if gravatar_enable(user) user.save! - referer = safe_referer(token.referer) if token.referer - token.destroy + referer = safe_referer(params[:referer]) if params[:referer] + UserToken.delete_by(:token => token) - if session[:token] - token = UserToken.find_by(:token => session[:token]) - session.delete(:token) - else - token = nil - end - - if token.nil? || token.user != user - flash[:notice] = t(".success") - redirect_to login_path(:referer => referer) - else - token.destroy + pending_user = session.delete(:pending_user) + if user.id == pending_user session[:user] = user.id session[:fingerprint] = user.fingerprint redirect_to referer || welcome_path + else + flash[:notice] = t(".success") + redirect_to login_path(:referer => referer) end end else @@ -61,12 +57,11 @@ class ConfirmationsController < ApplicationController def confirm_resend user = User.visible.find_by(:display_name => params[:display_name]) - token = UserToken.find_by(:token => session[:token]) - if user.nil? || token.nil? || token.user != user + if user.nil? || user.id != session[:pending_user] flash[:error] = t ".failure", :name => params[:display_name] else - UserMailer.signup_confirm(user, user.tokens.create).deliver_later + UserMailer.signup_confirm(user, user.generate_token_for(:new_user)).deliver_later flash[:notice] = { :partial => "confirmations/resend_success_flash", :locals => { :email => user.email, :sender => Settings.email_from } } end @@ -75,9 +70,12 @@ class ConfirmationsController < ApplicationController def confirm_email if request.post? - token = UserToken.find_by(:token => params[:confirm_string]) - if token&.user&.new_email? - self.current_user = token.user + token = params[:confirm_string] + + self.current_user = User.find_by_token_for(:new_email, token) || + UserToken.unexpired.find_by(:token => params[:confirm_string])&.user + + if current_user&.new_email? current_user.email = current_user.new_email current_user.new_email = nil current_user.email_valid = true @@ -94,7 +92,7 @@ class ConfirmationsController < ApplicationController current_user.tokens.delete_all session[:user] = current_user.id session[:fingerprint] = current_user.fingerprint - elsif token + elsif current_user flash[:error] = t ".failure" else flash[:error] = t ".unknown_token" diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 87d25df68..8025fd700 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -19,11 +19,10 @@ class PasswordsController < ApplicationController @title = t ".title" if params[:token] - token = UserToken.find_by(:token => params[:token]) + self.current_user = User.find_by_token_for(:password_reset, params[:token]) || + UserToken.unexpired.find_by(:token => params[:token])&.user - if token - self.current_user = token.user - else + if current_user.nil? flash[:error] = t ".flash token bad" redirect_to :action => "new" end @@ -42,7 +41,7 @@ class PasswordsController < ApplicationController end if user - token = user.tokens.create + token = user.generate_token_for(:password_reset) UserMailer.lost_password(user, token).deliver_later flash[:notice] = t ".notice email on way" redirect_to login_path @@ -54,11 +53,10 @@ class PasswordsController < ApplicationController def update if params[:token] - token = UserToken.find_by(:token => params[:token]) - - if token - self.current_user = token.user + self.current_user = User.find_by_token_for(:password_reset, params[:token]) || + UserToken.unexpired.find_by(:token => params[:token])&.user + if current_user if params[:user] current_user.pass_crypt = params[:user][:pass_crypt] current_user.pass_crypt_confirmation = params[:user][:pass_crypt_confirmation] @@ -66,7 +64,7 @@ class PasswordsController < ApplicationController current_user.email_valid = true if current_user.save - token.destroy + UserToken.delete_by(:token => params[:token]) session[:fingerprint] = current_user.fingerprint flash[:notice] = t ".flash changed" successful_login(current_user) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3c2084a5b..e57ffc06a 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -27,12 +27,7 @@ class SessionsController < ApplicationController @title = t ".title" if request.post? - if session[:token] - token = UserToken.find_by(:token => session[:token]) - token&.destroy - session.delete(:token) - end - + session.delete(:pending_user) session.delete(:user) session_expires_automatically 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 ab13f93be..fbf49ecbe 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -203,8 +203,8 @@ class UsersController < ApplicationController session[:referer] = referer successful_login(current_user) else - session[:token] = current_user.tokens.create.token - UserMailer.signup_confirm(current_user, current_user.tokens.create(:referer => referer)).deliver_later + session[:pending_user] = current_user.id + UserMailer.signup_confirm(current_user, current_user.generate_token_for(:new_user), referer).deliver_later redirect_to :controller => :confirmations, :action => :confirm, :display_name => current_user.display_name end else @@ -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/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 0894b972d..92c64b4d6 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -10,11 +10,12 @@ class UserMailer < ApplicationMailer before_action :set_shared_template_vars before_action :attach_project_logo - def signup_confirm(user, token) + def signup_confirm(user, token, referer = nil) with_recipient_locale user do @url = url_for(:controller => "confirmations", :action => "confirm", :display_name => user.display_name, - :confirm_string => token.token) + :confirm_string => token, + :referer => referer) mail :to => user.email, :subject => t(".subject") @@ -25,7 +26,7 @@ class UserMailer < ApplicationMailer with_recipient_locale user do @address = user.new_email @url = url_for(:controller => "confirmations", :action => "confirm_email", - :confirm_string => token.token) + :confirm_string => token) mail :to => user.new_email, :subject => t(".subject") @@ -34,7 +35,7 @@ class UserMailer < ApplicationMailer def lost_password(user, token) with_recipient_locale user do - @url = user_reset_password_url(:token => token.token) + @url = user_reset_password_url(:token => token) mail :to => user.email, :subject => t(".subject") diff --git a/app/models/trace.rb b/app/models/trace.rb index 3ab25ce30..818cc363b 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/models/user.rb b/app/models/user.rb index 7faf748cd..6fa0f330e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -124,6 +124,18 @@ class User < ApplicationRecord before_save :update_tile after_save :spam_check + generates_token_for :new_user, :expires_in => 1.week do + fingerprint + end + + generates_token_for :new_email, :expires_in => 1.week do + fingerprint + end + + generates_token_for :password_reset, :expires_in => 1.week do + fingerprint + end + def display_name_cannot_be_user_id_with_other_id display_name&.match(/^user_(\d+)$/i) do |m| errors.add :display_name, I18n.t("activerecord.errors.messages.display_name_is_user_n") unless m[1].to_i == id diff --git a/app/models/user_token.rb b/app/models/user_token.rb index f927563a9..fbd276a6f 100644 --- a/app/models/user_token.rb +++ b/app/models/user_token.rb @@ -21,6 +21,8 @@ class UserToken < ApplicationRecord belongs_to :user + scope :unexpired, -> { where("expiry >= now()") } + after_initialize :set_defaults def expired? 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/controllers/confirmations_controller_test.rb b/test/controllers/confirmations_controller_test.rb index 083619962..82580dc68 100644 --- a/test/controllers/confirmations_controller_test.rb +++ b/test/controllers/confirmations_controller_test.rb @@ -39,7 +39,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest user = build(:user, :pending) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) get user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } assert_response :success @@ -51,7 +51,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) # Get the confirmation page get user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } @@ -73,7 +73,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path @@ -87,7 +87,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } assert_redirected_to welcome_path @@ -98,7 +98,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path session_for(create(:user)) @@ -113,11 +113,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path(:referer => new_diary_entry_path) assert_match(/Confirmed your account/, flash[:notice]) end @@ -127,9 +127,9 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to new_diary_entry_path end @@ -138,12 +138,12 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path session_for(create(:user)) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path(:referer => new_diary_entry_path) assert_match(/Confirmed your account/, flash[:notice]) end @@ -153,9 +153,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:expiry => 1.day.ago).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + travel 2.weeks do + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + end assert_redirected_to :action => "confirm" assert_match(/confirmation code has expired/, flash[:error]) end @@ -165,15 +167,15 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to new_diary_entry_path post logout_path - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path assert_match(/already been confirmed/, flash[:error]) end @@ -183,7 +185,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) User.find_by(:display_name => user.display_name).hide! @@ -269,7 +271,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest def test_confirm_email_get user = create(:user) - confirm_string = user.tokens.create.token + confirm_string = user.generate_token_for(:new_email) get user_confirm_email_path, :params => { :confirm_string => confirm_string } assert_response :success @@ -279,7 +281,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest def test_confirm_email_success user = create(:user, :new_email => "test-new@example.com") stub_gravatar_request(user.new_email) - confirm_string = user.tokens.create.token + confirm_string = user.generate_token_for(:new_email) post user_confirm_email_path, :params => { :confirm_string => confirm_string } assert_response :redirect @@ -289,7 +291,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest def test_confirm_email_already_confirmed user = create(:user) - confirm_string = user.tokens.create.token + confirm_string = user.generate_token_for(:new_email) post user_confirm_email_path, :params => { :confirm_string => confirm_string } assert_response :redirect @@ -312,7 +314,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest # switch to email that has a gravatar user = create(:user, :new_email => "test-new@example.com") stub_gravatar_request(user.new_email, 200) - confirm_string = user.tokens.create.token + confirm_string = user.generate_token_for(:new_email) # precondition gravatar should be turned off assert_not user.image_use_gravatar post user_confirm_email_path, :params => { :confirm_string => confirm_string } @@ -327,7 +329,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest # switch to email without a gravatar user = create(:user, :new_email => "test-new@example.com", :image_use_gravatar => true) stub_gravatar_request(user.new_email, 404) - confirm_string = user.tokens.create.token + confirm_string = user.generate_token_for(:new_email) # precondition gravatar should be turned on assert user.image_use_gravatar post user_confirm_email_path, :params => { :confirm_string => confirm_string } diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb index c39e8465b..25cfdd4e5 100644 --- a/test/controllers/passwords_controller_test.rb +++ b/test/controllers/passwords_controller_test.rb @@ -127,21 +127,21 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to :action => :new # Create a valid token for a user - token = user.tokens.create + token = user.generate_token_for(:password_reset) # Test a request with a valid token - get user_reset_password_path, :params => { :token => token.token } + get user_reset_password_path, :params => { :token => token } assert_response :success assert_template :edit # Test that errors are reported for erroneous submissions - post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "different_password" } } + post user_reset_password_path, :params => { :token => token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "different_password" } } assert_response :success assert_template :edit assert_select "div.invalid-feedback" # Test setting a new password - post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "new_password" } } + post user_reset_password_path, :params => { :token => token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "new_password" } } assert_response :redirect assert_redirected_to root_path assert_equal user.id, session[:user] diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 4234bee70..71d6de184 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -88,10 +88,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest user = build(:user, :pending) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - - assert_difference "User.find_by(:email => user.email).tokens.count", -1 do - post logout_path - end + post logout_path assert_response :redirect assert_redirected_to root_path end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 402129d32..a530a6f85 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -312,15 +312,13 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_difference "User.count", 1 do assert_difference "ActionMailer::Base.deliveries.size", 1 do - perform_enqueued_jobs do - post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - end + post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } + assert_enqueued_with :job => ActionMailer::MailDeliveryJob, + :args => proc { |args| args[3][:args][2] == welcome_path(:editor => "id", :zoom => 1, :lat => 2, :lon => 3) } + perform_enqueued_jobs end end - assert_equal welcome_path(:editor => "id", :zoom => 1, :lat => 2, :lon => 3), - User.find_by(:email => user.email).tokens.order("id DESC").first.referer - ActionMailer::Base.deliveries.clear end 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 253f298a5..59efeaabb 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -209,11 +209,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -222,11 +222,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -362,11 +362,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_openid/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -375,11 +375,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -516,11 +516,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_google/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -529,11 +529,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -668,11 +668,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_facebook/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -681,11 +681,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -820,11 +820,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_microsoft/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -833,11 +833,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -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 @@ -974,11 +974,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_github/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -987,11 +987,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -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 @@ -1128,11 +1128,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_wikipedia/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -1141,11 +1141,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success 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