Merge 2fc4e3f37c into 432fa57e61

app/controllers/accounts/terms_controller.rb

@@ -18,11 +18,6 @@ module Accounts
         render :partial => "terms"
       else
         @title = t ".title"
-
-        if current_user.terms_agreed?
-          # Already agreed to terms, so just show settings
-          redirect_to account_path
-        end
       end
     end
 
@@ -32,13 +27,11 @@ module Accounts
 
         flash[:notice] = { :partial => "accounts/terms/terms_declined_flash" } if current_user.save
       else
-        unless current_user.terms_agreed?
+        current_user.tou_agreed = Time.now.utc
-          current_user.tou_agreed = Time.now.utc
+        current_user.terms_agreed = Time.now.utc
-          current_user.terms_agreed = Time.now.utc
+        current_user.terms_seen = true
-          current_user.terms_seen = true
 
-          flash[:notice] = t ".terms accepted" if current_user.save
+        flash[:notice] = t ".terms accepted" if current_user.save
-        end
       end
 
       referer = safe_referer(params[:referer]) if params[:referer]

app/views/accounts/show.html.erb

@@ -29,26 +29,6 @@
     <small class="form-text text-body-secondary">(<a href="<%= t ".openid.link" %>" target="_new"><%= t ".openid.link text" %></a>)</small>
   </fieldset>
 
-  <div class="mb-3 d-flex flex-column flex-sm-row column-gap-1">
-    <label class="form-label text-nowrap mb-0"><%= t ".contributor terms.heading" %></label>
-    <span class="form-text text-body-secondary">
-      <% if current_user.terms_agreed? %>
-        <%= t ".contributor terms.agreed" %>
-        (<a href="<%= t ".contributor terms.link" %>" target="_new"><%= t ".contributor terms.link text" %></a>)
-        <br>
-        <% if current_user.consider_pd? %>
-          <%= t ".contributor terms.agreed_with_pd" %>
-        <% else %>
-          <%= t ".contributor terms.not_agreed_with_pd" %>
-          (<%= link_to t(".contributor terms.pd_link_text"), account_pd_declaration_path %>)
-        <% end %>
-      <% else %>
-        <%= t ".contributor terms.not yet agreed" %>
-        <%= link_to t(".contributor terms.review link text"), account_terms_path %>
-      <% end %>
-    </span>
-  </div>
-
   <div class="row justify-content-between g-1">
     <div class="col-auto">
       <%= f.primary t(".save changes button") %>
@@ -57,8 +37,49 @@
       <%= link_to t(".delete_account"), account_deletion_path, :class => "btn btn-outline-danger" %>
     </div>
   </div>
+
 <% end %>
 
+<hr>
+
+<div class="row">
+  <section class="col-md-6 mb-3">
+    <h2 class="fs-6 fw-normal"><%= t ".contributor_terms.heading" %></h2>
+    <div class="small text-body-secondary">
+      <% if current_user.terms_agreed? %>
+        <%= t ".contributor_terms.agreed", :date => l(@current_user.terms_agreed.to_date, :format => :long) %>
+      <% else %>
+        <%= t ".contributor_terms.not_agreed" %>
+      <% end %>
+      <br>
+      <% if current_user.consider_pd? %>
+        <%= t ".contributor_terms.agreed_with_pd" %>
+      <% else %>
+        <%= t ".contributor_terms.not_agreed_with_pd" %>
+      <% end %>
+      <br class="mb-1">
+      <%= link_to current_user.terms_agreed? ? t(".contributor_terms.review") : t(".contributor_terms.review_and_accept"), account_terms_path %>
+      <% unless current_user.consider_pd? %>
+        &middot;
+        <%= link_to t(".contributor_terms.consider_pd"), account_pd_declaration_path %>
+      <% end %>
+    </div>
+  </section>
+
+  <section class="col-md-6 mb-3">
+    <h2 class="fs-6 fw-normal"><%= t ".terms_of_use.heading" %></h2>
+    <div class="small text-body-secondary">
+      <% if current_user.tou_agreed? %>
+        <%= t ".terms_of_use.agreed", :date => l(@current_user.tou_agreed.to_date, :format => :long) %>
+      <% else %>
+        <%= t ".terms_of_use.not_agreed" %>
+      <% end %>
+      <br class="mb-1">
+      <%= link_to current_user.tou_agreed? ? t(".terms_of_use.review") : t(".terms_of_use.review_and_accept"), account_terms_path %>
+    </div>
+  </section>
+</div>
+
 <% unless current_user.data_public? %>
   <%= render :partial => "go_public" %>
 <% end %>

config/locales/en.yml

@@ -252,16 +252,21 @@ en:
       openid:
         link: "https://wiki.openstreetmap.org/wiki/OpenID"
         link text: "what is this?"
-      contributor terms:
+      contributor_terms:
         heading: "Contributor Terms"
-        agreed: "You have agreed to the new Contributor Terms."
+        agreed: "You have agreed to the Contributor Terms on %{date}."
-        not yet agreed: "You have not yet agreed to the new Contributor Terms."
+        not_agreed: "You have not yet agreed to the Contributor Terms."
-        review link text: "Please follow this link at your convenience to review and accept the new Contributor Terms."
         agreed_with_pd: "You have also declared that you consider your edits to be in the Public Domain."
-        link: "https://osmfoundation.org/wiki/Licence/Contributor_Terms"
-        link text: "what is this?"
         not_agreed_with_pd: "You haven't declared that you consider your edits to be in the Public Domain."
-        pd_link_text: "declare"
+        review: "Review the Terms"
+        review_and_accept: "Review and accept the Terms"
+        consider_pd: "Consider Public Domain"
+      terms_of_use:
+        heading: "Terms of Use"
+        agreed: "You have agreed to the Terms of Use on %{date}."
+        not_agreed: "You have not yet agreed to the Terms of Use."
+        review: "Review the Terms"
+        review_and_accept: "Review and accept the Terms"
       save changes button: Save Changes
       delete_account: Delete Account...
     go_public:

test/controllers/accounts/terms_controller_test.rb

@@ -29,7 +29,7 @@ module Accounts
       session_for(user)
 
       get account_terms_path
-      assert_redirected_to account_path
+      assert_response :success
     end
 
     def test_show_not_seen_without_referer
@@ -48,32 +48,72 @@ module Accounts
       assert_response :success
     end
 
-    def test_update_not_seen_without_referer
+    def test_update_decline
-      user = create(:user, :terms_seen => false, :terms_agreed => nil)
+      user = create(:user, :terms_seen => false, :terms_agreed => nil, :tou_agreed => nil)
       session_for(user)
 
-      put account_terms_path, :params => { :read_ct => 1, :read_tou => 1 }
+      freeze_time do
-      assert_redirected_to account_path
+        put account_terms_path
-      assert_equal "Thanks for accepting the new contributor terms!", flash[:notice]
 
-      user.reload
+        assert_redirected_to account_path
+        assert_equal({ :partial => "accounts/terms/terms_declined_flash" }, flash[:notice])
 
-      assert_not_nil user.terms_agreed
+        user.reload
-      assert user.terms_seen
+        assert user.terms_seen
+        assert_nil user.terms_agreed
+        assert_nil user.tou_agreed
+      end
+    end
+
+    def test_update_not_seen
+      user = create(:user, :terms_seen => false, :terms_agreed => nil, :tou_agreed => nil)
+      session_for(user)
+
+      freeze_time do
+        put account_terms_path, :params => { :read_ct => 1, :read_tou => 1 }
+
+        assert_redirected_to account_path
+        assert_equal "Thanks for accepting the new contributor terms!", flash[:notice]
+
+        user.reload
+        assert user.terms_seen
+        assert_equal Time.now.utc, user.terms_agreed
+        assert_equal Time.now.utc, user.tou_agreed
+      end
     end
 
     def test_update_not_seen_with_referer
-      user = create(:user, :terms_seen => false, :terms_agreed => nil)
+      user = create(:user, :terms_seen => false, :terms_agreed => nil, :tou_agreed => nil)
       session_for(user)
 
-      put account_terms_path, :params => { :referer => "/test", :read_ct => 1, :read_tou => 1 }
+      freeze_time do
-      assert_redirected_to "/test"
+        put account_terms_path, :params => { :referer => "/test", :read_ct => 1, :read_tou => 1 }
-      assert_equal "Thanks for accepting the new contributor terms!", flash[:notice]
 
-      user.reload
+        assert_redirected_to "/test"
+        assert_equal "Thanks for accepting the new contributor terms!", flash[:notice]
 
-      assert_not_nil user.terms_agreed
+        user.reload
-      assert user.terms_seen
+        assert user.terms_seen
+        assert_equal Time.now.utc, user.terms_agreed
+        assert_equal Time.now.utc, user.tou_agreed
+      end
+    end
+
+    def test_update_previously_accepted
+      user = create(:user, :terms_seen => true, :terms_agreed => Date.yesterday, :tou_agreed => Date.yesterday)
+      session_for(user)
+
+      freeze_time do
+        put account_terms_path, :params => { :read_ct => 1, :read_tou => 1 }
+
+        assert_redirected_to account_path
+        assert_equal "Thanks for accepting the new contributor terms!", flash[:notice]
+
+        user.reload
+        assert user.terms_seen
+        assert_equal Time.now.utc, user.terms_agreed
+        assert_equal Time.now.utc, user.tou_agreed
+      end
     end
 
     # Check that if you haven't seen the terms, and make a request that requires authentication,

test/system/account_terms_test.rb

@@ -0,0 +1,83 @@
+require "application_system_test_case"
+
+class AccountTermsTest < ApplicationSystemTestCase
+  test "should inform about terms if not agreed" do
+    user = create(:user, :terms_seen => true, :terms_agreed => nil, :tou_agreed => nil)
+
+    sign_in_as(user)
+    visit account_path
+
+    within_content_body do
+      assert_text(/You have not yet agreed to.*Contributor Terms/)
+      assert_text(/You have not yet agreed to.*Terms of Use/)
+      assert_link "Review and accept the Terms"
+
+      click_on "Review and accept the Terms", :match => :first
+    end
+
+    assert_current_path account_terms_path
+  end
+
+  test "should inform about terms if partially agreed" do
+    user = create(:user, :terms_seen => true, :terms_agreed => "2022-03-14", :tou_agreed => nil)
+
+    sign_in_as(user)
+    visit account_path
+
+    within_content_body do
+      assert_text(/You have agreed to.*Contributor Terms.*March 14, 2022/)
+      assert_text(/You have not yet agreed to.*Terms of Use/)
+      assert_link "Review the Terms"
+      assert_link "Review and accept the Terms"
+
+      click_on "Review and accept the Terms", :match => :first
+    end
+
+    assert_current_path account_terms_path
+  end
+
+  test "should inform about terms if agreed" do
+    user = create(:user, :terms_seen => true, :terms_agreed => "2023-04-15", :tou_agreed => "2024-05-16")
+
+    sign_in_as(user)
+    visit account_path
+
+    within_content_body do
+      assert_text(/You have agreed to.*Contributor Terms.*April 15, 2023/)
+      assert_text(/You have agreed to.*Terms of Use.*May 16, 2024/)
+      assert_link "Review the Terms"
+
+      click_on "Review the Terms", :match => :first
+    end
+
+    assert_current_path account_terms_path
+  end
+
+  test "should ask to consider pd if not considered" do
+    user = create(:user, :consider_pd => false)
+
+    sign_in_as(user)
+    visit account_path
+
+    within_content_body do
+      assert_text(/You haven't declared.*Public Domain/)
+      assert_link "Consider Public Domain"
+
+      click_on "Consider Public Domain"
+    end
+
+    assert_current_path account_pd_declaration_path
+  end
+
+  test "should not ask to consider pd if considered" do
+    user = create(:user, :consider_pd => true)
+
+    sign_in_as(user)
+    visit account_path
+
+    within_content_body do
+      assert_text(/You have also declared.*Public Domain/)
+      assert_no_link "Consider Public Domain"
+    end
+  end
+end