Remove require_terms_agreed configuration option

This has been set to true for 6 years in production. Refs #2097

As per other user settings, we set the terms as seen by default for tests,
and we can override that when necessary for specific tests.

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index c4cca7cc5f0474ebefacd834d226ce0aedc9b24b..4127c0de796bbac56a16f08c93bd515f85295784 100644 (file)
--- a/app/abilities/ability.rb
+++ b/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

diff --git a/app/abilities/capability.rb b/app/abilities/capability.rb
index 3d951900be11c07b638b8410b5bc26be95e64ab6..d8f51eefea7562e723fb5a3ac47005a994d77f8a 100644 (file)
--- a/app/abilities/capability.rb
+++ b/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)

diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index fdad432a8d44802c592e309f54e95d0d9a334f4c..2ad0fe6e06b7a8930eab0b3d359c5d5a6a8cfbd4 100644 (file)
--- a/app/controllers/amf_controller.rb
+++ b/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 = {}

diff --git a/config/example.application.yml b/config/example.application.yml
index bf9ebeb59b789f69c1b09b48c60412af73450d58..8596e10cc26fa8fe16c8850f3950f5673e3e8f0a 100644 (file)
--- a/config/example.application.yml
+++ b/config/example.application.yml
@@ -87,8 +87,6 @@ defaults: &defaults
   #oauth_key: ""
   # OAuth consumer key for iD
   #id_key: ""
-  # 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

diff --git a/test/controllers/changeset_comments_controller_test.rb b/test/controllers/changeset_comments_controller_test.rb
index 2b661a7a9bf4be795454bb06636c99661351f464..33ee1deb522f6ea492d93e3d8d7410e7398740d6 100644 (file)
--- a/test/controllers/changeset_comments_controller_test.rb
+++ b/test/controllers/changeset_comments_controller_test.rb
@@ -255,66 +255,50 @@ 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)
-
-      # Hack together an oauth request - an alternative would be to sign the request properly
-      @request.env["oauth.version"] = 1
-      @request.env["oauth.strategies"] = [:token]
-      @request.env["oauth.token"] = token
-
-      assert_difference "ChangesetComment.count", 0 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :forbidden
+    user = create(:user, :terms_agreed => nil)
+    token = create(:access_token, :user => user, :allow_write_api => true)
+    changeset = create(:changeset, :closed)
 
-      # Try again, after agreement with the terms
-      user.terms_agreed = Time.now
-      user.save!
+    # Hack together an oauth request - an alternative would be to sign the request properly
+    @request.env["oauth.version"] = 1
+    @request.env["oauth.strategies"] = [:token]
+    @request.env["oauth.token"] = token
 
-      assert_difference "ChangesetComment.count", 1 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :success
+    assert_difference "ChangesetComment.count", 0 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
     end
+    assert_response :forbidden
+
+    # Try again, after agreement with the terms
+    user.terms_agreed = Time.now
+    user.save!
+
+    assert_difference "ChangesetComment.count", 1 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
+    end
+    assert_response :success
   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)
-
-      basic_authorization user.email, "test"
-
-      assert_difference "ChangesetComment.count", 0 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :forbidden
+    user = create(:user, :terms_agreed => nil)
+    changeset = create(:changeset, :closed)
 
-      # Try again, after agreement with the terms
-      user.terms_agreed = Time.now
-      user.save!
+    basic_authorization user.email, "test"
 
-      assert_difference "ChangesetComment.count", 1 do
-        post :create, :params => { :id => changeset.id, :text => "This is a comment" }
-      end
-      assert_response :success
+    assert_difference "ChangesetComment.count", 0 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
     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)
+    assert_response :forbidden
 
-    yield
+    # Try again, after agreement with the terms
+    user.terms_agreed = Time.now
+    user.save!
 
-    Object.send("remove_const", "REQUIRE_TERMS_AGREED")
-    Object.const_set("REQUIRE_TERMS_AGREED", require_terms_agreed)
+    assert_difference "ChangesetComment.count", 1 do
+      post :create, :params => { :id => changeset.id, :text => "This is a comment" }
+    end
+    assert_response :success
   end
 end

diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index a1fda559137b32da8f8ace96f7a7ae01f4680d8e..b85dcf65b0cdcc93c3671a1a6509482c94a97e28 100644 (file)
--- a/test/controllers/users_controller_test.rb
+++ b/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

diff --git a/test/factories/user.rb b/test/factories/user.rb
index 69d5ba376471b2684b6fff8ba31cdb4e45af39c6..48a124adb7c0fe9872cf1513ebb37eca461ea8ef 100644 (file)
--- a/test/factories/user.rb
+++ b/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

diff --git a/test/integration/user_terms_seen_test.rb b/test/integration/user_terms_seen_test.rb
index 33a6950ee62378ebabd7ab02acb4a6947b50a939..416d226ccd472fde9c735df02990433229475864 100644 (file)
--- a/test/integration/user_terms_seen_test.rb
+++ b/test/integration/user_terms_seen_test.rb
@@ -6,7 +6,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   end
 
   def test_api_blocked
-    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
@@ -20,7 +20,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   end
 
   def test_terms_presented_at_login
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     # try to log in
     get "/login"
@@ -45,7 +45,7 @@ class UserTermsSeenTest < ActionDispatch::IntegrationTest
   end
 
   def test_terms_cant_be_circumvented
-    user = create(:user, :terms_seen => false)
+    user = create(:user, :terms_seen => false, :terms_agreed => nil)
 
     # try to log in
     get "/login"