Adds handleAndFinally context to fflib_SObjectDomain

[jamessimone]

• Adds a method, handleAndFinally for consumers of fflib_SObjectDomain to opt into as something called at the end of each trigger run. This enables more idiomatic behavior, like a single place for committing logs (and, for my purposes, for rollups to be fired a la Rollup.runFromTrigger() in FFLIB Trigger Handler jamessimone/apex-rollup#605)
• Does the minimum amount of cleanup on fflib_SObjectDomain by moving classes used only in tests into the corresponding test class
• Updates the System namespace shadowing variable name fflib_SObjectDomain.Test to fflib_SObjectDomain.TestFactory to both properly represent the type it's referring to and eliminate the (probably unintentional) namespace shadowing

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 

[ImJohnMDaniel]

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]

@ImJohnMDaniel any update on this?

[ImJohnMDaniel]

@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]

@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]

Thanks, appreciate it!

[stohn777]

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]

@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]

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();
	}
}

[jamessimone]

@stohn777 it's certainly a convenience method - but what isn't? I respect your decision, though, and I'll close this as a result. I do hope that at some point in the future you all will consider some of my other suggestions, such as:

• removing test-only code from your production classes
• formatting the repository(s) to strip trailing whitespace (among other formatting oddities)
• updating variable names to no longer shadow the standard library's classes (like the TestFactory being referred to as Test)

[stohn777]

@jamessimone, those updates are eminently welcome. If you're willing to refactor your PR to remove the addition of the handleAndFinally(), we can certainly continue with those well-needed changes.