New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Lead Promotion Logic to Improve Separation of Concerns #1398

Open

TimothyKuehn wants to merge 3 commits into fatfreecrm:master from TimothyKuehn:master

TimothyKuehn commented Dec 8, 2024

Pull Request Summary

This pull request refactors the promotion logic for Leads to align with best practices in software design, specifically addressing concerns around bounded contexts and the single responsibility principle.

Changes Made:

Created LeadsPromotionHelper.rb:
- Introduced a dedicated helper file to encapsulate the business logic for promoting leads.
- This centralizes the promotion-related responsibilities in a single, focused entity.
Updated LeadsController:
- Modified the controller to utilize LeadsPromotionHelper.rb for handling lead promotions.
- Simplifies the controller by delegating the logic to a dedicated helper.
Removed promotion logic from Lead.rb:
- Extracted the logic to avoid Lead.rb handling responsibilities outside its bounded context.
- Improves adherence to the single responsibility principle by decoupling promotion logic from the Lead model.

Reasoning:

The previous implementation violated Domain-Driven Design (DDD) principles, as the Lead model managed adjustments to Account, Opportunity, and Contact. This approach entangled responsibilities, increasing complexity and making the code harder to maintain.

By isolating the promotion logic into LeadsPromotionHelper.rb, we:

Maintain a clean separation of concerns.
Create a more cohesive design, where each entity focuses on its intended responsibilities.
Enhance the maintainability and testability of the codebase.

TimothyKuehn added 3 commits

December 8, 2024 10:42


          added leadeds change to move promote to follow DDD principles


          fixing whitespace issue in leads_helper.rb


          fixing whitespace issue in leads_helper.rb end of file

2857cc9

github-advanced-security bot found potential problems

View reviewed changes

app/controllers/entities/leads_controller.rb

    
                def promote

                  @account, @opportunity, @contact = @lead.promote(params.permit!)

              def promote

                  @account, @opportunity, @contact = promote_lead(@lead, params.permit!)

Check warning

Code scanning / Brakeman

Specify exact keys allowed for mass assignment instead of using permit! which allows any keys. Warning

Specify exact keys allowed for mass assignment instead of using permit! which allows any keys.

github-advanced-security bot found potential problems

View reviewed changes

app/controllers/entities/leads_controller.rb

@@ @@ -115,8 +116,8 @@ @@
                 # PUT /leads/1/promote
                 #----------------------------------------------------------------------------
-                def promote
-                  @account, @opportunity, @contact = @lead.promote(params.permit!)
+              def promote

Check notice

Code scanning / Rubocop

Keep indentation straight. Note

Layout/IndentationConsistency: Inconsistent indentation detected.

app/controllers/entities/leads_controller.rb

@@ @@ -115,8 +116,8 @@ @@
                 # PUT /leads/1/promote
                 #----------------------------------------------------------------------------
-                def promote
-                  @account, @opportunity, @contact = @lead.promote(params.permit!)
+              def promote

Check notice

Code scanning / Rubocop

Use 2 spaces for indentation. Note

Layout/IndentationWidth: Use 2 (not 0) spaces for indentation.

app/controllers/entities/leads_controller.rb

    
                def promote

                  @account, @opportunity, @contact = @lead.promote(params.permit!)

              def promote

                  @account, @opportunity, @contact = promote_lead(@lead, params.permit!)

Check notice

Code scanning / Rubocop

Use 2 spaces for indentation. Note

Layout/IndentationWidth: Use 2 (not 4) spaces for indentation.

app/controllers/entities/leads_controller.rb

@@ @@ -270,4 +271,4 @@ @@
                 end
                 ActiveSupport.run_load_hooks(:fat_free_crm_leads_controller, self)
-              end
+              end

Check notice

Code scanning / Rubocop

Checks trailing blank lines and final newline. Note

Layout/TrailingEmptyLines: Final newline missing.

app/helpers/lead_promotion_helper.rb

+              module LeadPromotionHelper
+                  # Promote the lead by creating a contact and optional opportunity. Upon
+                  # successful promotion Lead status gets set to :converted.
+                  #----------------------------------------------------------------------------

Check notice

Code scanning / Rubocop

Avoid trailing whitespace. Note

Layout/TrailingWhitespace: Trailing whitespace detected.

app/helpers/lead_promotion_helper.rb

+                  # Promote the lead by creating a contact and optional opportunity. Upon
+                  # successful promotion Lead status gets set to :converted.
+                  #----------------------------------------------------------------------------
+                  def promote_lead(lead, params)

Check notice

Code scanning / Rubocop

Use 2 spaces for indentation. Note

Layout/IndentationWidth: Use 2 (not 4) spaces for indentation.

app/helpers/lead_promotion_helper.rb

+                  def promote_lead(lead, params)
+                    account_params = params[:account] || {}
+                    opportunity_params = params[:opportunity] || {}

Check notice

Code scanning / Rubocop

Avoid trailing whitespace. Note

Layout/TrailingWhitespace: Trailing whitespace detected.

app/helpers/lead_promotion_helper.rb

+                    account     = Account.create_or_select_for(lead, account_params)
+                    opportunity = Opportunity.create_for(lead, account, opportunity_params)
+                    contact     = Contact.create_for(lead, account, opportunity, params)

Check notice

Code scanning / Rubocop

Avoid trailing whitespace. Note

Layout/TrailingWhitespace: Trailing whitespace detected.

app/helpers/lead_promotion_helper.rb

+                    [account, opportunity, contact]
+                  end
+                end

Check notice

Code scanning / Rubocop

Avoid trailing whitespace. Note

Layout/TrailingWhitespace: Trailing whitespace detected.

app/models/entities/lead.rb

@@ @@ -184,4 +170,4 @@ @@
                 end
                 ActiveSupport.run_load_hooks(:fat_free_crm_lead, self)
-              end
+              end

Check notice

Code scanning / Rubocop

Checks trailing blank lines and final newline. Note

Layout/TrailingEmptyLines: Final newline missing.

Member

CloCkWeRX commented Dec 11, 2024

Probably not mergable as is.

Doesn't add tests or a compelling real world use case for why.
This takes a relatively clean method mutating a related object graph and puts it into a helper, but brings in controller concerns (params) when it does that.
IE: If downstream users of FFCRM are working with the lead model in a rake task; how do they migrate in your approach?

A service object would perhaps be better fit for this
https://jardo.dev/rails-service-objects

Even then, the litmus test is still the migration costs vs benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet