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

Adds handleAndFinally context to fflib_SObjectDomain #491

Open jamessimone opened 1 month ago

jamessimone commented 1 month ago

As a side note - fflib's files contain an enormous quantity of trailing whitespaces, which bloats file sizes unnecessarily. I'd recommend the entire repo be run through prettier at some point, or (failing that) at the very least the existing style of the code can be preserved while trimming the trailing whitespaces


This change is Reviewable

ImJohnMDaniel commented 1 month ago

G'day @jamessimone

Thanks for submitting this. Always appreciated!

The team will review it soon and reach out if there are questions.

Cheers!

John

jamessimone commented 1 month ago

@ImJohnMDaniel any update on this?

ImJohnMDaniel commented 1 month ago

@jamessimone -- re: the trailing whitespaces in the codebase is acknowledged by the team. Thanks for the callout. We will look into it a later time.

ImJohnMDaniel commented 1 month ago

@ImJohnMDaniel any update on this?

@jamessimone -- Some of the review team have been on vacation. We will be reviewing this soon. We appreciate your patience.

jamessimone commented 1 month ago

Thanks, appreciate it!

stohn777 commented 3 weeks ago

Hi @jamessimone,

Thank you for taking the time to contribute.

The other maintainers and I have looked over your proposal and landed on the below follow-up.

Are you aware of the DoWork feature on the SObject Unit of Work (SOUOW), which provides for the injection of logic to be completed upon a successful DML operation? The SOUOW, IDoWork interface specifies the structure of the injectable logic which is specified via the SOUOW.registerWork(...) method and consumed within the SOUOW.commitWork() logic. An example of its usage is illustrated in the fflib_SObjectUnitOfWorkTest.testDerivedUnitOfWork_CommitDoWorkFail() method, which is a negative test ensuring the DoWork functionality is working properly.

If you are familiar with this feature, can you help us to better understand your use case(s) that necessitates the handleAndFinally() feature, which the DoWork feature is incapable of performing?

Also on a minor note .............. We like the clean-up that you've proposed with moving the SObjectDomain's test functionality to the test class. Much appreciated! May we recommend that be done in a distinct PR, since such lesser significant modifications mask the true intent of the proposed work.

Thanks, @stohn777

jamessimone commented 3 weeks ago

@stohn777 This contribution came about as part of a suggestion made within Apex Rollup, and seems the most appropriate place to add the requested functionality. UOW cannot self-encapsulate all DML performed on platform, and seems an incomplete replacement for a consolidated approach within the actual triggers in question.

As far as refactoring goes, I'd go so far as to say that taking code only runnable via a test out of production-level code is more significant than adding a no-op method to an existing class. While I can understand the reticence of the existing maintainers in undertaking a "big bang" approach to repository cleanup, the implication (that cleanup is of lesser concern) will continually push the actual undertaking of anything like a cleanup further down the road - to everyone's detriment, in my opinion.

stohn777 commented 40 minutes ago

Hi @jamessimone,

My apology not responding lately, due to an illness and work matters.

Following-up a bit with your project's discussion and given the existing state of this project, I arrived at the most simple implementation of Apex-Rollup that I could think of [see below]. Am I missing anything? If not, then the addition of the handleAndFinally() seems like a convenience method. Unless something significant is overlooked, the maintainers agree that the convenience method doesn't address a gap in functionality and contributes minimally to the project and are inclined to forego.

The maintainers would prefer a more precise alignment with the native events provided by Salesforce's trigger feature. Additionally since this is a post-any-trigger-event logic, the door opens to a myriad of similar event-aggregations methods like, handleFollowingAnyBefore(), handlePrecedingAnyUpdate().

Please let me know if a significant gap is overlooked in the Commons triggerhandler framework that necessitates the inclusion of the handleAndFinally() method.

public with sharing class DEV_OpportunitiesTriggerHandler
    extends fflib_SObjectDomain
{
    public DEV_OpportunitiesTriggerHandler(List<Opportunity> sObjectList)
    {
        // Domain classes are initialised with lists to enforce bulkification throughout
        super(sObjectList);
    }
    public class Constructor implements fflib_SObjectDomain.IConstructable
    {
        public fflib_SObjectDomain construct(List<SObject> sObjectList)
        {
            return new DEV_OpportunitiesTriggerHandler(sObjectList);
        }
    }
    public override void onAfterInsert()
    {
        // Rollup.runFromTrigger();
    }
    public override void onAfterUpdate(Map<Id,SObject> existingRecords)
    {
        // Rollup.runFromTrigger();
    }
    public override void onBeforeDelete()
    {
        // Rollup.runFromTrigger();
    }
}