Ensure reliable user registration including creating person community membership

Author: rsmithlal

.rubocop.yml

@@ -1,3 +1,5 @@
+inherit_from: .rubocop_todo.yml
+
 AllCops:
   Exclude:
     - 'bin/*'

.rubocop_todo.yml

@@ -0,0 +1,43 @@
+# This configuration was generated by
+# `rubocop --auto-gen-config`
+# on 2025-11-04 02:06:37 UTC using RuboCop version 1.81.6.
+# The point is for the user to remove these configuration records
+# one by one as the offenses are removed from the code base.
+# Note that changes in the inspected code, or installation of new
+# versions of RuboCop, may require this file to be generated again.
+
+# Offense count: 52
+# This cop supports safe autocorrection (--autocorrect).
+Lint/RedundantCopDisableDirective:
+  Enabled: false
+
+# Offense count: 2
+# Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns.
+Metrics/MethodLength:
+  Max: 14
+
+# Offense count: 1
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: EnforcedStyle, SingleLineConditionsOnly, IncludeTernaryExpressions.
+# SupportedStyles: assign_to_condition, assign_inside_condition
+Style/ConditionalAssignment:
+  Exclude:
+    - 'app/controllers/better_together/joatu/hub_controller.rb'
+
+# Offense count: 29
+# This cop supports safe autocorrection (--autocorrect).
+Style/IfUnlessModifier:
+  Enabled: false
+
+# Offense count: 1
+# Configuration parameters: Max.
+Style/SafeNavigationChainLength:
+  Exclude:
+    - 'spec/features/devise/registration_spec.rb'
+
+# Offense count: 1
+# This cop supports safe autocorrection (--autocorrect).
+# Configuration parameters: AllowHeredoc, AllowURI, AllowQualifiedName, URISchemes, IgnoreCopDirectives, AllowedPatterns, SplitStrings.
+# URISchemes: http, https
+Layout/LineLength:
+  Max: 145

app/controllers/better_together/users/registrations_controller.rb

@@ -7,6 +7,7 @@ class RegistrationsController < ::Devise::RegistrationsController # rubocop:todo
       include DeviseLocales
 
       skip_before_action :check_platform_privacy
+      before_action :configure_permitted_parameters
       before_action :set_required_agreements, only: %i[new create]
       before_action :set_event_invitation_from_session, only: %i[new create]
       before_action :configure_account_update_params, only: [:update]
@@ -63,18 +64,12 @@ def update # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
 
       def new
         super do |user|
-          # Pre-fill email from platform invitation
-          user.email = @platform_invitation.invitee_email if @platform_invitation && user.email.empty?
-
-          if @event_invitation
-            # Pre-fill email from event invitation
-            user.email = @event_invitation.invitee_email if @event_invitation && user.email.empty?
-            user.person = @event_invitation.invitee if @event_invitation.invitee.present?
-          end
+          setup_user_from_invitations(user)
+          user.build_person unless user.person
         end
       end
 
-      def create # rubocop:todo Metrics/MethodLength
+      def create # rubocop:todo Metrics/MethodLength, Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
         unless agreements_accepted?
           handle_agreements_not_accepted
           return
@@ -87,11 +82,25 @@ def create # rubocop:todo Metrics/MethodLength
           return
         end
 
+        # Use transaction for all user creation and associated records
         ActiveRecord::Base.transaction do
-          super do |user|
-            handle_user_creation(user) if user.persisted?
+          # Call Devise's default create behavior
+          super
+
+          # Handle post-registration setup if user was created successfully
+          if resource.persisted? && resource.errors.empty?
+            handle_user_creation(resource)
+          elsif resource.persisted?
+            # User was created but has errors - rollback to maintain consistency
+            raise ActiveRecord::Rollback
           end
         end
+      rescue ActiveRecord::RecordInvalid, ActiveRecord::InvalidForeignKey => e
+        # Clean up and show user-friendly error
+        Rails.logger.error "Registration failed: #{e.message}"
+        build_resource(sign_up_params) if resource.nil?
+        resource&.errors&.add(:base, 'Registration could not be completed. Please try again.')
+        respond_with resource
       end
 
       protected
@@ -105,6 +114,10 @@ def configure_account_update_params
                                           keys: %i[email password password_confirmation current_password])
       end
 
+      def configure_permitted_parameters
+        devise_parameter_sanitizer.permit(:sign_up, keys: [person_attributes: %i[identifier name description]])
+      end
+
       def set_required_agreements
         @privacy_policy_agreement = BetterTogether::Agreement.find_by(identifier: 'privacy_policy')
         @terms_of_service_agreement = BetterTogether::Agreement.find_by(identifier: 'terms_of_service')
@@ -170,32 +183,116 @@ def handle_agreements_not_accepted
         respond_with resource
       end
 
+      def setup_user_from_invitations(user)
+        # Pre-fill email from platform invitation
+        user.email = @platform_invitation.invitee_email if @platform_invitation && user.email.empty?
+
+        return unless @event_invitation
+
+        # Pre-fill email from event invitation
+        user.email = @event_invitation.invitee_email if @event_invitation && user.email.empty?
+        user.person = @event_invitation.invitee if @event_invitation.invitee.present?
+      end
+
       def handle_user_creation(user)
-        setup_person_for_user(user)
-        return unless user.save!
+        return unless event_invitation_person_updated?(user)
 
+        # Reload user to ensure all nested attributes and associations are properly loaded
         user.reload
-        setup_community_membership(user)
+        person = user.person
+
+        return unless person_persisted?(user, person)
+
+        setup_community_membership(user, person)
         handle_platform_invitation(user)
         handle_event_invitation(user)
-        create_agreement_participants(user.person)
+        create_agreement_participants(person)
+      end
+
+      def event_invitation_person_updated?(user)
+        return true unless @event_invitation&.invitee.present?
+
+        return true if user.person.update(person_params)
+
+        Rails.logger.error "Failed to update person for event invitation: #{user.person.errors.full_messages}"
+        false
+      end
+
+      def person_persisted?(user, person)
+        return true if person&.persisted?
+
+        Rails.logger.error "Person not found or not persisted for user #{user.id}"
+        false
       end
 
       def setup_person_for_user(user)
-        if @event_invitation && @event_invitation.invitee.present?
-          user.person = @event_invitation.invitee
-          user.person.update(person_params)
-        else
-          user.build_person(person_params)
-        end
+        return update_existing_person_for_event(user) if @event_invitation&.invitee.present?
+
+        create_new_person_for_user(user)
+      end
+
+      def update_existing_person_for_event(user)
+        user.person = @event_invitation.invitee
+        return if user.person.update(person_params)
+
+        Rails.logger.error "Failed to update person for event invitation: #{user.person.errors.full_messages}"
+        user.errors.add(:person, 'Could not update person information')
       end
 
-      def setup_community_membership(user)
+      def create_new_person_for_user(user)
+        return handle_empty_person_params(user) if person_params.empty?
+
+        user.build_person(person_params)
+        return unless person_validated_and_saved?(user)
+
+        save_person_identification(user)
+      end
+
+      def handle_empty_person_params(user)
+        Rails.logger.error 'Person params are empty, cannot build person'
+        user.errors.add(:person, 'Person information is required')
+      end
+
+      def person_validated_and_saved?(user)
+        return save_person?(user) if user.person.valid?
+
+        Rails.logger.error "Person validation failed: #{user.person.errors.full_messages}"
+        user.errors.add(:person, 'Person information is invalid')
+        false
+      end
+
+      def save_person?(user)
+        return true if user.person.save
+
+        Rails.logger.error "Failed to save person: #{user.person.errors.full_messages}"
+        user.errors.add(:person, 'Could not save person information')
+        false
+      end
+
+      def save_person_identification(user)
+        person_identification = user.person_identification
+        return if person_identification&.save
+
+        Rails.logger.error "Failed to save person identification: #{person_identification&.errors&.full_messages}"
+        user.errors.add(:person, 'Could not link person to user')
+      end
+
+      def setup_community_membership(user, person_param = nil)
+        person = person_param || user.person
         community_role = determine_community_role
-        helpers.host_community.person_community_memberships.find_or_create_by!(
-          member: user.person,
-          role: community_role
-        )
+
+        begin
+          helpers.host_community.person_community_memberships.find_or_create_by!(
+            member: person,
+            role: community_role
+          )
+        rescue ActiveRecord::InvalidForeignKey => e
+          Rails.logger.error "Foreign key violation creating community membership: #{e.message}"
+          raise e
+        rescue StandardError => e
+          Rails.logger.error "Unexpected error creating community membership: #{e.message}"
+          raise e
+        end
       end
 
       def handle_platform_invitation(user)
@@ -235,10 +332,18 @@ def after_update_path_for(_resource)
       end
 
       def person_params
+        return {} unless params[:user] && params[:user][:person_attributes]
+
         params.require(:user).require(:person_attributes).permit(%i[identifier name description])
+      rescue ActionController::ParameterMissing => e
+        Rails.logger.error "Missing person parameters: #{e.message}"
+        {}
       end
 
       def agreements_accepted?
+        # Ensure required agreements are set
+        set_required_agreements if @privacy_policy_agreement.nil?
+
         required = [params[:privacy_policy_agreement], params[:terms_of_service_agreement]]
         # If a code of conduct agreement exists, require it as well
         required << params[:code_of_conduct_agreement] if @code_of_conduct_agreement.present?
@@ -247,11 +352,21 @@ def agreements_accepted?
       end
 
       def create_agreement_participants(person)
+        unless person&.persisted?
+          Rails.logger.error 'Cannot create agreement participants - person not persisted'
+          return
+        end
+
         identifiers = %w[privacy_policy terms_of_service]
         identifiers << 'code_of_conduct' if BetterTogether::Agreement.exists?(identifier: 'code_of_conduct')
         agreements = BetterTogether::Agreement.where(identifier: identifiers)
+
         agreements.find_each do |agreement|
-          BetterTogether::AgreementParticipant.create!(agreement: agreement, person: person, accepted_at: Time.current)
+          BetterTogether::AgreementParticipant.create!(
+            agreement: agreement,
+            person: person,
+            accepted_at: Time.current
+          )
         end
       end
     end

app/models/better_together/identification.rb

@@ -10,6 +10,8 @@ class Identification < ApplicationRecord
                polymorphic: true,
                autosave: true
 
+    accepts_nested_attributes_for :identity
+
     validates :identity,
               presence: true
     validates :agent,

app/models/better_together/user.rb

@@ -33,31 +33,40 @@ class User < ApplicationRecord
     delegate :permitted_to?, to: :person, allow_nil: true
 
     def build_person(attributes = {})
-      build_person_identification(
+      identification = build_person_identification(
         agent: self,
-        identity: ::BetterTogether::Person.new(attributes)
-      )&.identity
+        identity_type: 'BetterTogether::Person',
+        active: true
+      )
+      person = ::BetterTogether::Person.new(attributes)
+      identification.identity = person
+      person
     end
 
-    # Define person_attributes method to get attributes of associated Person
+    # Override person method to ensure it builds if needed
     def person
-      # Check if a Person object exists and return its attributes
-      super.present? ? super : build_person # this build_person call can cause issues in registration
+      person_identification&.identity
     end
 
-    # def person= arg
-    #     build_person_identification(
-    #     agent: self,
-    #     identity: arg
-    #   )&.identity
-    # end
+    # Custom person= method
+    def person=(person_obj)
+      if person_identification
+        person_identification.identity = person_obj
+      else
+        build_person_identification(
+          agent: self,
+          identity: person_obj,
+          identity_type: 'BetterTogether::Person',
+          active: true
+        )
+      end
+    end
 
-    # Define person_attributes= method
+    # Define person_attributes= method for nested attributes
     def person_attributes=(attributes)
-      # Check if a Person object already exists
-      if person.present?
+      if person_identification&.identity
         # Update existing Person object
-        person.update(attributes)
+        person_identification.identity.assign_attributes(attributes)
       else
         # Build new Person object if it doesn't exist
         build_person(attributes)