From 619d4cdb95a5a00e32b04b6e33bc4510df923e7f Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 19 Feb 2025 06:28:12 +0300 Subject: [PATCH 1/3] Allow to revisit terms page if already agreed --- app/controllers/accounts/terms_controller.rb | 5 ----- test/controllers/accounts/terms_controller_test.rb | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/controllers/accounts/terms_controller.rb b/app/controllers/accounts/terms_controller.rb index 568abcdb9..6fe498566 100644 --- a/app/controllers/accounts/terms_controller.rb +++ b/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 diff --git a/test/controllers/accounts/terms_controller_test.rb b/test/controllers/accounts/terms_controller_test.rb index ea8cd4e9f..e45c3c3bd 100644 --- a/test/controllers/accounts/terms_controller_test.rb +++ b/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 From 94e7c39bddf0448cdae9dd2c3fd9e1b9196a2f8e Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 19 Feb 2025 06:50:29 +0300 Subject: [PATCH 2/3] Allow to agree to new terms if already agreed to old terms --- app/controllers/accounts/terms_controller.rb | 10 ++- .../accounts/terms_controller_test.rb | 70 +++++++++++++++---- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/app/controllers/accounts/terms_controller.rb b/app/controllers/accounts/terms_controller.rb index 6fe498566..de8b8dbe7 100644 --- a/app/controllers/accounts/terms_controller.rb +++ b/app/controllers/accounts/terms_controller.rb @@ -27,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.terms_agreed = Time.now.utc - current_user.terms_seen = true + current_user.tou_agreed = Time.now.utc + current_user.terms_agreed = Time.now.utc + current_user.terms_seen = true - flash[:notice] = t ".terms accepted" if current_user.save - end + flash[:notice] = t ".terms accepted" if current_user.save end referer = safe_referer(params[:referer]) if params[:referer] diff --git a/test/controllers/accounts/terms_controller_test.rb b/test/controllers/accounts/terms_controller_test.rb index e45c3c3bd..4e9fbed25 100644 --- a/test/controllers/accounts/terms_controller_test.rb +++ b/test/controllers/accounts/terms_controller_test.rb @@ -48,32 +48,72 @@ module Accounts assert_response :success end - def test_update_not_seen_without_referer - user = create(:user, :terms_seen => false, :terms_agreed => nil) + def test_update_decline + 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 } - assert_redirected_to account_path - assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] + freeze_time do + put account_terms_path - user.reload + assert_redirected_to account_path + assert_equal({ :partial => "accounts/terms/terms_declined_flash" }, flash[:notice]) - assert_not_nil user.terms_agreed - assert user.terms_seen + user.reload + 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 } - assert_redirected_to "/test" - assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] + freeze_time do + put account_terms_path, :params => { :referer => "/test", :read_ct => 1, :read_tou => 1 } - user.reload + assert_redirected_to "/test" + assert_equal "Thanks for accepting the new contributor terms!", flash[:notice] - assert_not_nil user.terms_agreed - assert user.terms_seen + 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_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, From 2fc4e3f37c0280af07da1863ccc1dc782b65a08d Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Wed, 19 Feb 2025 07:48:07 +0300 Subject: [PATCH 3/3] Show terms acceptance dates and link to terms on account page --- app/views/accounts/show.html.erb | 61 +++++++++++++++-------- config/locales/en.yml | 19 ++++--- test/system/account_terms_test.rb | 83 +++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 27 deletions(-) create mode 100644 test/system/account_terms_test.rb diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 5c626fc9f..1643a8adf 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -29,26 +29,6 @@ (" target="_new"><%= t ".openid.link text" %>) -
- - - <% if current_user.terms_agreed? %> - <%= t ".contributor terms.agreed" %> - (" target="_new"><%= t ".contributor terms.link text" %>) -
- <% 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 %> - -
-
<%= f.primary t(".save changes button") %> @@ -57,8 +37,49 @@ <%= link_to t(".delete_account"), account_deletion_path, :class => "btn btn-outline-danger" %>
+ <% end %> +
+ +
+
+

<%= t ".contributor_terms.heading" %>

+
+ <% 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 %> +
+ <% if current_user.consider_pd? %> + <%= t ".contributor_terms.agreed_with_pd" %> + <% else %> + <%= t ".contributor_terms.not_agreed_with_pd" %> + <% end %> +
+ <%= link_to current_user.terms_agreed? ? t(".contributor_terms.review") : t(".contributor_terms.review_and_accept"), account_terms_path %> + <% unless current_user.consider_pd? %> + · + <%= link_to t(".contributor_terms.consider_pd"), account_pd_declaration_path %> + <% end %> +
+
+ +
+

<%= t ".terms_of_use.heading" %>

+
+ <% 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 %> +
+ <%= link_to current_user.tou_agreed? ? t(".terms_of_use.review") : t(".terms_of_use.review_and_accept"), account_terms_path %> +
+
+
+ <% unless current_user.data_public? %> <%= render :partial => "go_public" %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index f03a6eeb1..fa0ef1d60 100644 --- a/config/locales/en.yml +++ b/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." - not yet agreed: "You have not yet agreed to the new Contributor Terms." - review link text: "Please follow this link at your convenience to review and accept the new Contributor Terms." + agreed: "You have agreed to the Contributor Terms on %{date}." + not_agreed: "You have not yet agreed to the 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: diff --git a/test/system/account_terms_test.rb b/test/system/account_terms_test.rb new file mode 100644 index 000000000..35b2f9193 --- /dev/null +++ b/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