thewca / worldcubeassociation.org

All of the code that runs on worldcubeassociation.org
https://www.worldcubeassociation.org/
GNU General Public License v3.0
320 stars 175 forks source link

notify_user_of_delegate_demotion can attempt to notify new users without a confirmed email #1362

Open jfly opened 7 years ago

jfly commented 7 years ago

When creating a WCA website account, you can claim your WCA ID. If you type in a bogus email address while doing this, then permanently end up with a user with no confirmed email address who has issued a WCA ID claim. Normally this would be fine... no one is going to be bothered by these pending WCA ID claims (we don't show delegates WCA ID claims for unconfirmed accounts). However, when the delegate selected to handle that claim is demoted, we send out an email notification, which means we could be attempted to send emails to unconfirmed email addresses, which causes background job failures that the software team then has to look into.

One simple fix here might be to change the users_claiming_wca_id association to require that the user be confirmed. We currently only use users_claiming_wca_id in 2 places:

~/gitting/worldcubeassociation.org @kaladin> git grep users_claiming_wca_id
WcaOnRails/app/helpers/notifications_helper.rb:    user.users_claiming_wca_id.where.not(confirmed_at: nil).each do |user_claiming_wca_id|
WcaOnRails/app/models/user.rb:  has_many :users_claiming_wca_id, foreign_key: "delegate_id_to_handle_wca_id_claim", class_name: "User"
WcaOnRails/app/models/user.rb:      users_claiming_wca_id.each do |user|

However, just making that change would cause us to skip this WCA ID claim clearing code, which means we could end up with users whose delegate_id_to_handle_wca_id_claim points at a user who is no longer a delegate. As a side note, that unchecked called to update is giving me the jitters...

larspetrus commented 7 years ago

Without knowing any details, one solution is to catch the exception and accept that email didn't get through.