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

app/abilities/ability.rb

@@ -42,7 +42,7 @@ class Ability
       can [:account, :go_public, :make_friend, :remove_friend, :api_details, :api_gpx_files], User
       can [:read, :read_one, :update, :update_one, :delete_one], UserPreference
 
-      if user.terms_agreed? || !REQUIRE_TERMS_AGREED
+      if user.terms_agreed?
         can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset
         can :create, ChangesetComment
         can [:create, :update, :delete], Node
@@ -57,7 +57,7 @@ class Ability
         can :destroy, Note
         can [:new, :create, :edit, :update, :destroy], Redaction
         can [:new, :edit, :create, :update, :revoke], UserBlock
-        if user.terms_agreed? || !REQUIRE_TERMS_AGREED
+        if user.terms_agreed?
           can :redact, OldNode
           can :redact, OldWay
           can :redact, OldRelation

app/abilities/capability.rb

@@ -12,7 +12,7 @@ class Capability
     can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs)
     can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs)
 
-    if token&.user&.terms_agreed? || !REQUIRE_TERMS_AGREED
+    if token&.user&.terms_agreed?
       can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset if capability?(token, :allow_write_api)
       can :create, ChangesetComment if capability?(token, :allow_write_api)
       can [:create, :update, :delete], Node if capability?(token, :allow_write_api)
@@ -23,7 +23,7 @@ class Capability
     if token&.user&.moderator?
       can [:destroy, :restore], ChangesetComment if capability?(token, :allow_write_api)
       can :destroy, Note if capability?(token, :allow_write_notes)
-      if token&.user&.terms_agreed? || !REQUIRE_TERMS_AGREED
+      if token&.user&.terms_agreed?
         can :redact, OldNode if capability?(token, :allow_write_api)
         can :redact, OldWay if capability?(token, :allow_write_api)
         can :redact, OldRelation if capability?(token, :allow_write_api)

app/controllers/amf_controller.rb

@@ -144,7 +144,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so Potlatch can't write any changes to the database." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       if cstags
         return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(cstags)
@@ -537,7 +537,7 @@ class AmfController < ApplicationController
 
       return -1, "You are not logged in, so the relation could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(tags)
 
@@ -625,7 +625,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the way could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -2, "Server error - way is only #{pointlist.length} points long." if pointlist.length < 2
 
@@ -735,7 +735,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the point could not be saved." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       return -1, "One of the tags is invalid. Linux users may need to upgrade to Flash Player 10.1." unless tags_ok(tags)
 
@@ -822,7 +822,7 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       return -1, "You are not logged in, so the way could not be deleted." unless user
       return -1, t("application.setup_user_auth.blocked") if user.blocks.active.exists?
-      return -1, "You must accept the contributor terms before you can edit." if REQUIRE_TERMS_AGREED && user.terms_agreed.nil?
+      return -1, "You must accept the contributor terms before you can edit." if user.terms_agreed.nil?
 
       way_id = way_id.to_i
       nodeversions = {}

app/controllers/application_controller.rb

@@ -105,7 +105,7 @@ class ApplicationController < ActionController::Base
       # if the user hasn't seen the contributor terms then don't
       # allow editing - they have to go to the web site and see
       # (but can decline) the CTs to continue.
-      if REQUIRE_TERMS_SEEN && !current_user.terms_seen && flash[:skip_terms].nil?
+      if !current_user.terms_seen && flash[:skip_terms].nil?
         set_locale
         report_error t("application.setup_user_auth.need_to_see_terms"), :forbidden
       end

app/controllers/users_controller.rb

@@ -644,7 +644,7 @@ class UsersController < ApplicationController
     # - If they have a block on them, show them that.
     # - If they were referred to the login, send them back there.
     # - Otherwise, send them to the home page.
-    if REQUIRE_TERMS_SEEN && !user.terms_seen
+    if !user.terms_seen
       redirect_to :action => :terms, :referer => target
     elsif user.blocked_on_view
       redirect_to user.blocked_on_view, :referer => target

config/example.application.yml

@@ -87,10 +87,6 @@ defaults: &defaults
   #oauth_key: ""
   # OAuth consumer key for iD
   #id_key: ""
-  # Whether to require users to view the CTs before continuing to edit...
-  require_terms_seen: false
-  # Whether to require users to agree to the CTs before editing
-  require_terms_agreed: false
   # Imagery to return in capabilities as blacklisted
   imagery_blacklist:
     # Current Google imagery URLs have google or googleapis in the domain

test/controllers/changeset_comments_controller_test.rb

@@ -255,7 +255,6 @@ class ChangesetCommentsControllerTest < ActionController::TestCase
   # create method is simply a stand-in for any method that requires terms agreement.
   # But writing oauth tests is hard, and so it's easier to put in a controller test.)
   def test_api_write_and_terms_agreed_via_token
-    with_terms_agreed(true) do
     user = create(:user, :terms_agreed => nil)
     token = create(:access_token, :user => user, :allow_write_api => true)
     changeset = create(:changeset, :closed)
@@ -279,12 +278,10 @@ class ChangesetCommentsControllerTest < ActionController::TestCase
     end
     assert_response :success
   end
-  end
 
   # This test does the same as above, but with basic auth, to similarly test that the
   # abilities take into account terms agreement too.
   def test_api_write_and_terms_agreed_via_basic_auth
-    with_terms_agreed(true) do
     user = create(:user, :terms_agreed => nil)
     changeset = create(:changeset, :closed)
 
@@ -305,16 +302,3 @@ class ChangesetCommentsControllerTest < ActionController::TestCase
     assert_response :success
   end
 end
-
-  private
-
-  def with_terms_agreed(value)
-    require_terms_agreed = Object.send("remove_const", "REQUIRE_TERMS_AGREED")
-    Object.const_set("REQUIRE_TERMS_AGREED", value)
-
-    yield
-
-    Object.send("remove_const", "REQUIRE_TERMS_AGREED")
-    Object.const_set("REQUIRE_TERMS_AGREED", require_terms_agreed)
-  end
-end

test/controllers/users_controller_test.rb

@@ -646,7 +646,7 @@ class UsersControllerTest < ActionController::TestCase
   end
 
   def test_terms_not_seen_without_referer
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     session[:user] = user.id
 
@@ -667,7 +667,7 @@ class UsersControllerTest < ActionController::TestCase
   end
 
   def test_terms_not_seen_with_referer
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     session[:user] = user.id
 
@@ -690,7 +690,7 @@ class UsersControllerTest < ActionController::TestCase
   # 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)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
     session[:user] = user.id
 
     get :account, :params => { :display_name => user.display_name }
@@ -1098,8 +1098,8 @@ class UsersControllerTest < ActionController::TestCase
   # Test whether information about contributor terms is shown for users who haven't agreed
   def test_terms_not_agreed
     agreed_user = create(:user, :terms_agreed => 3.days.ago)
-    seen_user = create(:user, :terms_seen => true)
-    not_seen_user = create(:user, :terms_seen => false)
+    seen_user = create(:user, :terms_seen => true, :terms_agreed => nil)
+    not_seen_user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     get :show, :params => { :display_name => agreed_user.display_name }
     assert_response :success

test/factories/user.rb

@@ -8,6 +8,7 @@ FactoryBot.define do
     # a 'normal' user who can log in without being redirected etc.
     status { "active" }
     terms_seen { true }
+    terms_agreed { Time.now.getutc }
     data_public { true }
 
     trait :with_home_location do

test/integration/user_terms_seen_test.rb

@@ -6,8 +6,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   end
 
   def test_api_blocked
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
     assert_response :forbidden
@@ -19,11 +18,9 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
     get "/api/#{API_VERSION}/user/preferences", :headers => auth_header(user.display_name, "test")
     assert_response :success
   end
-  end
 
   def test_terms_presented_at_login
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     # try to log in
     get "/login"
@@ -46,11 +43,9 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_not flash[:notice].nil?
   end
-  end
 
   def test_terms_cant_be_circumvented
-    with_terms_seen(true) do
-      user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     # try to log in
     get "/login"
@@ -69,21 +64,10 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
     get "/traces/mine", :params => { :referer => "/diary/new" }
     assert_redirected_to :controller => :users, :action => :terms, :referer => "/diary/new"
   end
-  end
 
   private
 
   def auth_header(user, pass)
     { "HTTP_AUTHORIZATION" => format("Basic %{auth}", :auth => Base64.encode64("#{user}:#{pass}")) }
   end
-
-  def with_terms_seen(value)
-    require_terms_seen = Object.send("remove_const", "REQUIRE_TERMS_SEEN")
-    Object.const_set("REQUIRE_TERMS_SEEN", value)
-
-    yield
-
-    Object.send("remove_const", "REQUIRE_TERMS_SEEN")
-    Object.const_set("REQUIRE_TERMS_SEEN", require_terms_seen)
-  end
 end