Added Agent Sales Seeder + Fix Sold Appliance page

[beesaferoot]

Brief summary of the change made

These changes allows us to generate data from agent to customer relationships, by which we can then query upon these interactions.

The PR also includes minor big fixes on the UI related to appliance page.

Are there any other side effects of this change that we should be aware of?

Describe how you tested your changes?

Pull Request checklist

Please confirm you have completed any of the necessary steps below.

• Meaningful Pull Request title and description
• Changes tested as described above
• Added appropriate documentation for the change.
• Created GitHub issues for any relevant followup/future enhancements if appropriate.

[dmohns]

Hey @beesaferoot

Thanks for the PR. I was not expecting this topic to be so big 🙈

There is a couple of things that I don't like

• In the newly created processSaleFromFactory we are essentially "seeding" some data with things like 'tenure' => rand(6, 24),. I know it's minor, but still. I'd love to have all seeding code in seeders.
• There is a lot of redundancy between processSaleFromFactory and processSaleIfIsNotCreatedByFactory which will lead to hard-to-maintain code.

Both of the above aren't really flaws of your solution. But I think there is a bigger underlying problem in the code base here:

The AgentSoldApplianceObserver seems to heavily rely on request()->input(). This feels wrong on so many levels.

• Currently the Observer implements a lot of business logic like creating model objects from requests and setting relation ships between them. By understanding is that Observer should react to CRUD operations rather than performing them.
• Observer cannot be used outside of Request context. This is exactly the problem you are facing, which leads to a lot of code duplication.

What do you think about the following suggestion:

1. Let's merge your changes the way they are, so we can benefit from the additional Seeding data generation.
2. Let's create a follow-up ticket to refactor AgentSoldApplianceObserver in a way that it doesn't require `request()->input().

Number 2. will be quite big, as it will require shuffling a lot of code around from the Observer into corresponding Services and Controllers. But I think it would be a valuable addition and improvement to the code base.

WDYT?

[dmohns]

PS: @alaincodes24 I would also like to hear your opinion here

[beesaferoot]

Hey @beesaferoot

Thanks for the PR. I was not expecting this topic to be so big 🙈

There is a couple of things that I don't like

• In the newly created processSaleFromFactory we are essentially "seeding" some data with things like 'tenure' => rand(6, 24),. I know it's minor, but still. I'd love to have all seeding code in seeders.
• There is a lot of redundancy between processSaleFromFactory and processSaleIfIsNotCreatedByFactory which will lead to hard-to-maintain code.

Both of the above aren't really flaws of your solution. But I think there is a bigger underlying problem in the code base here:

The AgentSoldApplianceObserver seems to heavily rely on request()->input(). This feels wrong on so many levels.

• Currently the Observer implements a lot of business logic like creating model objects from requests and setting relation ships between them. By understanding is that Observer should react to CRUD operations rather than performing them.
• Observer cannot be used outside of Request context. This is exactly the problem you are facing, which leads to a lot of code duplication.

What do you think about the following suggestion:

1. Let's merge your changes the way they are, so we can benefit from the additional Seeding data generation.
2. Let's create a follow-up ticket to refactor AgentSoldApplianceObserver in a way that it doesn't require `request()->input().

Number 2. will be quite big, as it will require shuffling a lot of code around from the Observer into corresponding Services and Controllers. But I think it would be a valuable addition and improvement to the code base.

WDYT?

Thanks for the review @dmohns, I agree it does need some refactoring and cleanup.

Although one of the reasons I opted to use an entirely different function for seeding purposes was because it wasn't clear why the request guard was important and I didn't want to go too deep while still focusing on the high-level task.

[alaincodes24]

@dmohns I agree that the observer shouldn't be tied to request()->input() since it could lead to issues with duplicated code. @beesaferoot Before merging the PR, I propose we move the seeding code inside seeders for better separation of concerns.

[beesaferoot]

I propose we move the seeding code inside seeders for better separation of concerns.

@alaincodes24 Do you mind clarifying a bit more on this? Perhaps you can point to an example where the seed code is used outside a seeder.

I think I now understand what you are referring to. The problem is that to refactor this I need to refactor the Observer method processSaleFromFactory entirely, as the "seeding" part is actually for the request data and not a Model which I am not sure in what seeder that should to in.

[beesaferoot]

Updated, kindly review @dmohns @alaincodes24

[dmohns]

This looks great. Two follow up questions:

1. When as we are changing the request()->input() logic I would want to be able to test that AgentSoldAppliance API endpoints still work as expected. Could you add an example HTTP collection with some dummy data, so we can test before and after the change?
2. I'm a bit confused as to why we need to add columns to agent_sold_appliance table. Where is this data stored right now?

[beesaferoot]

This looks great. Two follow up questions:

1. When as we are changing the request()->input() logic I would want to be able to test that AgentSoldAppliance API endpoints still work as expected. Could you add an example HTTP collection with some dummy data, so we can test before and after the change?
2. I'm a bit confused as to why we need to add columns to agent_sold_appliance table. Where is this data stored right now?

I will be including a collection for this endpoint as noted.

For 2) so from my investigation, fields like down_payment and tenure used in association with asset table. As you pointed out in your previous comment, the Observer logic needed these fields for handling model creation that needed this inform but were not available in the AgentSoldAppliance model, hence the need to reference the request directly.

I considered two approaches:
1.) Provide these fields in the agent_sold_appliances table thereby removing a need for referencing request.
2.) Provide these fields as temporary data in the AgentSoldAppliance model but not on the table. This require extra logic to achieve.

After discusing a bit with Alain about a more idiomatic approach, I decided 1.) works well and is more laravel like.

[beesaferoot]

Added sample endpoint (app) usage as bruno collections.

@dmohns

[dmohns]

I meant something slightly different 🙈

We already have the columns down_payment, tenure (a.k.a rate_count) and first_payment_date in our database. Namely, on the asset_people table.

It feels a bit wrong to add the same fields to another model (agent_sold_appliances). Because then we would need to make sure the two are in sync.

I understand why this is needed in the current approach: Because currently the AgentSoldApplianceObserver creates this AgentPerson object.

Essentially, what I'm wondering is if we should move the responsibilities of this current Observer into Controller and Services.

[dmohns]

Looks good 👍 Two more small comments

[dmohns]

One last comment

[dmohns]

Really good work 🤩 Thanks for the patience!