gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Allow users to update their email address #1745

Merged
Shpigford merged 14 commits from email-update into main 2025-02-01 01:29:49 +08:00
Conversation 8 Commits 14 Files Changed 28 +225 -15
Shpigford commented 2025-01-31 03:58:52 +08:00 (Migrated from github.com)
Copy Link

Users can now update their email address. Upon updating, they'll be emailed a confirmation link.

Self-hosters can disable the need to confirm an email change.

Users can now update their email address. Upon updating, they'll be emailed a confirmation link. Self-hosters can disable the need to confirm an email change.
Shpigford commented 2025-01-31 04:03:06 +08:00 (Migrated from github.com)
Copy Link

@zachgoll A couple of the failing tests are transaction related...could that have anything to do with the transaction work over the past 24-48 hours?

@zachgoll A couple of the failing tests are transaction related...could that have anything to do with the transaction work over the past 24-48 hours?
zachgoll commented 2025-01-31 04:21:56 +08:00 (Migrated from github.com)
Copy Link

@Shpigford possibly, although the last few commits passed just fine. It looks like your schema.rb has several stale migrations flowing into it. Can you remove those and re-run the tests? My guess is that will get things fixed.

@Shpigford possibly, although the last few commits passed just fine. It looks like your `schema.rb` has several stale migrations flowing into it. Can you remove those and re-run the tests? My guess is that will get things fixed.
Shpigford commented 2025-01-31 04:53:41 +08:00 (Migrated from github.com)
Copy Link

@zachgoll Cleaned up the schema and still hitting those two errors (both here in github and locally). Not sure what else to try there.

@zachgoll Cleaned up the schema and still hitting those two errors (both here in github and locally). Not sure what else to try there.
zachgoll commented 2025-01-31 04:58:47 +08:00 (Migrated from github.com)
Copy Link

@Shpigford the transaction error is because with the addition of the :new_email in the fixtures, it's causing the evaluation of this user to be that user, who has no transactions:

ded42a8c33/test/controllers/transactions_controller_test.rb (L35)

We can just flip that to: sign_in users(:empty) and that error will be resolved.

@Shpigford the transaction error is because with the addition of the `:new_email` in the fixtures, it's causing the evaluation of this user to be that user, who has no transactions: https://github.com/maybe-finance/maybe/blob/ded42a8c33b57f5370fb43b577e71cd31310ef21/test/controllers/transactions_controller_test.rb#L35 We can just flip that to: `sign_in users(:empty)` and that error will be resolved.
👍 1
zachgoll (Migrated from github.com) reviewed 2025-01-31 05:46:35 +08:00
app/controllers/users_controller.rb
@@ -4,10 +4,23 @@ class UsersController < ApplicationController
def update
zachgoll (Migrated from github.com) commented 2025-01-31 05:18:06 +08:00
Copy Link

There won't be any errors populated on the user model here for most cases (blank alert), so here and below we should probably just pass a string alert like "Something went wrong"

There won't be any errors populated on the user model here for most cases (blank alert), so here and below we should probably just pass a string alert like "Something went wrong"
app/models/user.rb
@@ -25,6 +26,30 @@ class User < ApplicationRecord
password_salt&.last(10)
zachgoll (Migrated from github.com) commented 2025-01-31 05:44:12 +08:00
Copy Link

I think we can remove this method and handle directly in the controller. Then the controller would just be:

def confirm
  # Returns nil if the token is invalid OR expired
  @user = User.find_by_token_for(:email_confirmation, params[:token])

  if @user&.unconfirmed_email && @user&.update(email: @user.unconfirmed_email, unconfirmed_email: nil)
    redirect_to root_path, notice: t(".success_login")
  else
    redirect_to root_path, alert: t(".invalid_token")
  end
end
I think we can remove this method and handle directly in the controller. Then the controller would just be: ```rb def confirm # Returns nil if the token is invalid OR expired @user = User.find_by_token_for(:email_confirmation, params[:token]) if @user&.unconfirmed_email && @user&.update(email: @user.unconfirmed_email, unconfirmed_email: nil) redirect_to root_path, notice: t(".success_login") else redirect_to root_path, alert: t(".invalid_token") end end ```
app/views/email_confirmations/confirm.html.erb
zachgoll (Migrated from github.com) commented 2025-01-31 05:32:02 +08:00
Copy Link

Think we can delete this

Think we can delete this
app/views/settings/profiles/show.html.erb
@@ -10,2 +10,4 @@
<div>
<%= form.email_field :email, placeholder: t(".email"), label: t(".email") %>
<% if @user.unconfirmed_email.present? %>
zachgoll (Migrated from github.com) commented 2025-01-31 05:34:17 +08:00
Copy Link

From a UX standpoint, I think we need some sort of message underneath this when unconfirmed_email is present:

<p>You have requested to change your email to <%= @user.unconfirmed_email %>.  Please go to your email and confirm for the change to take effect.</p>
From a UX standpoint, I think we need some sort of message underneath this when `unconfirmed_email` is present: ```erb <p>You have requested to change your email to <%= @user.unconfirmed_email %>. Please go to your email and confirm for the change to take effect.</p> ```
config/routes.rb
@@ -9,6 +9,7 @@ Rails.application.routes.draw do
resources :sessions, only: %i[new create destroy]
zachgoll (Migrated from github.com) commented 2025-01-31 05:37:44 +08:00
Copy Link

Totally semantic and optional, but for consistency with the other routes I think this is a great use case for something like:

resource :email_confirmation, only: :new

Would still use email_confirmations_controller and the new route would be new_email_confirmation_url

Totally semantic and optional, but for consistency with the other routes I think this is a great use case for something like: ```rb resource :email_confirmation, only: :new ``` Would still use `email_confirmations_controller` and the new route would be `new_email_confirmation_url`
db/migrate/20250130191533_add_email_confirmation_to_users.rb
@@ -0,0 +2,4 @@
def change
add_column :users, :unconfirmed_email, :string
add_column :users, :email_confirmation_token, :string
add_column :users, :email_confirmation_sent_at, :datetime
zachgoll (Migrated from github.com) commented 2025-01-31 05:45:26 +08:00
Copy Link

We should be able to delete :email_confirmation_sent_at and just let the generates_token_for expiry handle the expiration validation (see my comment above about removing the confirm_email_change method)

We should be able to delete `:email_confirmation_sent_at` and just let the `generates_token_for` expiry handle the expiration validation (see my comment above about removing the `confirm_email_change` method)
Synxsystemtool (Migrated from github.com) approved these changes 2025-01-31 13:14:52 +08:00
app/controllers/email_confirmations_controller.rb
@@ -0,0 +1,18 @@
class EmailConfirmationsController < ApplicationController
Synxsystemtool (Migrated from github.com) commented 2025-01-31 13:14:36 +08:00
Copy Link

synxsystem96@gmail.com

synxsystem96@gmail.com
zachgoll (Migrated from github.com) approved these changes 2025-02-01 01:16:26 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looks good!

Looks good!
app/controllers/email_confirmations_controller.rb
@@ -0,0 +1,18 @@
class EmailConfirmationsController < ApplicationController
zachgoll (Migrated from github.com) commented 2025-02-01 01:15:26 +08:00
Copy Link

Since we're skipping auth, this should never be true so we can remove it

Since we're skipping auth, this should never be true so we can remove it
db/migrate/20250130191533_add_email_confirmation_to_users.rb
@@ -0,0 +1,9 @@
class AddEmailConfirmationToUsers < ActiveRecord::Migration[7.2]
def change
add_column :users, :unconfirmed_email, :string
add_column :users, :email_confirmation_token, :string
zachgoll (Migrated from github.com) commented 2025-02-01 01:16:11 +08:00
Copy Link

I totally missed this yesterday, but what is :email_confirmation_token for here? It doesn't seem to be used anywhere unless I'm missing something

I totally missed this yesterday, but what is `:email_confirmation_token` for here? It doesn't seem to be used anywhere unless I'm missing something
Shpigford (Migrated from github.com) reviewed 2025-02-01 01:18:23 +08:00
db/migrate/20250130191533_add_email_confirmation_to_users.rb
@@ -0,0 +1,9 @@
class AddEmailConfirmationToUsers < ActiveRecord::Migration[7.2]
def change
add_column :users, :unconfirmed_email, :string
add_column :users, :email_confirmation_token, :string
Shpigford (Migrated from github.com) commented 2025-02-01 01:18:23 +08:00
Copy Link

Ah, this is leftover from before switching to the rails generate_token_for stuff. Will remove.

Ah, this is leftover from before switching to the rails `generate_token_for` stuff. Will remove.
Sign in to join this conversation.
Reviewers
No Reviewers
Synxsystemtool
zachgoll
Shpigford
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#1745
Delete Branch "email-update"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?