Merge pull request #11396 from abdellani/fix-if-tos-is-not-set

fix If ToS file is not set, customer needs to accept Terms on each checkout

app/models/spree/app_configuration.rb

@@ -105,7 +105,6 @@ module Spree
     preference :embedded_shopfronts_whitelist, :text, default: nil
 
     # Legal Preferences
-    preference :footer_tos_url, :string, default: "/Terms-of-service.pdf"
     preference :enterprises_require_tos, :boolean, default: false
     preference :shoppers_require_tos, :boolean, default: false
     preference :privacy_policy_url, :string, default: nil

app/models/terms_of_service_file.rb

@@ -11,11 +11,7 @@ class TermsOfServiceFile < ApplicationRecord
   end
 
   def self.current_url
-    if current
-      Rails.application.routes.url_helpers.url_for(current.attachment)
-    else
-      Spree::Config.footer_tos_url
-    end
+    Rails.application.routes.url_helpers.url_for(current.attachment) if current
   end
 
   # If no file has been uploaded, we don't know when the old terms have

app/services/terms_of_service.rb

@@ -4,11 +4,11 @@ class TermsOfService
   def self.tos_accepted?(customer, distributor = nil)
     return false unless accepted_at = customer&.terms_and_conditions_accepted_at
 
-    accepted_at > if distributor
-                    distributor.terms_and_conditions_blob.created_at
-                  else
-                    TermsOfServiceFile.updated_at
-                  end
+    return accepted_at > distributor.terms_and_conditions_blob.created_at if distributor
+
+    return true unless TermsOfServiceFile.exists?
+
+    accepted_at > TermsOfServiceFile.updated_at
   end
 
   def self.required?(distributor)
@@ -16,7 +16,8 @@ class TermsOfService
   end
 
   def self.platform_terms_required?
-    Spree::Config.shoppers_require_tos
+    TermsOfServiceFile.exists? &&
+      Spree::Config.shoppers_require_tos
   end
 
   def self.distributor_terms_required?(distributor)

app/views/admin/terms_of_service_files/show.html.haml

@@ -11,7 +11,6 @@
   - else
     %p
       = t(".no_files")
-      = t(".using_default_terms_html", tos_link: link_to_platform_terms)
 
 = form_for [main_app, :admin, @new_file] do |f|
   = f.label :attachment, t(".attachment")

app/views/layouts/mailer.html.haml

@@ -43,8 +43,9 @@
               %tr
                 %td{:align => "center"}
                   %p
-                    = link_to_platform_terms
-                    |
+                    - if platform_terms_required?
+                      = link_to_platform_terms
+                      |
                     %a{:href => "#{ main_app.root_url }"}
                       = Spree::Config[:site_name]
                     / | <a href="#"><unsubscribe>Unsubscribe</unsubscribe></a>

app/views/shared/_footer.html.haml

@@ -103,9 +103,10 @@
             %img{src: ContentConfig.url_for(:footer_logo), width: "220"}
         .small-12.medium-5.columns.text-left
           %p.text-small
-            = t '.footer_legal_call'
-            = link_to_platform_terms
-            |
+            - if platform_terms_required?
+              = t '.footer_legal_call'
+              = link_to_platform_terms
+              |
             = t '.footer_legal_visit'
             %a{href:"https://github.com/openfoodfoundation/openfoodnetwork", target: "_blank"} GitHub
           %p.text-small

config/locales/en.yml

@@ -645,7 +645,6 @@ en:
       show:
         title: "Terms of Service files"
         no_files: "No terms of services have been uploaded yet."
-        using_default_terms_html: "The site is currently linking to your old %{tos_link}."
         current_terms_html: "View the current %{tos_link}. Upload time: %{datetime}."
         terms_of_service: "Terms of Service"
         delete: "Delete file"

spec/helpers/terms_and_conditions_helper_spec.rb

@@ -4,14 +4,29 @@ require 'spec_helper'
 
 describe TermsAndConditionsHelper, type: :helper do
   describe "#platform_terms_required?" do
-    it "returns true" do
-      expect(Spree::Config).to receive(:shoppers_require_tos).and_return(true)
-      expect(helper.platform_terms_required?).to eq true
-    end
+    context 'when ToS file is present' do
+      before do
+        allow(TermsOfServiceFile).to receive(:exists?).and_return(true)
+      end
+      it "returns true" do
+        expect(Spree::Config).to receive(:shoppers_require_tos).and_return(true)
+        expect(helper.platform_terms_required?).to eq true
+      end
 
-    it "returns false" do
-      expect(Spree::Config).to receive(:shoppers_require_tos).and_return(false)
-      expect(helper.platform_terms_required?).to eq false
+      it "returns false" do
+        expect(Spree::Config).to receive(:shoppers_require_tos).and_return(false)
+        expect(helper.platform_terms_required?).to eq false
+      end
+    end
+    context 'when ToS file is not present' do
+      before do
+        allow(TermsOfServiceFile).to receive(:exists?).and_return(false)
+      end
+
+      it "returns false" do
+        expect(Spree::Config).not_to receive(:shoppers_require_tos)
+        expect(helper.platform_terms_required?).to eq false
+      end
     end
   end
 end

spec/models/terms_of_service_file_spec.rb

@@ -24,8 +24,8 @@ describe TermsOfServiceFile do
   describe ".current_url" do
     let(:subject) { TermsOfServiceFile.current_url }
 
-    it "points to the old default" do
-      expect(subject).to eq "/Terms-of-service.pdf"
+    it "points to nil if not ToS file is present" do
+      expect(subject).to eq nil
     end
 
     it "points to a stored file" do

spec/services/terms_of_service_spec.rb

@@ -22,12 +22,25 @@ describe TermsOfService do
       allow(TermsOfServiceFile).to receive(:updated_at) { 2.weeks.ago }
     end
 
-    it "should reflect whether the platform TOS have been accepted since the last update" do
-      expect {
-        allow(TermsOfServiceFile).to receive(:updated_at) { Time.zone.now }
-      }.to change {
-        TermsOfService.tos_accepted?(customer)
-      }.from(true).to(false)
+    context "ToS file is present" do
+      it "should reflect whether the platform TOS have been accepted since the last update" do
+        expect {
+          allow(TermsOfServiceFile).to receive(:updated_at) { Time.zone.now }
+          allow(TermsOfServiceFile).to receive(:exists?) { true }
+        }.to change {
+          TermsOfService.tos_accepted?(customer)
+        }.from(true).to(false)
+      end
+    end
+
+    context "ToS file is not present" do
+      it "should always return true" do
+        expect {
+          allow(TermsOfServiceFile).to receive(:exists?) { false }
+        }.to_not change {
+          TermsOfService.tos_accepted?(customer)
+        }.from(true)
+      end
     end
   end
 

spec/system/admin/configuration/terms_of_service_files_spec.rb

@@ -15,8 +15,6 @@ describe "Terms of Service files" do
       click_link "Terms of Service"
 
       expect(page).to have_content "No terms of services have been uploaded yet."
-      expect(page).to have_content "your old Terms of service"
-      expect(page).to have_link "Terms of service", href: "/Terms-of-service.pdf"
     end
 
     it "can be uploaded" do
@@ -25,6 +23,7 @@ describe "Terms of Service files" do
       click_button "Create Terms of service file"
 
       expect(page).to have_link "Terms of Service"
+      expect(page).to have_link "Delete file"
     end
 
     it "provides Rails' standard action for a new file" do

spec/system/consumer/split_checkout_spec.rb

@@ -1029,13 +1029,16 @@ describe "As a consumer, I want to checkout my order" do
         context "when the platform's terms of service have to be accepted" do
           before do
             allow(Spree::Config).to receive(:shoppers_require_tos).and_return(true)
-            allow(Spree::Config).to receive(:footer_tos_url).and_return(tos_url)
+          end
+
+          let!(:tos) do
+            TermsOfServiceFile.create!(attachment: system_terms)
           end
 
           it "shows the terms which need to be accepted" do
             visit checkout_step_path(:summary)
 
-            expect(page).to have_link "Terms of service", href: tos_url
+            expect(page).to have_link("Terms of service", href: /Terms-of-service.pdf/, count: 2)
             expect(find_link("Terms of service")[:target]).to eq "_blank"
             expect(page).to have_field "order_accept_terms", checked: false
           end
@@ -1043,9 +1046,8 @@ describe "As a consumer, I want to checkout my order" do
           context "when the terms have been accepted in the past" do
             context "with a dedicated ToS file" do
               before do
-                TermsOfServiceFile.create!(
-                  attachment: system_terms,
-                  updated_at: 1.day.ago,
+                tos.update!(
+                  updated_at: 1.day.ago
                 )
                 customer.update(terms_and_conditions_accepted_at: Time.zone.now)
               end
@@ -1080,14 +1082,17 @@ describe "As a consumer, I want to checkout my order" do
             order.distributor.update!(terms_and_conditions: shop_terms)
 
             allow(Spree::Config).to receive(:shoppers_require_tos).and_return(true)
-            allow(Spree::Config).to receive(:footer_tos_url).and_return(tos_url)
+          end
+
+          let!(:tos) do
+            TermsOfServiceFile.create!(attachment: system_terms)
           end
 
           it "shows links to both terms and all need accepting" do
             visit checkout_step_path(:summary)
 
             expect(page).to have_link "Terms and Conditions", href: /#{shop_terms_path.basename}$/
-            expect(page).to have_link "Terms of service", href: tos_url
+            expect(page).to have_link("Terms of service", href: /Terms-of-service.pdf/, count: 2)
             expect(page).to have_field "order_accept_terms", checked: false
           end
         end