Merge pull request #2582 from mroderick/fix/tc-unauthenticated-handling

fix(tc): handle unauthenticated and already-accepted users

Author: olleolleolle

app/controllers/terms_and_conditions_controller.rb

@@ -1,12 +1,22 @@
 class TermsAndConditionsController < ApplicationController
-  before_action :logged_in?
+  # Skip accept_terms (from ApplicationController) to avoid an infinite redirect loop:
+  # - If user hasn't accepted T&C, ApplicationController already redirects here
+  # - If user has accepted, there's no reason to redirect away from this page
+  # The view handles both cases (needs to accept vs already accepted)
   skip_before_action :accept_terms
 
   def show
     @terms_and_conditions_form = TermsAndConditionsForm.new
   end
 
   def update
+    # The show action skips accept_terms, but unauthenticated users can still
+    # POST directly to update. Redirect to login to avoid NoMethodError on nil.
+    unless logged_in?
+      redirect_to '/auth/github'
+      return
+    end
+
     @terms_and_conditions_form = TermsAndConditionsForm.new(terms_params)
     if @terms_and_conditions_form.valid?
       member = current_user

app/views/terms_and_conditions/show.html.haml

@@ -2,13 +2,30 @@
   .row.justify-content-md-center
     .col-md-10.col-lg-8
       %h1= t('terms_and_conditions.title')
-      %p
-        = t('terms_and_conditions.body')
-        %br
-        = link_to t('terms_and_conditions.link_text'), code_of_conduct_path, target: '_blank'
+      - if logged_in?
+        - if current_user.accepted_toc_at?
+          %p
+            = t('terms_and_conditions.already_accepted_html', date: current_user.accepted_toc_at.strftime('%d %B %Y'))
+        - else
+          %p
+            = t('terms_and_conditions.body')
+            %br
+            = link_to t('terms_and_conditions.link_text'), code_of_conduct_path, target: '_blank', rel: 'noopener'
+      - else
+        %p
+          = t('terms_and_conditions.login_required_html')
+          %br
+          = link_to t('terms_and_conditions.login_link_text'), '/auth/github'
 
-  = simple_form_for @terms_and_conditions_form, url: terms_and_conditions_path, method: :patch do |f|
-    .row.justify-content-md-center
-      .col-md-10.col-lg-8
-        = f.input :terms, as: :boolean
-        = f.button :button, t('terms_and_conditions.accept'), class: 'btn btn-primary mb-0'
+  - if logged_in? && !current_user.accepted_toc_at?
+    = simple_form_for @terms_and_conditions_form, url: terms_and_conditions_path, method: :patch do |f|
+      .row.justify-content-md-center
+        .col-md-10.col-lg-8
+          = f.input :terms, as: :boolean
+          = f.button :button, t('terms_and_conditions.accept'), class: 'btn btn-primary mb-0'
+  - elsif !logged_in?
+    = simple_form_for @terms_and_conditions_form, url: terms_and_conditions_path, method: :patch, html: { style: 'opacity: 0.6; pointer-events: none;' } do |f|
+      .row.justify-content-md-center
+        .col-md-10.col-lg-8
+          = f.input :terms, as: :boolean, disabled: true
+          = f.button :button, t('terms_and_conditions.accept'), class: 'btn btn-primary mb-0', disabled: true

config/locales/en.yml

@@ -350,6 +350,9 @@ en:
     body: "Before you can proceed you must first read our Code of Conduct and agree to abide by it."
     link_text: "Read codebar's Code of Conduct"
     accept: "Accept"
+    already_accepted_html: "You have already accepted our Code of Conduct on %{date}."
+    login_required_html: "Please log in to accept our Code of Conduct."
+    login_link_text: "Log in with GitHub"
     messages:
       notice: "You have to accept the Terms and Conditions before you are able to proceed."
   footer:

spec/features/accepting_terms_and_conditions_spec.rb

@@ -4,7 +4,7 @@
       mock_github_auth
     end
 
-    scenario 'they can''t proceed unless they accept the ToCs' do
+    scenario "they can't proceed unless they accept the ToCs" do
       visit root_path
       click_on 'Join us as a student'
       click_on 'Join us as a student'
@@ -16,7 +16,7 @@
       expect(page).to have_content('You have to accept the Terms and Conditions before you are able to proceed.')
     end
 
-    scenario 'they can read the Code of Conduct before accepting the ToCs', js: true do
+    scenario 'they can read the Code of Conduct before accepting the ToCs', :js do
       visit root_path
       click_on 'Join us as a student'
       click_on 'Join us as a student'
@@ -47,7 +47,7 @@
   end
 
   context 'When an existing member logs in' do
-    context 'and they have not yet accepted codebar''s ToCs' do
+    context "and they have not yet accepted codebar's ToCs" do
       scenario 'they have to accept before continuing to the page they want to get' do
         member = Fabricate(:member_without_toc)
         login(member)
@@ -59,14 +59,40 @@
       end
     end
 
-    context 'and they have already accepted codebar''s ToCs' do
+    context "and they have already accepted codebar's ToCs" do
       scenario 'they will be redirected to the link they were trying to access' do
         member = Fabricate(:member)
         login(member)
 
         visit dashboard_path
         expect(page).to have_current_path(dashboard_path)
       end
+
+      scenario 'they see a message that they have already accepted on the T&C page' do
+        member = Fabricate(:member)
+        login(member)
+
+        visit terms_and_conditions_path
+        expect(page).to have_current_path(terms_and_conditions_path)
+        expect(page).to have_content(/already accepted.*#{member.accepted_toc_at.strftime('%d %B %Y')}/)
+        expect(page).not_to have_button('Accept')
+      end
+    end
+  end
+
+  context 'When a guest user (not logged in) visits the page' do
+    scenario 'they see a login prompt and cannot accept the ToCs' do
+      visit terms_and_conditions_path
+
+      expect(page).to have_content('Please log in to accept our Code of Conduct.')
+      expect(page).to have_link('Log in with GitHub')
+    end
+
+    scenario 'they see a disabled form' do
+      visit terms_and_conditions_path
+
+      expect(page).to have_field('terms_and_conditions_form_terms', disabled: true)
+      expect(page).to have_button('Accept', disabled: true)
     end
   end
 end