Move the register flow's remaining logic into BikeServices::Register (#4033)

`RegisterController` was still carrying registration logic that belongs
in `BikeServices::Register`. Pure refactor, no behavior change.

- **The service gains the flow's decisions** — `save_step_1`,
`acknowledge_step` (which record a step's boxes write to), `complete`,
`confirmation_token_valid?`, and the two steps' permitted-params lists.
`claim_creator`, `create_bike_if_ready` and `create_bike` are concealed
behind `complete`.
- **`save_step_1` and `save_step_2` add their own errors to `b_param`**,
so `create` and `update` now read the same way: save, or re-render 422
with whatever the service put there. Their copy moves from
`controllers.register.{create,update}` to `shared.register_flow`, the
way `OrganizedServices::UserMenuItems` holds its labels.
- **`update` drops its `acknowledged?` guard and `create` its duplicated
422 render** — `complete` already returns nil when the registration
isn't ready, and both branches redirected to the same place.

The controller keeps what's actually HTTP: the session token, the
redirects, the flashes, the b_param lookups, and signing in a confirmed
address.

Author: sethherr

app/controllers/register_controller.rb

@@ -54,45 +54,41 @@ def show
   end
 
   def create
-    @b_param.clean_params(create_params.as_json)
-    # The 422 renders skip the derived meta title, which now needs the interpolation
-    @page_title = I18n.t("meta_titles.register_create", cycle_type: @b_param.type)
-    @b_param.errors.add(:base, translation(:email_required)) if @b_param.owner_email.blank?
-    @b_param.errors.add(:base, translation(:manufacturer_required)) if @b_param.manufacturer_id.blank?
-    if @b_param.errors.any?
-      render Register::Step1::Component.new(b_param: @b_param, sequence: @registration_sequence, current_user:), status: :unprocessable_entity
-    elsif @b_param.save
-      # Step 2 says the link is on its way, so it goes out here rather than at the end
-      BikeServices::Register.send_confirmation_email(@b_param)
-      redirect_to step_path(2)
-    else
-      @b_param.errors.add(:base, translation(:unable_to_save))
-      render Register::Step1::Component.new(b_param: @b_param, sequence: @registration_sequence, current_user:), status: :unprocessable_entity
+    saved = BikeServices::Register.save_step_1(@b_param, bike_params: create_params,
+      propulsion_type_motorized: params[:propulsion_type_motorized])
+    unless saved
+      # The 422 render skips the derived meta title, which now needs the interpolation
+      @page_title = I18n.t("meta_titles.register_create", cycle_type: @b_param.type)
+      return render(Register::Step1::Component.new(b_param: @b_param, sequence: @registration_sequence, current_user:),
+        status: :unprocessable_entity)
     end
+
+    # Step 2 says the link is on its way, so it goes out here rather than at the end
+    BikeServices::Register.send_confirmation_email(@b_param)
+    redirect_to step_path(2)
   end
 
   def update
     # Both read straight from params - update_params is stored as json, which an upload can't be
-    saved_step_2 = BikeServices::Register.save_step_2(@b_param, user: current_user,
+    saved = BikeServices::Register.save_step_2(@b_param, user: current_user,
       image: params.dig(:bike, :image), image_signed_id: params.dig(:bike, :image_signed_id),
       bike_params: update_params)
     # Saved either way, so the re-render has everything they entered
-    unless saved_step_2
-      @b_param.errors.add(:base, translation(:name_required))
+    unless saved
       return render(Register::Step2::Component.new(b_param: @b_param, sequence: @registration_sequence, current_user:),
         status: :unprocessable_entity)
     end
 
-    # An e-vehicle's safety pages come between the details and the bike
-    return redirect_to_current_step unless BikeServices::Register.acknowledged?(@b_param, sequence: @registration_sequence)
-
     complete_registration
   end
 
   # Each acknowledgment page posts here, and the review's final acknowledgment
   def acknowledge
     step = BikeServices::Register.permitted_step(@b_param, params[:step], sequence: @registration_sequence)
-    unless save_acknowledgment(step)
+    acknowledged = BikeServices::Register.acknowledge_step(@b_param, step,
+      sequence: @registration_sequence, user: current_user,
+      acknowledged_all: params[:acknowledged_all], checked: params[:acknowledged]&.to_unsafe_h&.values)
+    unless acknowledged
       flash[:error] = translation(:acknowledge_everything)
       return redirect_to step_path(step)
     end
@@ -114,8 +110,7 @@ def confirm_email
     # Single use, so a second click has nothing left to do - the first one signed them in
     return redirect_to_current_step if @b_param.email_confirmed?
 
-    if @b_param.email_confirmation_token_expired? ||
-        !Binxtils::Secure.compare?(params[:confirmation_token], @b_param.email_confirmation_token)
+    unless BikeServices::Register.confirmation_token_valid?(@b_param, params[:confirmation_token])
       BikeServices::Register.send_confirmation_email(@b_param)
       flash[:error] = translation(:confirmation_link_expired)
       return redirect_to_current_step
@@ -135,22 +130,10 @@ def confirm_email
 
   private
 
-  def save_acknowledgment(step)
-    if step == "review"
-      return BikeServices::Register.save_acknowledgment(@b_param, @registration_sequence,
-        acknowledged_all: params[:acknowledged_all], user: current_user)
-    end
-
-    BikeServices::Register.acknowledge_page(@b_param,
-      BikeServices::Register.page_for_step(step, sequence: @registration_sequence),
-      checked: params[:acknowledged]&.to_unsafe_h&.values)
-  end
-
   def complete_registration
-    BikeServices::Register.claim_creator(@b_param, current_user)
-    bike = BikeServices::Register.create_bike_if_ready(@b_param,
+    bike = BikeServices::Register.complete(@b_param, user: current_user,
       sequence: @registration_sequence, ip_address: forwarded_ip_address)
-    # No bike yet - everything stays on the b_param until the emailed link is clicked
+    # No bike yet - everything stays on the b_param until the flow's remaining steps are done
     return redirect_to_current_step if bike.blank?
 
     redirect_after_bike_creation(bike)
@@ -240,16 +223,11 @@ def redirect_after_bike_creation(bike)
   end
 
   def create_params
-    bike_params = params.require(:b_param).permit(:manufacturer_id, :cycle_type, :owner_email)
+    params.require(:b_param).permit(*BikeServices::Register.permitted_step_1_params)
       .to_h.merge(BParam.status_hash_from_params(params))
-    {bike: bike_params, propulsion_type_motorized: params[:propulsion_type_motorized]}
   end
 
   def update_params
-    params.fetch(:bike, {}).permit(:primary_frame_color_id, :secondary_frame_color_id,
-      :tertiary_frame_color_id, :serial_number, :frame_size, :frame_size_number, :frame_size_unit,
-      :bike_sticker, :phone, :status, :frame_model, :year, :user_name,
-      :extra_registration_number, :organization_affiliation, :student_id,
-      address_record_attributes: AddressRecord.permitted_params)
+    params.fetch(:bike, {}).permit(*BikeServices::Register.permitted_step_2_params)
   end
 end

app/services/bike_services/register.rb

@@ -117,6 +117,14 @@ def acknowledge_page(b_param, page, checked:)
       b_param.save
     end
 
+    # What a step's submission agrees to: an acknowledgment page's own boxes, or the
+    # single agreement the review ends with
+    def acknowledge_step(b_param, step, sequence:, user:, acknowledged_all:, checked:)
+      return save_acknowledgment(b_param, sequence, acknowledged_all:, user:) if step == "review"
+
+      acknowledge_page(b_param, page_for_step(step, sequence:), checked:)
+    end
+
     # The moment the pages become an agreement - promoted off the b_param onto a
     # record of its own, which outlives the registration
     def save_acknowledgment(b_param, sequence, acknowledged_all:, user: nil)
@@ -152,13 +160,44 @@ def send_confirmation_email(b_param)
       true
     end
 
+    # Time limited, so an old link proves nothing - the address gets a fresh one
+    def confirmation_token_valid?(b_param, token)
+      return false if b_param.email_confirmation_token_expired?
+
+      Binxtils::Secure.compare?(token, b_param.email_confirmation_token)
+    end
+
     # Whether a bike can be created now - Ownership requires a creator, so
     # anonymous registrations wait for the confirmation email to prove one
     def creator_available?(b_param)
       b_param.creator_id.present? || b_param.creation_organization&.auto_user_id.present? ||
         confirmed_email_creator_id(b_param).present?
     end
 
+    def permitted_step_1_params = %i[manufacturer_id cycle_type owner_email]
+
+    def permitted_step_2_params
+      [:primary_frame_color_id, :secondary_frame_color_id, :tertiary_frame_color_id,
+        :serial_number, :frame_size, :frame_size_number, :frame_size_unit, :bike_sticker,
+        :phone, :status, :frame_model, :year, :user_name, :extra_registration_number,
+        :organization_affiliation, :student_id,
+        {address_record_attributes: AddressRecord.permitted_params}]
+    end
+
+    # Step 1 is the least a registration can be: who owns it and what it is. The params are
+    # merged in whether or not it passes, so a re-render still shows everything they entered
+    def save_step_1(b_param, bike_params:, propulsion_type_motorized:)
+      b_param.clean_params({bike: bike_params, propulsion_type_motorized:}.as_json)
+      # Before save, which clears the errors it's about to re-run validations for
+      b_param.errors.add(:base, translation(:email_required)) if b_param.owner_email.blank?
+      b_param.errors.add(:base, translation(:manufacturer_required)) if b_param.manufacturer_id.blank?
+      return false if b_param.errors.any?
+      return true if b_param.save
+
+      b_param.errors.add(:base, translation(:unable_to_save))
+      false
+    end
+
     # Step 2 merges over step 1 - creator claimed for signed-in users, the photo and the
     # fields into the params json. The photo arrives one of two ways: as bytes from a plain
     # file field, or as the signed id of a blob the browser already uploaded.
@@ -171,9 +210,21 @@ def save_step_2(b_param, user:, image:, image_signed_id:, bike_params:)
       completed = b_param.self_made?(user) || bike_params["user_name"].present?
       b_param.clean_params(step_2_params(bike_params, image_signed_id:, completed:).as_json)
       b_param.save
+      b_param.errors.add(:base, translation(:name_required)) unless completed
       completed
     end
 
+    # Everything a submission does once its step is saved. Returns the bike, or nil while
+    # the registration is still short of one - more to enter, or the email unconfirmed
+    def complete(b_param, user:, sequence:, ip_address:)
+      claim_creator(b_param, user)
+      create_bike_if_ready(b_param, sequence:, ip_address:)
+    end
+
+    #
+    # private below here
+    #
+
     # Ownership requires a creator, and signing in can happen anywhere in the flow -
     # e.g. partway through the acknowledgment pages
     def claim_creator(b_param, user)
@@ -197,10 +248,6 @@ def create_bike(b_param, ip_address:)
       bike
     end
 
-    #
-    # private below here
-    #
-
     def ready_for_bike?(b_param, sequence:)
       details_completed?(b_param) && acknowledged?(b_param, sequence:)
     end
@@ -273,8 +320,11 @@ def step_2_params(bike_params, image_signed_id:, completed:)
       {details_completed: completed, bike: bike_params, image_signed_id: image_signed_id.presence}.compact
     end
 
-    conceal :ready_for_bike?, :reusable?, :all_steps, :permitted_steps,
-      :confirmed_email_creator_id, :owner_email_for, :assign_owner_email,
-      :details_completed?, :step_2_params
+    def translation(key) = I18n.t(key, scope: "shared.register_flow")
+
+    conceal :claim_creator, :create_bike_if_ready, :create_bike, :ready_for_bike?,
+      :reusable?, :all_steps, :permitted_steps, :confirmed_email_creator_id,
+      :owner_email_for, :assign_owner_email, :details_completed?, :step_2_params,
+      :translation
   end
 end

config/i18n-tasks.yml

@@ -172,6 +172,7 @@ ignore_unused:
   - "controllers.*" # our translation helper component blocks detection
   - "components.*" # our translation helper component blocks detection
   - "shared.organized_menu_items.*" # used dynamically by OrganizedServices::UserMenuItems
+  - "shared.register_flow.*" # used dynamically by BikeServices::Register
   - "*_mailer.*"
   - "date.*"
   - "datetime.*"

config/locales/en.yml

@@ -1926,18 +1926,12 @@ en:
         signed_in_as_other: >-
           You're signed in as %{email}, so we kept you in that account. The registration
           is still saved to the address we emailed.
-      create:
-        email_required: Email is required to register
-        manufacturer_required: Manufacturer is required to register
-        unable_to_save: We're unable to save that registration, please try again
       find_b_param:
         registration_not_found: We couldn't find that registration — start a new one
           below
       sign_in_confirmed_user:
         unable_to_sign_in: We confirmed your email, but couldn't sign you in to that
           account
-      update:
-        name_required: Owner name is required to register
     search:
       marketplace:
         set_interpreted_params:
@@ -5071,6 +5065,11 @@ en:
       bike: Bike
       recovered_date: recovered %{date}
       translated_from_english: translated from English
+    register_flow:
+      email_required: Email is required to register
+      manufacturer_required: Manufacturer is required to register
+      name_required: Owner name is required to register
+      unable_to_save: We're unable to save that registration, please try again
     shops_map:
       map_of_partner_locations: Map of partner locations
     user_general_alert: