From 051cab6ff774cd1811952fd739e4bc14b0535f56 Mon Sep 17 00:00:00 2001 From: Mohamed El-Sawy Date: Mon, 21 Oct 2024 21:45:42 +0300 Subject: [PATCH] CV2-5388: Improve ToS and Privacy Policy UX (#2098) * CV2-5388: add flag for terms_last_updated_at notification * CV2-5388: apply PR comment and fix tests * CV2-5388: update the mails notification flag * CV2-5388: disable dialog for google signin --- app/models/concerns/user_multi_auth_login.rb | 1 + app/models/user.rb | 25 ++--------------- app/workers/terms_of_service_update_worker.rb | 2 +- test/models/user_test.rb | 28 +++---------------- .../terms_of_service_update_worker_test.rb | 7 ++++- 5 files changed, 14 insertions(+), 49 deletions(-) diff --git a/app/models/concerns/user_multi_auth_login.rb b/app/models/concerns/user_multi_auth_login.rb index bc9f2d83a9..146d8283de 100644 --- a/app/models/concerns/user_multi_auth_login.rb +++ b/app/models/concerns/user_multi_auth_login.rb @@ -46,6 +46,7 @@ def self.create_omniauth_user(u, auth) user.login = auth.info.nickname || auth.info.name.tr(' ', '-').downcase user.from_omniauth_login = true user.skip_confirmation! + user.last_accepted_terms_at = Time.now if user.last_accepted_terms_at.nil? User.current = user user.save! user.confirm unless user.is_confirmed? diff --git a/app/models/user.rb b/app/models/user.rb index 94167341d6..3620a4e314 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -267,30 +267,9 @@ def accepted_terms self.last_accepted_terms_at.to_i > User.terms_last_updated_at end - def self.terms_last_updated_at_by_page(page) - mapping = { - tos: 'tos_url', - tos_smooch: 'tos_smooch_url', - privacy_policy: 'privacy_policy_url' - }.with_indifferent_access - return 0 unless mapping.has_key?(page) - Rails.cache.fetch("last_updated_at:#{page}", expires_in: 24.hours) do - html = URI(CheckConfig.get(mapping[page], nil, :json)).open(read_timeout: 5, open_timeout: 5).read - date = ActionController::Base.helpers.strip_tags(html).match(/Last modified(?(Jan(?:uary)?|Feb(?:ruary)?|Mar(?:ch)?|Apr(?:il)?|May|Jun(?:e)?|Jul(?:y)?|Aug(?:ust)?|Sep(?:tember)?|Oct(?:ober)?|Nov(?:ember)?|Dec(?:ember)?)\s\d{1,2},\s\d{2,4})/)["modified_date"] - Time.parse(date).to_i - end - end - def self.terms_last_updated_at - begin - tos = User.terms_last_updated_at_by_page(:tos) - pp = User.terms_last_updated_at_by_page(:privacy_policy) - tos > pp ? tos : pp - rescue StandardError => e - error = ToSOrPrivacyPolicyReadError.new(e) - CheckSentry.notify(error) - 0 - end + # Cached value should be an integer timestamp i.e Time.now.to_i + Rails.cache.read('terms_last_updated_at').to_i || 0 end def self.delete_check_user(user) diff --git a/app/workers/terms_of_service_update_worker.rb b/app/workers/terms_of_service_update_worker.rb index 451ae8022f..f3cd1c500c 100644 --- a/app/workers/terms_of_service_update_worker.rb +++ b/app/workers/terms_of_service_update_worker.rb @@ -4,7 +4,7 @@ class TermsOfServiceUpdateWorker sidekiq_options queue: 'terms_mailer', retry: 0 def perform - last_updated = Time.at(User.terms_last_updated_at) + last_updated = Time.at(Rails.cache.read('terms_last_updated_at_notification').to_i) updated_time = Time.now # Based on our AWS SES account (Maximum send rate: 200 emails per second) I set a batch size 200 and do a sleep 1 User.where(is_active: true).where('email IS NOT NULL') diff --git a/test/models/user_test.rb b/test/models/user_test.rb index ef31bcf3b7..112005e931 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -791,35 +791,15 @@ def setup end test "should return the last time that the terms were updated" do - assert User.terms_last_updated_at > 0 - end - - test "should return the last time that the terms of service were updated" do - assert User.terms_last_updated_at_by_page(:tos) > 0 - end - - test "should return the last time that the terms of privacy were updated" do - assert User.terms_last_updated_at_by_page(:privacy_policy) > 0 - end - - test "should return the last time that invalid terms were updated" do - assert_equal 0, User.terms_last_updated_at_by_page(:invalid) - end - - test "should not crash but notify if could not get the last time that the terms were updated" do - stub_configs({ 'tos_url' => 'invalid-tos-url' }) do - assert_nothing_raised do - assert_equal 0, User.terms_last_updated_at - end - end + assert_equal 0, User.terms_last_updated_at + time_now = Time.now.to_i + Rails.cache.write('terms_last_updated_at', time_now) + assert_equal time_now, User.terms_last_updated_at end test "should return if user accepted terms" do u = create_user assert !u.reload.accepted_terms - u.last_accepted_terms_at = Time.parse('2018-08-01') - u.save! - assert !u.reload.accepted_terms u.last_accepted_terms_at = Time.now u.save! assert u.reload.accepted_terms diff --git a/test/workers/terms_of_service_update_worker_test.rb b/test/workers/terms_of_service_update_worker_test.rb index 5527ef9cbf..b205594178 100644 --- a/test/workers/terms_of_service_update_worker_test.rb +++ b/test/workers/terms_of_service_update_worker_test.rb @@ -1,12 +1,16 @@ require_relative '../test_helper' class TermsOfServiceUpdateWorkerTest < ActiveSupport::TestCase - test "should notify users based on last term update" do + test "should notify users based on terms_last_updated_at_notification flag" do User.stubs(:terms_last_updated_at).returns(Time.now.to_i) terms_update = Time.now - 1.day u = create_user u.last_received_terms_email_at = terms_update u.save! + last_received_terms_email_at = u.reload.last_received_terms_email_at + TermsOfServiceUpdateWorker.new.perform + assert_equal last_received_terms_email_at, u.reload.last_received_terms_email_at + Rails.cache.write('terms_last_updated_at_notification', Time.now.to_i) TermsOfServiceUpdateWorker.new.perform last_term_update = u.reload.last_received_terms_email_at assert last_term_update > terms_update @@ -15,6 +19,7 @@ class TermsOfServiceUpdateWorkerTest < ActiveSupport::TestCase end test "should notify users in background" do + Rails.cache.write('terms_last_updated_at_notification', Time.now.to_i) User.stubs(:terms_last_updated_at).returns(Time.now.to_i) terms_update = Time.now - 1.day u = create_user