change association between transmittable application and applicants#278
change association between transmittable application and applicants#278
Conversation
ymhari
added
the
cr-120+
WalkthroughTransmittable::Applicant was converted from an embedded document to a referenced model (belongs_to Transmittable::Application with index). Transmittable::Application now has_many applicants. The fdsh/ssa_vlp/rj3 operation now persists Application earlier and creates/persists Applicant records immediately. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Op as HandleVerificationRequest
participant Application as Transmittable::Application
participant Applicant as Transmittable::Applicant
rect #E6F4EA
Note over Op,Application: Persist parent first
Client->>Op: submit verification request
Op->>Application: create + save Application
end
rect #FFF4E6
Note over Op,Applicant: Create and persist applicants immediately
Op->>Applicant: application.applicants.create!(data)
Applicant-->>Application: belongs_to -> application_id
end
Op-->>Client: return result (persisted application with applicants)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/transmittable/applicant.rb(1 hunks)app/models/transmittable/application.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-upload-image
- GitHub Check: rspec
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (1)
66-76: Guard against nil application and fix unsafe error dereference (possible NoMethodError).create_transmittable_application returns nil on save failure, but add_applicants_to_application is called unconditionally and the error message uses application.errors..., both can blow up.
Apply this restructuring to fail fast before using application and avoid nil deref:
def create_applicant_subjects(values, application_type) primary_applicant = find_primary_applicant return Failure("No primary applicant found") unless primary_applicant - application = create_transmittable_application(values, primary_applicant) - add_applicants_to_application(application, application_type) - - return Success(application) if application.persisted? + application = create_transmittable_application(values, primary_applicant) + unless application&.persisted? + add_errors({ job: @job }, "Unable to create applicant subject due to #{application&.errors&.full_messages || 'application save returned nil'}", + :create_applicant_subject) + status_result = update_status({ job: @job }, :failed, "Unable to save applicant subject") + return status_result if status_result.failure? + return Failure("Unable to save application subject") + end + + add_applicants_to_application(application, application_type) + + return Success(application) @@ - application.save ? application : nil + application.save ? application : nilOptional follow-up (for consistency with Dry::Monads): make create_transmittable_application return Success/Failure and
yieldit here.Also applies to: 97-97
♻️ Duplicate comments (1)
app/models/transmittable/application.rb (1)
26-26: Add dependent cleanup and inverse_of on has_many to prevent orphaned applicants.Children won’t be auto-removed when the application is deleted. Also set inverse_of for better identity map behavior.
-has_many :applicants, class_name: 'Transmittable::Applicant' +has_many :applicants, + class_name: 'Transmittable::Applicant', + inverse_of: :application, + dependent: :destroy
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/transmittable/application.rb(1 hunks)app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (1)
app/models/transaction.rb (1)
applicants(54-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-upload-image
- GitHub Check: rspec
🔇 Additional comments (1)
app/models/transmittable/application.rb (1)
21-21: Mongoid uses_typeby default — don't assumetypeis the inheritance discriminator; confirm before renaming. Mongoid stores class discriminators in_typeby default and lets you change the discriminator key viaself.discriminator_keyorMongoid.discriminator_key; check your codebase (global/class-level discriminator_key) and Mongoid.destructive_fields before migrating this field. (mongodb.com)
Location: app/models/transmittable/application.rb (line 21). Rename to e.g.application_typeand update accessors only if your app requires avoiding ambiguity or if the discriminator key is set to"type".Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (2)
66-86: Fix ordering and nil-handling to avoid NoMethodError and premature child creation
- create_transmittable_application may yield nil; calling add_applicants_to_application and then application.persisted? will raise.
- Error logging uses application.errors without guarding against nil.
- Only create applicants after confirming the parent is persisted.
Apply:
def create_applicant_subjects(values, application_type) primary_applicant = find_primary_applicant return Failure("No primary applicant found") unless primary_applicant - application = create_transmittable_application(values, primary_applicant) - add_applicants_to_application(application, application_type) - - return Success(application) if application.persisted? - - add_errors({ job: @job }, "Unable to create applicant subject due to #{application.errors&.full_messages}", - :create_applicant_subject) - status_result = update_status({ job: @job }, :failed, "Unable to save applicant subject") - return status_result if status_result.failure? - Failure("Unable to save application subject") + application = create_transmittable_application(values, primary_applicant) + unless application&.persisted? + add_errors({ job: @job }, "Unable to create applicant subject due to #{application&.errors&.full_messages}", + :create_applicant_subject) + status_result = update_status({ job: @job }, :failed, "Unable to save applicant subject") + return status_result if status_result.failure? + return Failure("Unable to save application subject") + end + + add_applicants_to_application(application, application_type) + return Success(application) rescue StandardError => e
88-98: Return the application instance even when save fails to surface validation errorsReturning nil loses validation errors and contributes to the nil hazards above. Keep the instance and still run validations.
- application.save ? application : nil + application.tap(&:save)
♻️ Duplicate comments (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (1)
214-215: Good: switched to create! to surface validation failuresThis resolves the prior silent-failure risk noted earlier. 👍
🧹 Nitpick comments (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (1)
206-216: Consider wrapping applicant creation in a transaction to avoid partial insertsIf one applicant fails, earlier ones remain persisted, which may be undesirable. Use a transaction to ensure all-or-nothing (or confirm partial persistence is acceptable).
def add_applicants_to_application(application, application_type) - eligible_applicants(application_type).each do |applicant| - params = if faa_application?(application_type) - faa_applicant_params(applicant) - else - qhp_applicant_params(applicant) - end - - application.applicants.create!(params) - end + Transmittable::Applicant.transaction do + eligible_applicants(application_type).each do |applicant| + params = if faa_application?(application_type) + faa_applicant_params(applicant) + else + qhp_applicant_params(applicant) + end + application.applicants.create!(params) + end + end endWould you like this broadened to include the application save as well (wrapping both parent and children for atomicity)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/operations/fdsh/ssa_vlp/rj3/handle_verification_request.rb (1)
app/models/transaction.rb (1)
applicants(54-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-upload-image
- GitHub Check: rspec
3 participants
https://app.clickup.com/t/868fh8mqf
Summary by CodeRabbit