Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Email verification #4268

Merged
merged 10 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/controllers/confirmations_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

class ConfirmationsController < Devise::ConfirmationsController
protected

def after_confirmation_path_for(resource_name, resource)
zwolf marked this conversation as resolved.
Show resolved Hide resolved
'https://www.zooniverse.org'
end
end
8 changes: 5 additions & 3 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class User < ApplicationRecord
attr_accessor :minor_age

devise :database_authenticatable, :registerable,
:recoverable, :rememberable, :trackable, :validatable,
:recoverable, :rememberable, :trackable, :validatable, :confirmable,
zwolf marked this conversation as resolved.
Show resolved Hide resolved
:omniauthable, omniauth_providers: [:facebook, :google_oauth2]

has_many :classifications, dependent: :restrict_with_exception
Expand Down Expand Up @@ -56,15 +56,13 @@ class User < ApplicationRecord
validates_with IdentityGroupNameValidator

after_create :set_zooniverse_id
after_create :send_welcome_email, unless: :migrated
before_create :set_ouroboros_api_key

delegate :projects, to: :identity_group
delegate :collections, to: :identity_group
delegate :subjects, to: :identity_group
delegate :owns?, to: :identity_group


pg_search_scope :search_name,
against: [:login],
using: {
Expand Down Expand Up @@ -204,6 +202,10 @@ def self.find_by_lower_login(login)
find_by("lower(login) = ?", login.downcase)
end

def after_confirmation
send_welcome_email
end

def subject_limit
super || Panoptes.max_subjects
end
Expand Down
3 changes: 2 additions & 1 deletion config/initializers/devise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
config.strip_whitespace_keys = [ :email ]
config.skip_session_storage = [:http_auth]
config.stretches = Rails.env.test? ? 1 : 10
config.reconfirmable = true
config.reconfirmable = false
config.allow_unconfirmed_access_for = nil
config.password_length = 8..128
config.reset_password_within = 6.hours
config.paranoid = true
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
end

devise_for :users,
controllers: { omniauth_callbacks: 'omniauth_callbacks', passwords: 'passwords' },
controllers: { confirmations: 'confirmations', omniauth_callbacks: 'omniauth_callbacks', passwords: 'passwords' },
yuenmichelle1 marked this conversation as resolved.
Show resolved Hide resolved
skip: [ :sessions, :registrations ]

as :user do
Expand Down
21 changes: 21 additions & 0 deletions db/migrate/20231025200957_add_confirmable_to_devise.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# frozen_string_literal: true

class AddConfirmableToDevise < ActiveRecord::Migration[6.1]
disable_ddl_transaction!

def change
add_column :users, :confirmation_token, :string
add_column :users, :confirmed_at, :datetime
add_column :users, :confirmation_sent_at, :datetime

add_index :users, :confirmation_token, unique: true, algorithm: :concurrently
end

def down
remove_column :users, :confirmation_token
remove_column :users, :confirmed_at
remove_column :users, :confirmation_sent_at

remove_index :users, :confirmation_token
end
end
17 changes: 13 additions & 4 deletions db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ COMMENT ON EXTENSION pg_trgm IS 'text similarity measurement and index searching

SET default_tablespace = '';

SET default_with_oids = false;

--
-- Name: access_control_lists; Type: TABLE; Schema: public; Owner: -
--
Expand Down Expand Up @@ -1802,7 +1800,10 @@ CREATE TABLE public.users (
upload_whitelist boolean DEFAULT false NOT NULL,
ux_testing_email_communication boolean DEFAULT false,
intervention_notifications boolean DEFAULT true,
nasa_email_communication boolean DEFAULT false
nasa_email_communication boolean DEFAULT false,
confirmation_token character varying,
confirmed_at timestamp without time zone,
confirmation_sent_at timestamp without time zone
);


Expand Down Expand Up @@ -3568,6 +3569,13 @@ CREATE INDEX index_users_on_activated_state ON public.users USING btree (activat
CREATE INDEX index_users_on_beta_email_communication ON public.users USING btree (beta_email_communication) WHERE (beta_email_communication = true);


--
-- Name: index_users_on_confirmation_token; Type: INDEX; Schema: public; Owner: -
--

CREATE UNIQUE INDEX index_users_on_confirmation_token ON public.users USING btree (confirmation_token);


--
-- Name: index_users_on_display_name; Type: INDEX; Schema: public; Owner: -
--
Expand Down Expand Up @@ -4576,6 +4584,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20211124175756'),
('20211201164326'),
('20221018032140'),
('20230613165746');
('20230613165746'),
('20231025200957');


12 changes: 12 additions & 0 deletions lib/tasks/user.rake
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ namespace :user do
end
end

desc 'Backfill confirmed_at in batches (restartable)'
task backfill_confirmed_at: :environment do
mass_confirmed_at = Time.current.to_s(:db)
User.select(:id).find_in_batches do |users|
null_confirmed_at_user_scope = User.where(
id: users.map(&:id),
confirmed_at: nil
)
null_confirmed_at_user_scope.update_all(confirmed_at: mass_confirmed_at)
end
end

namespace :limit do

class UpdateUserLimitArgsError < StandardError; end
Expand Down
5 changes: 3 additions & 2 deletions spec/controllers/passwords_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@
expect(response.status).to eq(200)
end

it "should not send an email to the account email address" do
expect(ActionMailer::Base.deliveries).to be_empty
it 'should send confirmation instructions to the account email address' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSpec/MultipleExpectations: Example has too many expectations [2/1].
RSpec/ExampleWording: Do not use should when describing your tests.

expect(ActionMailer::Base.deliveries).not_to be_empty
expect(ActionMailer::Base.deliveries.first.body).to include('confirm your account')
end
end

Expand Down
16 changes: 8 additions & 8 deletions spec/controllers/registrations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@
post :create, params: { user: user_attributes }
end

it "should queue a welcome worker to send an email" do
expect(UserWelcomeMailerWorker).to receive(:perform_async)
it "should send a confirmation instructions email" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
RSpec/ExampleWording: Do not use should when describing your tests.

expect_any_instance_of(User).to receive(:send_confirmation_instructions).once
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSpec/AnyInstance: Avoid stubbing using expect_any_instance_of.

post :create, params: { user: user_attributes }
end

Expand Down Expand Up @@ -212,8 +212,8 @@
end
end

it "should not queue a welcome worker to send an email" do
expect(UserWelcomeMailerWorker).to_not receive(:perform_async)
it "should not send a confirmation instructions email" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
RSpec/ExampleWording: Do not use should when describing your tests.

expect_any_instance_of(User).to_not receive(:send_confirmation_instructions)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSpec/AnyInstance: Avoid stubbing using expect_any_instance_of.
RSpec/NotToNot: Prefer not_to over to_not.

post :create, params: { user: user_attributes }
end
end
Expand Down Expand Up @@ -279,8 +279,8 @@
post :create, params: { user: user_attributes }
end

it "should queue a welcome worker to send an email" do
expect(UserWelcomeMailerWorker).to receive(:perform_async)
it "should send a confirmation instructions email" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
RSpec/ExampleWording: Do not use should when describing your tests.

expect_any_instance_of(User).to receive(:send_confirmation_instructions).once
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSpec/AnyInstance: Avoid stubbing using expect_any_instance_of.

post :create, params: { user: user_attributes }
end
end
Expand Down Expand Up @@ -313,8 +313,8 @@
expect(User.where(login: user_attributes[:login])).to_not exist
end

it "should not queue a welcome worker to send an email" do
expect(UserWelcomeMailerWorker).to_not receive(:perform_async)
it "should not send a confirmation instructions email" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
RSpec/ExampleWording: Do not use should when describing your tests.

expect_any_instance_of(User).to_not receive(:send_confirmation_instructions)
zwolf marked this conversation as resolved.
Show resolved Hide resolved
post :create, params: { user: user_attributes }
end
end
Expand Down
2 changes: 2 additions & 0 deletions spec/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
sequence(:email) {|n| "example#{n}@example.com"}
password { 'password' }
encrypted_password { User.new.send(:password_digest, 'password') }
confirmed_at { Time.current }
confirmation_sent_at { Time.current }
credited_name { 'Dr User' }
activated_state { :active }
sequence(:login) { |n| "new_user_#{n}" }
Expand Down
29 changes: 20 additions & 9 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,17 @@ def dormant_user_ids(num_days_since_activity=5)
end
end

describe 'confirmation' do
context 'when a new user is saved' do
let(:user) { build(:user, confirmed_at: nil, confirmation_sent_at: nil) }

it 'sends a confirmation email' do
expect_any_instance_of(User).to receive(:send_confirmation_instructions).once
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSpec/AnyInstance: Avoid stubbing using expect_any_instance_of.
RSpec/DescribedClass: Use described_class instead of User.

user.save!
end
end
end

describe '::send_reset_password_instructions' do
context 'when the user exists' do
let(:user) { create(:user) }
Expand Down Expand Up @@ -872,42 +883,42 @@ def dormant_user_ids(num_days_since_activity=5)
end
end

describe "#send_welcome_email after_create callback" do
let(:user) { build(:user) }
describe "#send_welcome_email on confirm" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

let(:user) { create(:user, confirmed_at: nil) }

it "should send the welcome email" do
expect(user).to receive(:send_welcome_email).once
user.save!
user.confirm
end

it "should queue the worker with the user id" do
allow(UserWelcomeMailerWorker).to receive :perform_async
user.save!
user.confirm
expect(UserWelcomeMailerWorker).to have_received(:perform_async).with(user.id, nil).ordered
end

context "when redis is unreachable" do
it "should not raise an error on save" do
[ Timeout::Error, Redis::TimeoutError, Redis::CannotConnectError ].each do |redis_error|
allow(UserWelcomeMailerWorker).to receive(:perform_async).and_raise(redis_error)
expect { user.save! }.not_to raise_error
expect { user.confirm }.not_to raise_error
end
end

it "should send the welcome email in band" do
allow(UserWelcomeMailerWorker).to receive(:perform_async).and_raise(Redis::CannotConnectError)
expect_any_instance_of(UserWelcomeMailerWorker).to receive(:perform).with(Integer, nil)
user.save!
user.confirm
end
end

context "when the user has a project id" do
let(:project) { create :project }
let!(:user) { build(:user, project_id: project.id) }
let!(:user) { create(:user, project_id: project.id, confirmed_at: nil) }

it "should queue the worker with the user id and project id" do
allow(UserWelcomeMailerWorker).to receive :perform_async
user.save!
user.confirm
expect(UserWelcomeMailerWorker).to have_received(:perform_async).with(user.id, project.id).ordered
end
end
Expand All @@ -917,7 +928,7 @@ def dormant_user_ids(num_days_since_activity=5)

it "should not queue the worker with the user id" do
expect(UserWelcomeMailerWorker).not_to receive(:perform_async)
user.save!
user.confirm
end
end
end
Expand Down