apex-enterprise-patterns / fflib-apex-common

Common Apex Library supporting Apex Enterprise Patterns and much more!
BSD 3-Clause "New" or "Revised" License
899 stars 514 forks source link

add to DomainFactory and SelectorFactory instantiation by recordTypeId #471

Closed kamelhanna closed 9 months ago

kamelhanna commented 9 months ago

New simple functionality added to DomainFactory and SelectorFactory; be able to make new Domain and Selector by RecordTypeId.

Usecase: In your Org if you have different type of ex: Accounts by recordType, now you can make separate domain and selector of each.


This change is Reviewable

wimvelzeboer commented 9 months ago

Hi @kamelhanna, I do not fully understand why this change is needed. I believe you can already do everything with the existing logic, but then implemented differently.

How about the following:

IAccounts accounts = Accounts.newInstance(recordIds);   // Generic Accounts
IAccounts partnerAccounts = Accounts.newPartnerInstance(recordIds);  // Accounts with a certain recordTypeId

The domain would then implement the following static methods:

public static IAccounts newInstance(Set<Id> ids) 
{
  return (IAccounts) Application.Domain.newInstance(ids);
}
public static IAccounts newPartnerInstance(Set<Id> ids) 
{
  return ((IAccounts) Application.Domain.newInstance(ids))
          .selectByRecordTypeId(PARTNER_RECORDTYPE_ID);   // Filter method on the domain
}

This design pattern would make more sense, it also avoids having to get the record type Id everywhere where you need the domain instance.

This will also avoid the problem where you would create the domain with record Ids that do not belong to the desired recordtype, in this design they do get queried but filtered out.

You will not be able to use a different selector class with this design, but it goes against the design principles of FFLIB to have a list of records List<Account> with different fields. Whenever you come across an Account record you should be able to assume that all the default fields are present. If they are not, then the selector should return a wrapper class that contains the data.

kamelhanna commented 9 months ago

Hello @wimvelzeboer Hope all is well,

The main cause that forced us to go with separate domains and selector by recordTypeId is our Enterprise Org has different Account types each Account has separate business needs, from the other hand the Sample Code provided with Apex Common don't cover this use case so if there is a sample code cover this challenge provide it to me , and after research and discussion we recommend to use different domains according recordType.

But if the another best practice way to fit our big business requirements without using recordTypeId, we will use it.

Thanks a lot.

ImJohnMDaniel commented 9 months ago

G’day @kamelhanna -- First off, Thanks for submitting the PR.

The Selector and Domain Patterns call for only one Domain and only one Selector per SObject. Multiple domains and selectors per SObject are discouraged in the pattern based. In theory, with this change, you could setup differing domains and selectors over the same SObject by record type. That would in turn force the other consumers of the domains and selectors (i.e. Service tier, Client tier, other Domains, etc.) to have more knowledge over the SObject. This should be avoided as the Domain and Selector have primary responsibility for the SObject’s business logic (i.e. Domain class) and the SObject’s query paths (i.e. Selector class).

Again, while we appreciate your interest in the framework and the offer to help enhance it, we will not be accepting this PR because it is considered somewhat of an anti-pattern to the Domain and Selector patterns.

Please let us know if you have any questions.

ImJohnMDaniel commented 9 months ago

But if the another best practice way to fit our big business requirements without using recordTypeId, we will use it.

@kamelhanna, your logic in the Domain class needs to take appropriate steps based on the record type that it sees with the supplied records. The Selector methods would need to factor in conditions in the QueryFactory to filter to a specific record type, if that is the desire behavior.

wimvelzeboer commented 9 months ago

@kamelhanna If its indeed two complete different behaviors, then I would suggest to split the source code entirely into two separate packages. Each with their own Application class and domain/selector implementations.

I have worked in a similar Enterprise Org before. There we made sure that each triggerHandler/domain/selector classe only touched the records of its own record type.

So, in the trigger we basically did something like the following, (to prevent two triggers)

fflib_SObjectDomain.triggerHandler(AccountsTriggerHandler_ONE.class);
fflib_SObjectDomain.triggerHandler(AccountsTriggerHandler_TWO.class);

The Constructor class in each domain/TriggerHandler then had a filter build in:

    public class Constructor implements fflib_SObjectDomain.IConstructable2
    {
        public fflib_SObjectDomain construct(List<SObject> sObjectList)
        {
            return new AccountsTriggerHandler( filter(sObjectList) );
        }

        public fflib_SObjectDomain construct(List<SObject> sObjectList, SObjectType sObjectType)
        {
            return new AccountsTriggerHandler( filter(sObjectList) );
        }
        private List<SObject> filter(List<SObject> records)
        {
            // filter goes here.
        }
    }

I think we needed to do a few isEmpty checks here and there to make it work.

ImJohnMDaniel commented 9 months ago

Actually, @wimvelzeboer and @kamelhanna , having multiple Application factories in an org is also an anti-pattern.

The idea is that the Application Factory needs to be universally accessible to every app in the org so you don't end up with differing implementations. There are definitely ways for each application to add domain processes specific to the app or to add additional Selector methods or UnitOfWork bindings, but everything in the org should look to a single Application Factory to supply all of its implementations.

Please let me know if you have questions.

cc: @daveespo @stohn777

kamelhanna commented 9 months ago

@kamelhanna If its indeed two complete different behaviors, then I would suggest to split the source code entirely into two separate packages. Each with their own Application class and domain/selector implementations.

I have worked in a similar Enterprise Org before. There we made sure that each triggerHandler/domain/selector classe only touched the records of its own record type.

So, in the trigger we basically did something like the following, (to prevent two triggers)

fflib_SObjectDomain.triggerHandler(AccountsTriggerHandler_ONE.class);
fflib_SObjectDomain.triggerHandler(AccountsTriggerHandler_TWO.class);

The Constructor class in each domain/TriggerHandler then had a filter build in:

  public class Constructor implements fflib_SObjectDomain.IConstructable2
  {
      public fflib_SObjectDomain construct(List<SObject> sObjectList)
      {
          return new AccountsTriggerHandler( filter(sObjectList) );
      }

      public fflib_SObjectDomain construct(List<SObject> sObjectList, SObjectType sObjectType)
      {
          return new AccountsTriggerHandler( filter(sObjectList) );
      }
      private List<SObject> filter(List<SObject> records)
      {
          // filter goes here.
      }
  }

I think we needed to do a few isEmpty checks here and there to make it work.

@wimvelzeboer, In my opinion, the idea of filtering records according recordType is more best practice than do that, really I can't thank you enough for your time.

wimvelzeboer commented 9 months ago

@ImJohnMDaniel The idea of those two Application classes, in that particular project, was to eventually split the entire Org into two separate managed packages.

ImJohnMDaniel commented 9 months ago

@wimvelzeboer just to be clear, you're saying that they wanted to take an existing Org and split it into two "managed packages" or "unlocked packages"?

wimvelzeboer commented 9 months ago

@ImJohnMDaniel Yes, to two separate locked managed packages. In the process of unraveling the dependencies we created these two Application classes. In the end they chose to have two separate Orgs..

ImJohnMDaniel commented 9 months ago

Well okay. I do find it curious that they went to the managed package route and not unlocked packages for standard org development; even if the intent is to eventually split to multiple orgs. The introduction of the namespace adds a complexity that is probably unnecessary and forces subsequent development in the respective orgs to have additional constraints that would be avoided if they stay with unlocked packages.