diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 7ed6470b84..9516a30126 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -23,7 +23,8 @@ def initialize(user) can :read, Redaction can [:create, :destroy], :session can [:read, :data, :georss], Trace - can [:read, :terms, :create, :save, :suspended, :auth_success, :auth_failure], User + can [:read, :create, :suspended, :auth_success, :auth_failure], User + can [:read, :update], :account_terms can :read, UserBlock end diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index 323f60e08c..9ce6aec343 100644 --- a/app/assets/stylesheets/common.scss +++ b/app/assets/stylesheets/common.scss @@ -818,7 +818,7 @@ tr.turn { /* Rules for the account confirmation page */ -.users-terms { +.accounts-terms-show { .legale { padding: $lineheight; margin-bottom: $lineheight; diff --git a/app/controllers/accounts/terms_controller.rb b/app/controllers/accounts/terms_controller.rb new file mode 100644 index 0000000000..0513a031c9 --- /dev/null +++ b/app/controllers/accounts/terms_controller.rb @@ -0,0 +1,65 @@ +module Accounts + class TermsController < ApplicationController + include SessionMethods + + layout "site" + + before_action :disable_terms_redirect + before_action :authorize_web + before_action :set_locale + before_action :check_database_readable + + authorize_resource :class => :account_terms + + def show + @legale = params[:legale] || OSM.ip_to_country(request.remote_ip) || Settings.default_legale + @text = OSM.legal_text_for_country(@legale) + + if request.xhr? + render :partial => "terms" + else + @title = t ".title" + + if current_user&.terms_agreed? + # Already agreed to terms, so just show settings + redirect_to edit_account_path + elsif current_user.nil? + redirect_to login_path(:referer => request.fullpath) + end + end + end + + def update + @title = t "users.new.title" + + if params[:decline] || !(params[:read_tou] && params[:read_ct]) + if current_user + current_user.terms_seen = true + + flash[:notice] = { :partial => "accounts/terms/terms_declined_flash" } if current_user.save + + referer = safe_referer(params[:referer]) if params[:referer] + + redirect_to referer || edit_account_path + elsif params[:decline] + redirect_to t("users.terms.declined"), :allow_other_host => true + else + redirect_to account_terms_path + end + elsif current_user + unless current_user.terms_agreed? + current_user.consider_pd = params[:user][:consider_pd] + current_user.tou_agreed = Time.now.utc + current_user.terms_agreed = Time.now.utc + current_user.terms_seen = true + + flash[:notice] = t "users.new.terms accepted" if current_user.save + end + + referer = safe_referer(params[:referer]) if params[:referer] + + redirect_to referer || edit_account_path + end + end + end +end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1ef49bf462..25c430f1c0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -56,11 +56,11 @@ def authorize_web # don't allow access to any auth-requiring part of the site unless # the new CTs have been seen (and accept/decline chosen). elsif !current_user.terms_seen && flash[:skip_terms].nil? - flash[:notice] = t "users.terms.you need to accept or decline" + flash[:notice] = t "accounts.terms.show.you need to accept or decline" if params[:referer] - redirect_to :controller => "users", :action => "terms", :referer => params[:referer] + redirect_to account_terms_path(:referer => params[:referer]) else - redirect_to :controller => "users", :action => "terms", :referer => request.fullpath + redirect_to account_terms_path(:referer => request.fullpath) end end end diff --git a/app/controllers/concerns/session_methods.rb b/app/controllers/concerns/session_methods.rb index 45cf0d9439..2cfc4e8231 100644 --- a/app/controllers/concerns/session_methods.rb +++ b/app/controllers/concerns/session_methods.rb @@ -48,7 +48,7 @@ def successful_login(user, referer = nil) # - If they were referred to the login, send them back there. # - Otherwise, send them to the home page. if !user.terms_seen - redirect_to :controller => :users, :action => :terms, :referer => target + redirect_to account_terms_path(:referer => target) elsif user.blocked_on_view redirect_to user.blocked_on_view, :referer => target else diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fa311ab39e..904b960a28 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -6,7 +6,6 @@ class UsersController < ApplicationController layout "site" skip_before_action :verify_authenticity_token, :only => [:auth_success] - before_action :disable_terms_redirect, :only => [:terms, :save] before_action :authorize_web before_action :set_locale before_action :check_database_readable @@ -106,57 +105,6 @@ def destroy redirect_to user_path(:display_name => params[:display_name]) end - def terms - @legale = params[:legale] || OSM.ip_to_country(request.remote_ip) || Settings.default_legale - @text = OSM.legal_text_for_country(@legale) - - if request.xhr? - render :partial => "terms" - else - @title = t ".title" - - if current_user&.terms_agreed? - # Already agreed to terms, so just show settings - redirect_to edit_account_path - elsif current_user.nil? - redirect_to login_path(:referer => request.fullpath) - end - end - end - - def save - @title = t "users.new.title" - - if params[:decline] || !(params[:read_tou] && params[:read_ct]) - if current_user - current_user.terms_seen = true - - flash[:notice] = { :partial => "users/terms_declined_flash" } if current_user.save - - referer = safe_referer(params[:referer]) if params[:referer] - - redirect_to referer || edit_account_path - elsif params[:decline] - redirect_to t("users.terms.declined"), :allow_other_host => true - else - redirect_to :action => :terms - end - elsif current_user - unless current_user.terms_agreed? - current_user.consider_pd = params[:user][:consider_pd] - current_user.tou_agreed = Time.now.utc - current_user.terms_agreed = Time.now.utc - current_user.terms_seen = true - - flash[:notice] = t "users.new.terms accepted" if current_user.save - end - - referer = safe_referer(params[:referer]) if params[:referer] - - redirect_to referer || edit_account_path - end - end - def go_public current_user.data_public = true current_user.save diff --git a/app/views/accounts/edit.html.erb b/app/views/accounts/edit.html.erb index e31c5e90d2..16f109c9f9 100644 --- a/app/views/accounts/edit.html.erb +++ b/app/views/accounts/edit.html.erb @@ -53,7 +53,7 @@ <% end %> <% else %> <%= t ".contributor terms.not yet agreed" %> - <%= link_to t(".contributor terms.review link text"), :controller => "users", :action => "terms" %> + <%= link_to t(".contributor terms.review link text"), account_terms_path %> <% end %> diff --git a/app/views/users/_terms.html.erb b/app/views/accounts/terms/_terms.html.erb similarity index 100% rename from app/views/users/_terms.html.erb rename to app/views/accounts/terms/_terms.html.erb diff --git a/app/views/users/_terms_declined_flash.html.erb b/app/views/accounts/terms/_terms_declined_flash.html.erb similarity index 100% rename from app/views/users/_terms_declined_flash.html.erb rename to app/views/accounts/terms/_terms_declined_flash.html.erb diff --git a/app/views/users/terms.html.erb b/app/views/accounts/terms/show.html.erb similarity index 98% rename from app/views/users/terms.html.erb rename to app/views/accounts/terms/show.html.erb index a5dc3291de..3cc52302ff 100644 --- a/app/views/users/terms.html.erb +++ b/app/views/accounts/terms/show.html.erb @@ -9,7 +9,7 @@ <% end %> -<%= form_tag({ :action => "save" }) do %> +<%= form_tag account_terms_path, :method => :put do %>

<%= t ".read and accept with tou" %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index fcaf6ddfe8..69e220a21a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -304,6 +304,35 @@ en: recent_editing_html: "As you have edited recently your account cannot currently be deleted. Deletion will be possible in %{time}." confirm_delete: Are you sure? cancel: Cancel + terms: + show: + title: "Terms" + heading: "Terms" + heading_ct: "Contributor terms" + read and accept with tou: "Please read the contributor agreement and the terms of use, check both checkboxes when done and then press the continue button." + contributor_terms_explain: "This agreement governs the terms for your existing and future contributions." + read_ct: "I have read and agree to the above contributor terms" + tou_explain_html: "These %{tou_link} govern the use of the website and other infrastructure provided by the OSMF. Please click on the link, read and agree to the text." + read_tou: "I have read and agree to the Terms of Use" + consider_pd: "In addition to the above, I consider my contributions to be in the Public Domain" + consider_pd_why: "what's this?" + consider_pd_why_url: https://osmfoundation.org/wiki/Licence_and_Legal_FAQ/Why_would_I_want_my_contributions_to_be_public_domain + guidance_info_html: "Information to help understand these terms: a %{readable_summary_link} and some %{informal_translations_link}" + readable_summary: human readable summary + informal_translations: informal translations + continue: "Continue" + declined: "https://wiki.openstreetmap.org/wiki/Contributor_Terms_Declined" + cancel: "Cancel" + you need to accept or decline: "Please read and then either accept or decline the new Contributor Terms to continue." + legale_select: "Country of residence:" + legale_names: + france: "France" + italy: "Italy" + rest_of_world: "Rest of the world" + terms_declined_flash: + terms_declined_html: We are sorry that you have decided to not accept the new Contributor Terms. For more information, please see %{terms_declined_link}. + terms_declined_link: this wiki page + terms_declined_url: https://wiki.openstreetmap.org/wiki/Contributor_Terms_Declined browse: deleted_ago_by_html: "Deleted %{time_ago} by %{user}" edited_ago_by_html: "Edited %{time_ago} by %{user}" @@ -2762,34 +2791,6 @@ en: consider_pd_url: https://osmfoundation.org/wiki/Licence_and_Legal_FAQ/Why_would_I_want_my_contributions_to_be_public_domain or: "or" use external auth: "or sign up with a third party" - terms: - title: "Terms" - heading: "Terms" - heading_ct: "Contributor terms" - read and accept with tou: "Please read the contributor agreement and the terms of use, check both checkboxes when done and then press the continue button." - contributor_terms_explain: "This agreement governs the terms for your existing and future contributions." - read_ct: "I have read and agree to the above contributor terms" - tou_explain_html: "These %{tou_link} govern the use of the website and other infrastructure provided by the OSMF. Please click on the link, read and agree to the text." - read_tou: "I have read and agree to the Terms of Use" - consider_pd: "In addition to the above, I consider my contributions to be in the Public Domain" - consider_pd_why: "what's this?" - consider_pd_why_url: https://osmfoundation.org/wiki/Licence_and_Legal_FAQ/Why_would_I_want_my_contributions_to_be_public_domain - guidance_info_html: "Information to help understand these terms: a %{readable_summary_link} and some %{informal_translations_link}" - readable_summary: human readable summary - informal_translations: informal translations - continue: "Continue" - declined: "https://wiki.openstreetmap.org/wiki/Contributor_Terms_Declined" - cancel: "Cancel" - you need to accept or decline: "Please read and then either accept or decline the new Contributor Terms to continue." - legale_select: "Country of residence:" - legale_names: - france: "France" - italy: "Italy" - rest_of_world: "Rest of the world" - terms_declined_flash: - terms_declined_html: We are sorry that you have decided to not accept the new Contributor Terms. For more information, please see %{terms_declined_link}. - terms_declined_link: this wiki page - terms_declined_url: https://wiki.openstreetmap.org/wiki/Contributor_Terms_Declined no_such_user: title: "No such user" heading: "The user %{user} does not exist" diff --git a/config/routes.rb b/config/routes.rb index d89068c140..479d353463 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -182,8 +182,6 @@ get "/key" => "site#key" get "/id" => "site#id" get "/query" => "browse#query" - get "/user/terms" => "users#terms" - post "/user/save" => "users#save" post "/user/:display_name/confirm/resend" => "confirmations#confirm_resend", :as => :user_confirm_resend match "/user/:display_name/confirm" => "confirmations#confirm", :via => [:get, :post] match "/user/confirm" => "confirmations#confirm", :via => [:get, :post] @@ -267,6 +265,7 @@ post "/diary_comments/:comment/unhide" => "diary_comments#unhide", :comment => /\d+/, :as => :unhide_diary_comment # user pages + get "/user/terms", :to => redirect(:path => "/account/terms") resources :users, :path => "user", :param => :display_name, :only => [:new, :create, :show, :destroy] do resource :role, :controller => "user_roles", :path => "roles/:role", :only => [:create, :destroy] scope :module => :users do @@ -278,7 +277,10 @@ post "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user resource :account, :only => [:edit, :update, :destroy] do - resource :deletion, :module => :accounts, :only => :show + scope :module => :accounts do + resource :terms, :only => [:show, :update] + resource :deletion, :only => :show + end end resource :dashboard, :only => [:show] diff --git a/test/controllers/accounts/terms_controller_test.rb b/test/controllers/accounts/terms_controller_test.rb new file mode 100644 index 0000000000..7688846662 --- /dev/null +++ b/test/controllers/accounts/terms_controller_test.rb @@ -0,0 +1,91 @@ +require "test_helper" + +module Accounts + class TermsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/account/terms", :method => :get }, + { :controller => "accounts/terms", :action => "show" } + ) + assert_routing( + { :path => "/account/terms", :method => :put }, + { :controller => "accounts/terms", :action => "update" } + ) + + get "/user/terms" + assert_redirected_to "/account/terms" + end + + def test_show_not_logged_in + get account_terms_path + + assert_redirected_to login_path(:referer => account_terms_path) + end + + def test_show_agreed + user = create(:user, :terms_seen => true, :terms_agreed => Date.yesterday) + session_for(user) + + get account_terms_path + assert_redirected_to edit_account_path + end + + def test_show_not_seen_without_referer + user = create(:user, :terms_seen => false, :terms_agreed => nil) + session_for(user) + + get account_terms_path + assert_response :success + end + + def test_show_not_seen_with_referer + user = create(:user, :terms_seen => false, :terms_agreed => nil) + session_for(user) + + get account_terms_path(:referer => "/test") + assert_response :success + end + + def test_update_not_seen_without_referer + user = create(:user, :terms_seen => false, :terms_agreed => nil) + session_for(user) + + put account_terms_path, :params => { :user => { :consider_pd => true }, :read_ct => 1, :read_tou => 1 } + assert_redirected_to edit_account_path + assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] + + user.reload + + assert user.consider_pd + assert_not_nil user.terms_agreed + assert user.terms_seen + end + + def test_update_not_seen_with_referer + user = create(:user, :terms_seen => false, :terms_agreed => nil) + session_for(user) + + put account_terms_path, :params => { :user => { :consider_pd => true }, :referer => "/test", :read_ct => 1, :read_tou => 1 } + assert_redirected_to "/test" + assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] + + user.reload + + assert user.consider_pd + assert_not_nil user.terms_agreed + assert user.terms_seen + end + + # Check that if you haven't seen the terms, and make a request that requires authentication, + # that your request is redirected to view the terms + def test_terms_not_seen_redirection + user = create(:user, :terms_seen => false, :terms_agreed => nil) + session_for(user) + + get edit_account_path + assert_redirected_to account_terms_path(:referer => "/account/edit") + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index e021513da6..9ffa6695d9 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -14,16 +14,6 @@ def test_routes { :controller => "users", :action => "create" } ) - assert_routing( - { :path => "/user/terms", :method => :get }, - { :controller => "users", :action => "terms" } - ) - - assert_routing( - { :path => "/user/save", :method => :post }, - { :controller => "users", :action => "save" } - ) - assert_routing( { :path => "/user/go_public", :method => :post }, { :controller => "users", :action => "go_public" } @@ -212,71 +202,6 @@ def test_create_referer_params ActionMailer::Base.deliveries.clear end - def test_terms_agreed - user = create(:user, :terms_seen => true, :terms_agreed => Date.yesterday) - - session_for(user) - - get user_terms_path - assert_redirected_to edit_account_path - end - - def test_terms_not_seen_without_referer - user = create(:user, :terms_seen => false, :terms_agreed => nil) - - session_for(user) - - get user_terms_path - assert_response :success - assert_template :terms - - post user_save_path, :params => { :user => { :consider_pd => true }, :read_ct => 1, :read_tou => 1 } - assert_redirected_to edit_account_path - assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] - - user.reload - - assert user.consider_pd - assert_not_nil user.terms_agreed - assert user.terms_seen - end - - def test_terms_not_seen_with_referer - user = create(:user, :terms_seen => false, :terms_agreed => nil) - - session_for(user) - - get user_terms_path, :params => { :referer => "/test" } - assert_response :success - assert_template :terms - - post user_save_path, :params => { :user => { :consider_pd => true }, :referer => "/test", :read_ct => 1, :read_tou => 1 } - assert_redirected_to "/test" - assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] - - user.reload - - assert user.consider_pd - assert_not_nil user.terms_agreed - assert user.terms_seen - end - - # Check that if you haven't seen the terms, and make a request that requires authentication, - # that your request is redirected to view the terms - def test_terms_not_seen_redirection - user = create(:user, :terms_seen => false, :terms_agreed => nil) - session_for(user) - - get edit_account_path - assert_redirected_to :controller => :users, :action => :terms, :referer => "/account/edit" - end - - def test_terms_not_logged_in - get user_terms_path - - assert_redirected_to login_path(:referer => "/user/terms") - end - def test_go_public user = create(:user, :data_public => false) session_for(user) diff --git a/test/integration/user_terms_seen_test.rb b/test/integration/user_terms_seen_test.rb index d419003d9b..76d384fff2 100644 --- a/test/integration/user_terms_seen_test.rb +++ b/test/integration/user_terms_seen_test.rb @@ -25,12 +25,12 @@ def test_terms_presented_at_login assert_template "sessions/new" post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" } # but now we need to look at the terms - assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new" + assert_redirected_to "/account/terms?#{{ :referer => '/diary/new' }.to_query}" follow_redirect! assert_response :success # don't agree to the terms, but hit decline - post "/user/save", :params => { :decline => true, :referer => "/diary/new" } + put "/account/terms", :params => { :decline => true, :referer => "/diary/new" } assert_redirected_to "/diary/new" follow_redirect! @@ -49,13 +49,13 @@ def test_terms_cant_be_circumvented assert_template "sessions/new" post "/login", :params => { :username => user.email, :password => "test", :referer => "/diary/new" } # but now we need to look at the terms - assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new" + assert_redirected_to "/account/terms?#{{ :referer => '/diary/new' }.to_query}" # check that if we go somewhere else now, it redirects # back to the terms page. get "/traces/mine" - assert_redirected_to :controller => :users, :action => :terms, :referer => "/traces/mine" + assert_redirected_to "/account/terms?#{{ :referer => '/traces/mine' }.to_query}" get "/traces/mine", :params => { :referer => "/diary/new" } - assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new" + assert_redirected_to "/account/terms?#{{ :referer => '/diary/new' }.to_query}" end end