From a2e7de0007510b568b4a133fa3baa4a87ef24b1c Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Wed, 5 Aug 2026 15:05:08 +0200 Subject: [PATCH 1/3] feat(member): derive active? from auth services, drop can_log_in column Replace the stored can_log_in boolean with a derived active? (an auth service exists), move the profile validation gates over to it, and remove the column. Members are active by having an auth service; the stored flag was misnamed and had corroded into a parity value. Follows the strong_migrations ignore-then-drop pattern. --- app/models/member.rb | 12 +++++-- ...06000000_remove_can_log_in_from_members.rb | 5 +++ db/schema.rb | 3 +- spec/models/member_spec.rb | 32 +++++++++++++++++-- 4 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20260806000000_remove_can_log_in_from_members.rb diff --git a/app/models/member.rb b/app/models/member.rb index b2a96dfa7..bef9bddf8 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -1,4 +1,6 @@ class Member < ApplicationRecord + self.ignored_columns += ['can_log_in'] + include DigestHelper rolify role_cname: 'Permission', role_table_name: :permission, role_join_table_name: :members_permissions @@ -27,9 +29,9 @@ class Member < ApplicationRecord has_many :member_email_deliveries validates :auth_services, presence: true - validates :name, :surname, :email, :about_you, presence: true, if: :can_log_in? + validates :name, :surname, :email, :about_you, presence: true, if: :active? validates :email, uniqueness: true - validates :email, email: { mode: :strict }, if: :can_log_in? + validates :email, email: { mode: :strict }, if: :active? validates :about_you, length: { maximum: 255 } DIETARY_RESTRICTIONS = %w[vegan vegetarian pescetarian halal gluten_free dairy_free other].freeze @@ -125,7 +127,11 @@ def avatar(size = 100) end def requires_additional_details? - can_log_in? && !valid? + active? && !valid? + end + + def active? + auth_services.exists? end def existing_rsvp_on?(date) diff --git a/db/migrate/20260806000000_remove_can_log_in_from_members.rb b/db/migrate/20260806000000_remove_can_log_in_from_members.rb new file mode 100644 index 000000000..ef9c03797 --- /dev/null +++ b/db/migrate/20260806000000_remove_can_log_in_from_members.rb @@ -0,0 +1,5 @@ +class RemoveCanLogInFromMembers < ActiveRecord::Migration[8.1] + def change + safety_assured { remove_column :members, :can_log_in, :boolean } + end +end diff --git a/db/schema.rb b/db/schema.rb index 9983a5607..6820b6fcd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_07_31_104506) do +ActiveRecord::Schema[8.1].define(version: 2026_08_06_000000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -441,7 +441,6 @@ create_table "members", id: :serial, force: :cascade do |t| t.string "about_you" t.datetime "accepted_toc_at", precision: nil - t.boolean "can_log_in", default: false, null: false t.datetime "created_at", precision: nil t.enum "dietary_restrictions", default: [], array: true, enum_type: "dietary_restriction_enum" t.string "email" diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index db211631d..7813b51b4 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -7,8 +7,10 @@ it { is_expected.to validate_length_of(:about_you).is_at_most(255) } it { is_expected.to validate_uniqueness_of(:email) } - describe 'can_log_in' do - let(:member) { Fabricate.build(:member, can_log_in: true) } + describe 'active' do + # Fabricated members have a persisted auth service, so #active? is true + # and the profile presence/email validations run. + let(:member) { Fabricate(:member) } it { expect(member).to validate_presence_of(:name) } it { expect(member).to validate_presence_of(:surname) } @@ -45,6 +47,32 @@ member.email = 'user+tag@example.com' expect(member).to be_valid end + + context 'when the member has no auth service' do + let(:member) do + member = Fabricate(:member) + member.auth_services.delete_all + member.name = nil + member.surname = nil + member.about_you = nil + member + end + + it 'is not active' do + expect(member.active?).to be(false) + end + + it 'skips the gated profile presence validations' do + member.valid? + expect(member.errors[:name]).to be_empty + expect(member.errors[:surname]).to be_empty + expect(member.errors[:about_you]).to be_empty + end + + it 'does not require additional details' do + expect(member.requires_additional_details?).to be(false) + end + end end end From eb407eac8c1f3911b455407ba3c627e9c47539a6 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Wed, 5 Aug 2026 15:05:23 +0200 Subject: [PATCH 2/3] fix(auth): stop storing can_log_in and gate the details redirect Remove the can_log_in write from the OAuth callback (members are active by having an auth service) and only send members to the details page when they actually need additional details. A complete member who links a second provider now goes to the referer/dashboard instead of being bounced to profile completion. Closes the unconditional-redirect behaviour behind #2783. --- app/controllers/auth_services_controller.rb | 14 +++---- spec/features/member_joining_spec.rb | 2 - spec/requests/auth_services_callback_spec.rb | 41 ++++++++++++++++++-- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/app/controllers/auth_services_controller.rb b/app/controllers/auth_services_controller.rb index ab2676592..b9ce29875 100644 --- a/app/controllers/auth_services_controller.rb +++ b/app/controllers/auth_services_controller.rb @@ -44,10 +44,8 @@ def create uid: omnihash[:uid] ) - created = false begin member.save! - created = true rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique # Concurrent OAuth callback for the same account: another request just # created this member/auth_service (both email and (uid, provider) are @@ -59,18 +57,16 @@ def create member = member_service.member end - # Set, not Toggle: toggling would flip can_log_in off for a member who - # links a second auth service. Skip the conditional profile validations - # here on purpose — a brand-new member completes name/about_you on the - # next (details) page, and running them now aborts the signup callback. - member.update_column(:can_log_in, true) if created # rubocop:disable Rails/SkipsModelValidations - session[:member_id] = member.id session[:service_id] = member_service.id session[:oauth_token] = omnihash[:credentials][:token] session[:oauth_token_secret] = omnihash[:credentials][:secret] - redirect_to edit_member_details_path(member_type: member_type) + if member.requires_additional_details? + redirect_to edit_member_details_path(member_type: member_type) + else + redirect_to referer_or_dashboard_path + end end end diff --git a/spec/features/member_joining_spec.rb b/spec/features/member_joining_spec.rb index b5ad30bcf..c4c8420d3 100644 --- a/spec/features/member_joining_spec.rb +++ b/spec/features/member_joining_spec.rb @@ -16,7 +16,6 @@ scenario 'A visitor must fill in all mandatory fields in order to sign up' do member = Fabricate(:member, name: nil, surname: nil, email: nil, about_you: nil, how_you_found_us: nil, how_you_found_us_other_reason: nil) - member.update_attribute(:can_log_in, true) login member visit edit_member_details_path @@ -68,7 +67,6 @@ group = Fabricate(:group) login member - member.update(can_log_in: true) visit step2_member_path expect(page).to have_current_path(step2_member_path) diff --git a/spec/requests/auth_services_callback_spec.rb b/spec/requests/auth_services_callback_spec.rb index ae04ea59f..1110989b6 100644 --- a/spec/requests/auth_services_callback_spec.rb +++ b/spec/requests/auth_services_callback_spec.rb @@ -17,10 +17,45 @@ expect { post '/auth/github/callback' }.not_to raise_error - expect(response).to redirect_to(edit_member_details_path) + # The winner is a complete member, so the losing callback must not bounce + # them back to the details page — they go to the dashboard instead. + expect(response).to redirect_to(dashboard_path) expect(session[:member_id]).to eq(winner.id) expect(session[:service_id]).to eq(winner_service.id) - # the losing callback must not flip the winner's can_log_in flag back - expect(winner.reload.can_log_in).to be(false) + expect(winner.reload.active?).to be(true) + end + + it 'sends a complete member who links a second provider to the dashboard, not details' do + complete = Fabricate(:member, email: 'existing@example.com') + mock_auth_hash(provider: 'github', uid: 'second-uid', + email: 'existing@example.com') + + post '/auth/github/callback' + + expect(response).to redirect_to(dashboard_path) + expect(session[:member_id]).to eq(complete.id) + end + + it 'sends a complete member who links a second provider to a stored referer' do + complete = Fabricate(:member, email: 'referer@example.com') + mock_auth_hash(provider: 'github', uid: 'second-uid-referer', + email: 'referer@example.com') + + # AuthServicesController#new (GET /login) stores a workshop/event/meeting + # referer in the session; a complete member then follows it instead of + # being bounced to the details page. + get '/login', headers: { 'HTTP_REFERER' => '/workshops/1' } + post '/auth/github/callback' + + expect(response).to redirect_to('/workshops/1') + expect(session[:member_id]).to eq(complete.id) + end + + it 'sends a brand-new member to complete their profile details' do + mock_auth_hash(provider: 'github', uid: 'new-uid', email: 'new@example.com') + + post '/auth/github/callback' + + expect(response).to redirect_to(edit_member_details_path) end end From 1a73e98c4ba82a6d1747338f0cd9e59a2de3a047 Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:51:06 +0200 Subject: [PATCH 3/3] Update app/controllers/auth_services_controller.rb Co-authored-by: Olle Jonsson --- app/controllers/auth_services_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/auth_services_controller.rb b/app/controllers/auth_services_controller.rb index b9ce29875..a5277878e 100644 --- a/app/controllers/auth_services_controller.rb +++ b/app/controllers/auth_services_controller.rb @@ -63,7 +63,7 @@ def create session[:oauth_token_secret] = omnihash[:credentials][:secret] if member.requires_additional_details? - redirect_to edit_member_details_path(member_type: member_type) + redirect_to edit_member_details_path(member_type:) else redirect_to referer_or_dashboard_path end