Skip to content

Commit 337335a

Browse files
sethherrclaude
andcommitted
Prune the comments this branch added
Per AGENTS.md: drop the ones that restate the name below them, and the clauses that narrate the change or defend a choice against an edit nobody was going to make. What's left is the why a reader would otherwise get wrong - token_time flooring so a blank token reads as expired, the sent-at stamp being written by the caller rather than the delivering job, the self-posting confirm form defeating link scanners. find_b_param_for_confirmation's had grown to five lines when the deleted service method's comment folded into it; now it leads with why it isn't find_b_param. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 90d96fd commit 337335a

11 files changed

Lines changed: 35 additions & 63 deletions

File tree

app/components/emails/partial_register_confirmation/component.rb

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ def expiration_days
1515

1616
def color_and_brand = @b_param.color_and_brand
1717

18-
# The confirmation token only ever exists in this email - never rendered
19-
# anywhere the registrant's browser could have shown it first
2018
def tokenized_url
2119
confirm_register_url(b_param_token: @b_param.id_token,
2220
confirmation_token: @b_param.email_confirmation_token)

app/controllers/concerns/sessionable.rb

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,8 @@ def sign_in_and_redirect(user, signed_up: false)
3737
end
3838
end
3939

40-
# Everything signing in does apart from deciding where to go next - so a flow with
41-
# its own destination (the register flow's confirmation link) lands in the same
42-
# session state as every other sign in
40+
# Everything signing in does apart from deciding where to go next, for flows with
41+
# a destination of their own
4342
def sign_in_user(user)
4443
confirm_user_from_claim_token(user)
4544
session[:last_seen] = Time.current

app/controllers/register_controller.rb

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ class RegisterController < ApplicationController
77
before_action -> { find_b_param(build: true) }, only: %i[create]
88
# The emailed link resumes a registration the session knows nothing about
99
before_action :find_b_param_for_confirmation, only: %i[confirm confirm_email]
10-
# confirm renders a form and nothing else - neither is read before it posts itself
10+
# confirm renders a self-posting form and nothing else, so it reads neither
1111
before_action :assign_organization, except: %i[new confirm]
1212
before_action :find_registration_sequence, except: %i[new confirm]
1313
before_action :redirect_finished, only: %i[create update acknowledge]
@@ -62,8 +62,7 @@ def create
6262
if @b_param.errors.any?
6363
render Register::Step1::Component.new(b_param: @b_param, sequence: @registration_sequence, current_user:), status: :unprocessable_entity
6464
elsif @b_param.save
65-
# Anonymous registrations need the address proven before there's a bike, and step 2
66-
# says the link is on its way - so it goes out here, not at the end
65+
# Step 2 says the link is on its way, so it goes out here rather than at the end
6766
BikeServices::Register.send_confirmation_email(@b_param)
6867
redirect_to step_path(2)
6968
else
@@ -109,8 +108,8 @@ def confirm
109108
render Register::Confirm::Component.new(b_param: @b_param, token: params[:confirmation_token])
110109
end
111110

112-
# The confirmation itself: the address is proven, so the registration has an account
113-
# behind it - created here if this is their first registration
111+
# The confirmation itself - the proven address gets an account, created here if
112+
# this is their first registration
114113
def confirm_email
115114
# Single use, so a second click has nothing left to do - the first one signed them in
116115
return redirect_to_current_step if @b_param.email_confirmed?
@@ -151,15 +150,13 @@ def complete_registration
151150
BikeServices::Register.claim_creator(@b_param, current_user)
152151
bike = BikeServices::Register.create_bike_if_ready(@b_param,
153152
sequence: @registration_sequence, ip_address: forwarded_ip_address)
154-
# Everything is saved on the b_param - the bike is created once the
155-
# confirmation link from the registration email is clicked
153+
# No bike yet - everything stays on the b_param until the emailed link is clicked
156154
return redirect_to_current_step if bike.blank?
157155

158156
redirect_after_bike_creation(bike)
159157
end
160158

161-
# The account the confirmed address belongs to, created if it doesn't have one yet -
162-
# passwordless, so it gets the same offer to set one as every other emailed sign in
159+
# The account the confirmed address belongs to, created if it doesn't have one yet
163160
def sign_in_confirmed_user
164161
user, signed_up = UserServices::PasswordlessCreator.find_or_create(@b_param.owner_email)
165162
if user.blank? || user.banned?
@@ -203,11 +200,9 @@ def find_registration_sequence
203200
@registration_sequence = BikeServices::Register.registration_sequence(@b_param)
204201
end
205202

206-
# The emailed link carries the registration's token, so it resumes from any browser.
207-
# Nothing is put in the session here - the token hasn't been checked yet, and dropping
208-
# a registration already underway in this browser isn't the link's to do.
209-
# No window like find_token's: the confirmation token expires on its own clock, and
210-
# finding the registration is what lets an expired link say so rather than dead-end
203+
# Not find_b_param: the emailed token authorizes this, not the session, and an expired
204+
# link has to find its registration to say so rather than dead-end. Nothing is written
205+
# to the session - the token hasn't been checked yet
211206
def find_b_param_for_confirmation
212207
token = params[:b_param_token]
213208
@b_param = BParam.find_by(id_token: token) if token.present?

app/jobs/email/partial_registration_job.rb

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
# frozen_string_literal: true
22

33
module Email
4-
# The emails a registration sends before it has a bike, and so before there's an account
5-
# to notify - the address itself is all that's been entered, so every one of them runs
6-
# the domain check first. kind names both the notification and the mailer method, and
7-
# defaults so jobs enqueued before it existed still run
4+
# The emails a registration sends before it has a bike - only an address has been
5+
# entered, so every one runs the domain check first. kind names both the notification
6+
# and the mailer method, and defaults for jobs enqueued before it took one
87
class PartialRegistrationJob < ApplicationJob
98
sidekiq_options queue: "notify", retry: 3
109

app/mailers/organized_mailer.rb

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ def partial_registration(b_param)
1313
b_param_mail(b_param, Emails::PartialRegistration::Component.new(b_param:), tag: __callee__)
1414
end
1515

16-
# The register flow's confirmation link - proves the address, and signs them in
1716
def partial_register_confirmation(b_param)
1817
b_param_mail(b_param, Emails::PartialRegisterConfirmation::Component.new(b_param:), tag: __callee__)
1918
end
@@ -118,8 +117,7 @@ def impound_claim_approved_or_denied(impound_claim)
118117

119118
private
120119

121-
# The emails a registration sends before it has a bike - addressed to whoever
122-
# entered it, and subjected by the mailer method's own name
120+
# Addressed to whoever entered the registration, and subjected by the caller's own name
123121
def b_param_mail(b_param, component, tag:)
124122
@organization = b_param.creation_organization
125123
mail(reply_to: reply_to,

app/models/b_param.rb

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ class BParam < ApplicationRecord
7272
stolen
7373
street
7474
].freeze
75-
# How long a register flow registration resumes by token - and so how long its emailed
76-
# confirmation link works, since a link outliving the registration it resumes is a dead end
75+
# How long a register flow registration resumes by token, and so how long its
76+
# emailed confirmation link works
7777
TOKEN_EXPIRATION = 90.days
7878
mount_uploader :image, ImageUploaderBackgrounded
7979
process_in_background :image, CarrierWaveProcessJob # Defer version generation so large uploads don't hit the 30s Rack::Timeout
@@ -100,7 +100,6 @@ class BParam < ApplicationRecord
100100
scope :without_bike_values, -> { bike_params_empty.or(where(origin: "register_flow").where("(params -> 'bike' -> 'manufacturer_id') IS NULL")) }
101101
# Tokenized lookups resume registrations for up to a month
102102
scope :recent_with_token, ->(toke) { where(id_token: toke).where("created_at >= ?", Time.current - 1.month) }
103-
# The register flow's own (longer) window - see TOKEN_EXPIRATION
104103
scope :unexpired_with_token, ->(toke) { where(id_token: toke).where("created_at >= ?", Time.current - TOKEN_EXPIRATION) }
105104
scope :unprocessed_image, -> { where(image_processed: false).where.not(image: nil) }
106105
scope :with_cycle_type, -> { bike_params.where("(params -> 'bike' -> 'cycle_type') IS NOT NULL") }
@@ -448,8 +447,7 @@ def email_unconfirmed?
448447
owner_email.present? && !email_confirmed?
449448
end
450449

451-
# Spends the confirmation token, so a forwarded email can't sign anyone in later.
452-
# creator_id: assigned in the same write, the registration having proven who it's for
450+
# Spends the confirmation token, so a forwarded email can't sign anyone in later
453451
def confirm_email!(creator_id: nil)
454452
return true if email_confirmed?
455453

@@ -458,15 +456,13 @@ def confirm_email!(creator_id: nil)
458456
.except("email_confirmation_token", "email_confirmation_email"))
459457
end
460458

461-
# The emailed link's credential. Distinct from id_token, which is in the registrant's
462-
# own URL before any email goes out - only a secret that lived solely in the email
463-
# proves the address received it. Nil once owner_email is edited to a different
464-
# address, since all the token proves is that the one it was mailed to received it
459+
# Distinct from id_token, which is in the registrant's own URL before any email goes
460+
# out - only a secret that lived solely in the email proves the address received it,
461+
# and only for the address it was mailed to
465462
def email_confirmation_token
466463
params["email_confirmation_token"] if email_confirmation_email == EmailNormalizer.normalize(owner_email)
467464
end
468465

469-
# The address the token in hand was mailed to
470466
def email_confirmation_email
471467
params["email_confirmation_email"]
472468
end
@@ -476,8 +472,8 @@ def email_confirmation_token_expired?
476472
SecurityTokenizer.token_time(email_confirmation_token) < Time.current - TOKEN_EXPIRATION
477473
end
478474

479-
# Resending reuses an unexpired token, so the link already in their inbox keeps working.
480-
# The stamp is what rate limits resends, so it's rewritten either way
475+
# Reuses an unexpired token, so the link already in their inbox keeps working - but
476+
# re-stamps either way, since the stamp is what rate limits resends
481477
def generate_email_confirmation_token!
482478
token = email_confirmation_token_expired? ? SecurityTokenizer.new_token : email_confirmation_token
483479
update(params: params.merge("email_confirmation_token" => token,
@@ -486,8 +482,8 @@ def generate_email_confirmation_token!
486482
token
487483
end
488484

489-
# When a link last went out - written by whatever asked for the send, rather than by
490-
# the job that delivers it, so it rate limits even when delivery drops the email
485+
# Written by whatever asked for the send rather than by the job that delivers it,
486+
# so it rate limits even when delivery drops the email
491487
def email_confirmation_sent_at
492488
Binxtils::TimeParser.parse(params["email_confirmation_sent_at"])
493489
end
@@ -682,7 +678,6 @@ def mnfg_name
682678
Manufacturer.calculated_mnfg_name(manufacturer, bike["manufacturer_other"])
683679
end
684680

685-
# What the registration's emails call the bike - everything entered that names it
686681
def color_and_brand
687682
[primary_frame_color.presence, mnfg_name].compact.join(" ")
688683
end

app/services/bike_services/register.rb

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ module Register
77

88
# Step "3" is the first of the e-vehicle acknowledgment pages
99
ACKNOWLEDGMENT_OFFSET = 3
10-
# How often a registration will email its confirmation link
1110
CONFIRMATION_EMAIL_INTERVAL = 5.minutes
1211

1312
# The token's registration when step 1 was never submitted (redirecting into
@@ -134,19 +133,16 @@ def finished?(b_param, sequence:)
134133
ready_for_bike?(b_param, sequence:) && !creator_available?(b_param)
135134
end
136135

137-
# A link went to the address and nothing has proven it yet - what the steps tell
138-
# the registrant to expect, and what makes their bike once they click it.
139-
# user: whoever is looking, since being signed in as the address settles it too
136+
# user: being signed in as the address settles it, without any link being clicked
140137
def confirmation_email_pending?(b_param, user: nil)
141138
return false if b_param.self_made?(user)
142139

143140
b_param.email_unconfirmed? && !creator_available?(b_param)
144141
end
145142

146143
# Anonymous registrations can't create a bike - Ownership needs a creator - so the
147-
# address is emailed a link that proves it, makes an account and signs them in
148-
# A resend can be asked for by anyone holding the registration's token, so the address
149-
# it was entered for is only emailed this often
144+
# address is emailed a link that proves it. Rate limited: anyone holding the
145+
# registration's token can ask for a resend
150146
def send_confirmation_email(b_param)
151147
return false unless confirmation_email_pending?(b_param)
152148
return false if b_param.email_confirmation_sent_at.to_i > (Time.current - CONFIRMATION_EMAIL_INTERVAL).to_i
@@ -186,8 +182,6 @@ def claim_creator(b_param, user)
186182
b_param.update(creator_id: user.id)
187183
end
188184

189-
# The bike everything has been waiting on - nil until the registration is complete
190-
# and has a creator, which is what the confirmation link finally supplies
191185
def create_bike_if_ready(b_param, sequence:, ip_address:)
192186
return nil if b_param.with_bike? || !creator_available?(b_param) ||
193187
!ready_for_bike?(b_param, sequence:)
@@ -207,7 +201,6 @@ def create_bike(b_param, ip_address:)
207201
# private below here
208202
#
209203

210-
# Everything's entered - all that's missing is a creator
211204
def ready_for_bike?(b_param, sequence:)
212205
details_completed?(b_param) && acknowledged?(b_param, sequence:)
213206
end

app/services/user_services/passwordless_creator.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ module UserServices
66
module PasswordlessCreator
77
extend Functionable
88

9-
# Returns the user and whether this call is what created them - confirming saves
9+
# Returns whether this call created them alongside the user - confirming saves
1010
# again, so previously_new_record? can't answer that by the time it returns
1111
def find_or_create(email)
1212
return [nil, false] if email.blank?

db/seeds/seed_organized_emails.rb

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,8 @@
235235
puts " Created transferred bike ##{bike.id}"
236236
end
237237

238-
# --- Registrations that never became bikes: the two emails a b_param sends.
239-
# Both are mid-flow states nothing else seeds, since every other seeded b_param
240-
# is only an input to BikeServices::Creator and ends up with a created bike. ---
238+
# --- Registrations that never became bikes: mid-flow states nothing else seeds,
239+
# since every other seeded b_param ends up with a created bike ---
241240
if BParam.partial_registrations.none?
242241
b_param = BParam.create!(origin: "embed_partial", params: {
243242
bike: {
@@ -251,10 +250,8 @@
251250
puts " Created partial registration b_param ##{b_param.id}"
252251
end
253252

254-
# The register flow parks an anonymous registration here until the emailed link proves
255-
# the address - generate_email_confirmation_token! is what mints the credential.
256-
# Brakebills so the preview renders its snippets, though a registration for an
257-
# organization with an auto_user already has a creator and never sends this
253+
# Brakebills so the preview renders its snippets, though an organization with an
254+
# auto_user already has a creator and would never send this
258255
if BParam.where(origin: "register_flow").where("(params -> 'email_confirmation_token') IS NOT NULL").none?
259256
b_param = BParam.create!(origin: "register_flow", params: {
260257
bike: {

spec/integration/registration/register_spec.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,7 @@ def confirmation_link
184184
bike = Bike.last
185185
expect(bike).to have_attributes(owner_email:, serial_number: "made_without_serial",
186186
status: "status_stolen", frame_model: "Marlin 7")
187-
# Signed in as the account the link made - to anyone else this page would say the
188-
# registration is the owner's to claim
187+
# Signed in as the account the link made - to anyone else it reads as unclaimed
189188
expect(page).to have_content("keep watch")
190189

191190
# An account nobody signed up for, so the terms are the first thing it's asked

0 commit comments

Comments
 (0)