Merge pull request #5462 from AntonKhorev/user-terms-resource

Use resourceful routes for terms view/accept/decline

app/abilities/ability.rb

@@ -23,7 +23,8 @@ class Ability
       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
 

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;

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

app/controllers/application_controller.rb

@@ -56,11 +56,11 @@ class ApplicationController < ActionController::Base
       # 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

app/controllers/concerns/session_methods.rb

@@ -48,7 +48,7 @@ module SessionMethods
     # - 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

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 @@ class UsersController < ApplicationController
     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

app/helpers/application_helper.rb

@@ -37,7 +37,8 @@ module ApplicationHelper
     if content_for? :body_class
       content_for :body_class
     else
-      "#{params[:controller]} #{params[:controller]}-#{params[:action]}"
+      controller_part = params[:controller].tr("/", "-")
+      "#{controller_part} #{controller_part}-#{params[:action]}"
     end
   end
 

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 %>
     </span>
   </div>

app/views/accounts/terms/show.html.erb

@@ -9,7 +9,7 @@
   </div>
 <% end %>
 
-<%= form_tag({ :action => "save" }) do %>
+<%= form_tag account_terms_path, :method => :put do %>
   <!-- legale is <%= @legale %> -->
   <p class="text-body-secondary"><%= t ".read and accept with tou" %></p>
   <h4>

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"

config/routes.rb

@@ -182,8 +182,6 @@ OpenStreetMap::Application.routes.draw do
   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 @@ OpenStreetMap::Application.routes.draw do
   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 @@ OpenStreetMap::Application.routes.draw do
   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]

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

test/controllers/users_controller_test.rb

@@ -14,16 +14,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
       { :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 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     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)

test/integration/user_terms_seen_test.rb

@@ -25,12 +25,12 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
     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_path(:referer => "/diary/new")
     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 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
     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_path(:referer => "/diary/new")
 
     # 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_path(:referer => "/traces/mine")
     get "/traces/mine", :params => { :referer => "/diary/new" }
-    assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
+    assert_redirected_to account_terms_path(:referer => "/diary/new")
   end
 end