[Feature - Reset Password Flow] by milos-dukic · Pull Request #100 · basecamp/once-campfire

milos-dukic · 2025-10-23T08:09:10Z

PR for issue #72
Files changed:

app/assets/stylesheets/messages.css - some UI styling for error message
app/controllers/sessions/password_resets_controller.rb - new controller for handling the reset password flow
app/helpers/application_helper.rb - new helper method for checking is smtp is configured
app/javascript/controllers/password_reset_controller.js - new Stimulus controller for checking the new password and for displaying errors
app/mailers/application_mailer.rb - new application mailer
app/mailers/password_reset_mailer.rb - password reset mailer
app/models/user.rb - updated User model to be resettable
app/models/user/resettable.rb - new concern for password reset
app/views/password_reset_mailer/password_reset_email.html.erb - password reset HTML view
app/views/password_reset_mailer/password_reset_email.text.erb - password reset text view
app/views/sessions/new.html.erb - Added button for forgot password
app/views/sessions/password_resets/index.html.erb - HTML to input user email
app/views/sessions/password_resets/new.html.erb - HTML that email with reset link has been sent
app/views/sessions/password_resets/show.html.erb - HTML to enter new passwrod
config/environments/development.rb - SMTP development setup
config/environments/production.rb - SMPT production setup
config/routes.rb - new routes for password reset flow
test/mailers/fixture_templates/password_reset_html_fixture.txt - new fixture for html reset email
test/mailers/fixture_templates/password_reset_text_fixture.txt - new fixture for text reset email
test/mailers/password_reset_mailer_test.rb - new test
test/mailers/previews/password_reset_mailer_preview.rb - new test
config/environments/* - updated with new ENVs

ENV setup:

SMTP_ENABLED - defaults to false
SMTP_ADDRESS - obtained from mailing service
SMTP_PORT - obtained from mailing service
SMTP_DOMAIN - obtained from mailing service
SMTP_USER_NAME - obtained from mailing service
SMTP_PASSWORD - obtained from mailing service
SMTP_INFO_EMAIL_FROM - defaults to noreply@default.com
SMTP_PASSWORD_RESET_EMAIL_FROM - defaults to noreply@default.com

For SMTP feature to be enable i.e. supported following ENVs MUST be set:

SMTP_ENABLED
SMTP_ADDRESS
SMTP_PORT
SMTP_DOMAIN
SMTP_USER_NAME
SMTP_PASSWORD

SMTP config development:

  config.feature_enable_smtp = ENV.fetch("SMTP_ENABLED", false)
  config.action_mailer.delivery_method = :smtp
  config.action_mailer.smtp_settings = {
    address:         ENV["SMTP_ADDRESS"],
    port:            ENV["SMTP_PORT"],
    domain:          ENV["SMTP_DOMAIN"],
    user_name:       ENV["SMTP_USER_NAME"],
    password:        ENV["SMTP_PASSWORD"],
    authentication:  "plain",
    open_timeout:    5,
    read_timeout:    5,
    openssl_verify_mode:  "none"
  }

Note openssl_verify_mode: "none" is set since I had issues with certificates in sandbox env.

SMTP config production:

  config.feature_enable_smtp = ENV.fetch("SMTP_ENABLED", false)
  config.action_mailer.delivery_method = :smtp
  config.action_mailer.smtp_settings = {
    address:         ENV["SMTP_ADDRESS"],
    port:            ENV["SMTP_PORT"],
    domain:          ENV["SMTP_DOMAIN"],
    user_name:       ENV["SMTP_USER_NAME"],
    password:        ENV["SMTP_PASSWORD"],
    authentication:  "plain",
    enable_starttls: true,
    open_timeout:    5,
    read_timeout:    5
  }

Screenshots:

p-schlickmann · 2025-10-23T17:36:44Z

app/controllers/sessions/password_resets_controller.rb

+    @user = User.find_by_password_reset_id(password_reset_params[:password_reset_id])
+
+    redirect_to root_url unless @user
+    redirect_to root_url unless password_reset_params[:new_password] == password_reset_params[:confirm_new_password]


Maybe you could create another method called passwords_match? to make it a bit cleaner?

def update ... redirect_to root_url unless passwords_match? ... end def passwords_match? password_reset_params[:new_password] == password_reset_params[:confirm_new_password] end

yeah sure why not :)

p-schlickmann · 2025-10-23T17:39:43Z

app/models/user/resettable.rb

+  extend ActiveSupport::Concern
+
+  # RESET_PASSWORD_LINK_EXPIRY_DURATION = 5.minutes
+  RESET_PASSWORD_LINK_EXPIRY_DURATION = 5.hours


maybe you forgot to delete the comment in line 4?

p-schlickmann · 2025-10-23T18:06:02Z

config/environments/test.rb

+  config.feature_enable_smtp = ENV.fetch("SMTP_ENABLED", false)
+  config.action_mailer.delivery_method = :test
+  config.action_mailer.smtp_settings = {
+    address:         ENV.fetch("SMTP_ADDRESS", nil),
+    port:            ENV.fetch("SMTP_PORT", nil),
+    domain:          ENV.fetch("SMTP_DOMAIN", nil),
+    user_name:       ENV.fetch("SMTP_USER_NAME", nil),
+    password:        ENV.fetch("SMTP_PASSWORD", nil),
+    authentication:  "plain",
+    open_timeout:    5,
+    read_timeout:    5,
+    openssl_verify_mode:  "none"
+  }


We shouldn't use SMTP in the test environment because it makes tests slow, flaky and dependent on external services. The :test delivery method combined with perform_deliveries = true allows us to manipulate the ActionMailer::Base.deliveries array during tests.

Docs: https://guides.rubyonrails.org/testing.html#testing-mailers

Suggested change

config.feature_enable_smtp = ENV.fetch("SMTP_ENABLED", false)

config.action_mailer.delivery_method = :test

config.action_mailer.smtp_settings = {

address: ENV.fetch("SMTP_ADDRESS", nil),

port: ENV.fetch("SMTP_PORT", nil),

domain: ENV.fetch("SMTP_DOMAIN", nil),

user_name: ENV.fetch("SMTP_USER_NAME", nil),

password: ENV.fetch("SMTP_PASSWORD", nil),

authentication: "plain",

open_timeout: 5,

read_timeout: 5,

openssl_verify_mode: "none"

}

config.action_mailer.delivery_method = :test

config.action_mailer.perform_deliveries = true

yeah about that...it all comes down to the method in application helper that checks whether the smtp is enabled or not, and that comes down to checking all above envs...I don't like it either :)... ok so I will try to find workaround for the testing purposes. Thx.

p-schlickmann · 2025-10-23T18:08:43Z

test/controllers/sessions/password_resets_controller_test.rb

Considering my previous comment, smtp usages should be removed in testing.

Test logic might need to be rewritten to leverage ActionMailer::Base.deliveries.

ok will see about that :)
EDIT: This is not for smpt per se, this mimics the reset password process, it has nothing to do with the smpt, apart from that the password reset feature is not available if the smpt is not set up. So the call to

def update @user = User.find_by_password_reset_id(password_reset_params[:password_reset_id]) redirect_to root_url unless @user redirect_to root_url unless password_match? @user.update(password: password_reset_params[:new_password]) redirect_to new_session_path end

will either reset password or won't be executed at all depending on the smpt set up i.e. if the config is set up.
So I would leave this, since it tests the controller and not the sending of the email :)

Yes, totally understand your point, but smtp_enabled? will always be false while running the test suite, so the if statement in line 22 could be removed

will do I misunderstood...my bad. thx

p-schlickmann · 2025-10-23T18:09:54Z

test/mailers/password_reset_mailer_test.rb

Remove smtp usages here as well

yep thx
EDIT: Not sure why to remove this, as this tests the body of the email, from, to etc. And when I replaced the envs with config.action_mailer.perform_deliveries = true config/test.rb test passes. So again not sure why to remove this? basically it is this: https://guides.rubyonrails.org/testing.html#unit-testing :)

Yes it should pass, but SMTP_PASSWORD_RESET_EMAIL_FROM in line 11 is irrelevant since SMTP is not configured in the test environment

aha yeah sure sure...I misunderstood...my bad. thx

@p-schlickmann sorry :) for the test env it isn't set, but for me locally, or for anyone else who configures it locally (I mean smtp), this will fail if we leave out ENV fetch, because in the code it checks whether the env is set or not. So, for the test to pass locally as well if the SMTP is configured i.e. if SMTP_PASSWORD_RESET_EMAIL_FROM env is set this needs to stay, or we can completely remove line 11 assert_equal [ ENV.fetch("SMTP_PASSWORD_RESET_EMAIL_FROM", ApplicationMailer::DEFAULT_BASE_FROM) ], email.from and don't test the from at all...I would leave this as it is, rather then remove the test case...
The part of the code that sets the FROM in app/mailers/password_reset_mailer.rb:

class PasswordResetMailer < ApplicationMailer default from: ENV.fetch("SMTP_PASSWORD_RESET_EMAIL_FROM", DEFAULT_BASE_FROM) def password_reset_email @email = params[:email] @url = params[:url] mail(to: @email, subject: "Campfire Reset Password") end end

milos-dukic · 2025-10-24T14:34:48Z

@p-schlickmann updated the PR :) thx for the review btw

milos-dukic · 2025-10-27T14:41:45Z

@p-schlickmann updated PR :) with one comment on test/mailers/password_reset_mailer_test.rb thx

p-schlickmann · 2025-10-28T19:14:25Z

Thanks @milos-dukic, I'll wait for a mantainer's opinion now.

…s and stimulus controllers and mailer. New tests and fixtures added.

milos-dukic mentioned this pull request Oct 23, 2025

Add *optional* email-based password reset flow #72

Open

p-schlickmann reviewed Oct 23, 2025

View reviewed changes

milos-dukic force-pushed the feature-reset-passwrod-flow branch from d2ce64f to 5d1fae8 Compare October 24, 2025 14:34

milos-dukic force-pushed the feature-reset-passwrod-flow branch from 5d1fae8 to b13696f Compare October 27, 2025 14:40

[Feature - Reset Password Flow] - added new ENVs, pages, routes, rail…

78cb507

…s and stimulus controllers and mailer. New tests and fixtures added.

milos-dukic force-pushed the feature-reset-passwrod-flow branch from b13696f to 78cb507 Compare December 22, 2025 15:29

Conversation

milos-dukic commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-schlickmann Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milos-dukic Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-schlickmann Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milos-dukic Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-schlickmann Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milos-dukic Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milos-dukic Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milos-dukic commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milos-dukic commented Oct 27, 2025

Uh oh!

p-schlickmann commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

milos-dukic commented Oct 23, 2025 •

edited

Loading

p-schlickmann Oct 23, 2025 •

edited

Loading

milos-dukic Oct 24, 2025 •

edited

Loading

p-schlickmann Oct 24, 2025 •

edited

Loading

milos-dukic Oct 24, 2025 •

edited

Loading

p-schlickmann Oct 24, 2025 •

edited

Loading

milos-dukic Oct 27, 2025 •

edited

Loading

milos-dukic Oct 27, 2025 •

edited

Loading

milos-dukic commented Oct 24, 2025 •

edited

Loading