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

With latest fflib, do TriggerHandlers become service class consumers? If so what happens to UoW? #443

Closed parsam97 closed 1 year ago

parsam97 commented 1 year ago

Preface: I have a couple questions. I'm new to both fflib and posting on github so my apologies if the format is wrong. Please inform me and I shall learn!

With the latest fflib, the trigger handler and domain logic were separated. I can see from the changes to the repo that previously, the Opportunities.cls domain class' onAfterInsert() method updates the account's fields by instantiating a UoW and an Accounts.cls domain class and calling its appropriate method and passing the UoW to finally commit the change.

Now with the separation, the logic in updateOpportunityActivityOnAccount, which is called with onAfterInsert(), calls the updateOpportunityActivitymethod on AccountsService.cls.

Question 1

Is it correct to say the TriggerHandler classes can call service methods if cross object updating is required?

Question 2

As I understand it, public domain methods take unitOfWork as a parameter. This becomes confusing if the TriggerHandler class is going to call a domain's method in a before context where unitOfWork is not needed. For example, consider the following scenario where the TriggerHandler class calls a method on the context record's domain class, but has to instantiate an unnecessary unitOfWork due to calling context being from onBeforeInsert.

InteractionsTriggerHandler.cls

    public override void onBeforeInsert()
    {
        fflib_ISObjectUnitOfWork uow = new fflib_SObjectUnitOfWork(
                                new Schema.SObjectType[] { Interaction__c.SObjectType }); 

        // Assign ownership
        IInteractions interactions = Interactions.newInstance(this.records);
        interactions.assignOwnership(uow);
    }

Interactions.cls

    public void assignOwnership(fflib_ISObjectUnitOfWork uow)
    {
        // Collect channels
        Map<Id, Channel__c> channelsById = 
            new Map<Id, Channel__c>( (List<Channel__c>) Application.Selector.selectById( getChannelIds() ) );

        for (Interaction__c interaction : (List<Interaction__c>) getRecords())
        {
            // Get channel
            Channel__c channel = channelsById.get(interaction.Interaction_Channel__c);

            // Assign ownership
            interaction.OwnerId = channel.OwnerId;

            uow.registerDirty(interaction);
        }
    }

Of course, the unitOfWork parameter on the domain class is useful for when the method is called from a service such as InteractionsServiceImpl, but is not always necessary as seen above. What is the correct way of handling that?

wimvelzeboer commented 1 year ago

Hi @parsam97,

The thought behind splitting the domain from the triggerhandler is to limit the scope of the domain class. The domain is now just a wrapper around a List of SObjects, providing methods to get & set fields, and to filter the SObjects. It shouldn't do something more. If you would to combine a UnitOfWork with a domain method, that will immediately render that method useless if you want to use the same domain method in a before Insert/update context. I would leave the UnitOfWork to the abstraction level that is calling the domain methods, e.g. a service method. Alternatively you could add method overloads in the domain to handle that (see my example at the bottom).

Looking at your example, One thing I notice is that the domain goes beyond its scope, and invokes a selector. In those cases you would want to introduce a service method. I also see that the trigger runs on Interaction__c in a before insert context, which means that you do not require a unitOfWork. You can just set the fields with a domain setter method.

I would do something like:

InteractionsTriggerHandler.cls

public override void onBeforeInsert()
{
  InteractionsService.assignOwnership(getRecords());
}

InteractionsService.cls

public static void assignOwnership(List<Interaction__c> records) {
   Service.assignOwnership(records);
}

private static IInteractionsService Service() {
    (IInteractionsService) Application.Service.newInstance(IInteractionsService.class);
}

InteractionsServiceImpl.cls

public void assignOwnership(List<Interaction__c> records) {
   assignOwnership(Interactions.newInstance(records));
}
public void assignOwnership(Interactions domain){
    // Collect channels
    IChannels channels = Channels.newInstance(domain.getChannelIds());

   // Create a map of key = Channel__c.Id to Channel.OwnerId
   Map<Id, Id> ownerIdByChannelId = channels.getOwnerIdById();

   domain.setOwnerIdByInteractionChannelId(ownerIdByChannelId);
} 

Interaction.cls

public static IInteractions newInstance(List<Interaction__c> records)
{
  return (IInteractions) Application.Domain.newInstance(records, Schema.Interaction__c.SObjectType);
}

public IInteractions setOwnerIdByInteractionChannelId(Map<Id, Id> ownerIdById) {
  for (Interaction__c record : (List<Interaction__c>) getRecords())
  {
    if (ownerIdById.containsKey(record.Interaction_Channel__c) == false) {
      continue;
    }
    record.ownerId = ownerIdById.get(record.Interaction_Channel__c);
  }
  return this;
}

And you could add a method overload to the domain method for the unitOfWork:

public IInteractions setOwnerIdByInteractionChannelId(fflib_ISObjectUnitOfWork uow, Map<Id, Id> ownerIdById) {
  setOwnerIdByInteractionChannelId(ownerIdById);
  uow.registerDirty(getRecords(), new List<SObjectField>{ Interaction__c.OwnerId });  // only register the modified field as dirty
}

If you would also use the fflib-apex-extensions package and extend your domain from fflib_SObject2, then you could even change the setOwnerIdByInteractionChannelId into this:

public IInteractions setOwnerIdByInteractionChannelId(Map<Id, Id> ownerIdById) {
  setFieldValue(
      Schema.Interaction__c.Interaction_Channel__c,
      Schema.Interaction__c.OwnerId,
      ownerIdById);   
  return this;
}
parsam97 commented 1 year ago

Hi @wimvelzeboer,

Really appreciate you taking the time to reply and help me out 🙏🙏

Since my posting this question, I got Andrew Fawcett's 4 edition enterprise patterns book, where I learned more about CDCL and SDCL approaches. I've since decided to use the CDCL approach to simplify things for myself as I am just starting out.

The code you kindly provided still leaves me with the following question:

What if I there's a requirement to assign ownership (or something similar of the sort) onAfterInsert? If I understand your suggestions correctly, this would mean the domain class should call the InteractionService to assign ownership onAfterInsert? It seems wrong to me because the InteractionService is just going to call the domain again, which is where we start from. Although yes, the service method would collect the necessary Channel__c data. But are we doing this just so we don't have our domain layer calling the selector layer or is there another reason?

Thanks again.

wimvelzeboer commented 1 year ago

@parsam97 In the scenario of an on after insert, I would do the following:

InteractionsTriggerHandler.cls

public override void onAfterInsert()
{
  fflib_ISObjectUnitOfWork uow = Application.UnitOfWork.newInstance();
  InteractionsService.assignOwnership(uow, getRecords());
  uow.commitWork();
}

InteractionsService.cls

public static void assignOwnership(fflib_ISObjectUnitOfWork uow, List<Interaction__c> records) {
   Service.assignOwnership(uow, records);
}

private static IInteractionsService Service() {
    (IInteractionsService) Application.Service.newInstance(IInteractionsService.class);
}

InteractionsServiceImpl.cls

public void assignOwnership(fflib_ISObjectUnitOfWork uow, List<Interaction__c> records) {
   assignOwnership(uow, Interactions.newInstance(records));
}

public void assignOwnership(fflib_ISObjectUnitOfWork uow, Interactions domain){
    // Collect channels
    IChannels channels = Channels.newInstance(domain.getChannelIds());

   // Create a map of key = Channel__c.Id to Channel.OwnerId
   Map<Id, Id> ownerIdByChannelId = channels.getOwnerIdById();

   domain.setOwnerIdByInteractionChannelId(uow, ownerIdByChannelId);
} 

Interaction.cls

public static IInteractions newInstance(List<Interaction__c> records)
{
  return (IInteractions) Application.Domain.newInstance(records, Schema.Interaction__c.SObjectType);
}

public IInteractions setOwnerIdByInteractionChannelId(Map<Id, Id> ownerIdById) {
  for (Interaction__c record : (List<Interaction__c>) getRecords())
  {
    if (ownerIdById.containsKey(record.Interaction_Channel__c) == false) {
      continue;
    }
    record.ownerId = ownerIdById.get(record.Interaction_Channel__c);
  }
  return this;
}

public IInteractions setOwnerIdByInteractionChannelId(fflib_ISObjectUnitOfWork uow, Map<Id, Id> ownerIdById) {
  setOwnerIdByInteractionChannelId(ownerIdById);
  uow.registerDirty(getRecords(), new List<SObjectField>{ Interaction__c.OwnerId });  // only register the modified field as dirty
}

You would still use the same method to update the field, but you reach that method via slightly different method overloads with the unit-of-work as extra parameter. So, the domain would indeed not call a selector class.

parsam97 commented 1 year ago

Got it, thanks for the help again!

parsam97 commented 1 year ago

public void assignOwnership(fflib_ISObjectUnitOfWork uow, Interactions domain){

@wimvelzeboer You mentioned this, including the domain as an argument to the Impl class. Did you do this so we could later call service method from domain like this: InteractionService.assignOwnership(this) ?

daveespo commented 1 year ago

Closing this as it's not a bug or feature request and has been largely addressed by William (thank you!)