Skip to content

Update sample to latest recommendations: optional Application/DI, VF→LWC, and Agentforce actions - #61

Open
afawcett wants to merge 12 commits into
masterfrom
refresh-sample
Open

afawcett wants to merge 12 commits into
masterfrom
refresh-sample

Conversation

@afawcett

@afawcett afawcett commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Updates the sample to reflect current recommendations—making the Application class and interfaces optional unless using DI—along with modernized UI (VF→LWC) and Agentforce actions.

  • Remove Application.cls and service interfaces in favour of concrete services with newInstance() factories and test mocks — see Apex Enterprise Patterns: Recent Updates and Thoughts on the Application Class for background on this change
  • Replace Visualforce custom buttons with Lightning Web Components and quick actions
  • Add invocable actions and an AI authoring bundle for Agentforce
  • Move user mode Unit of Work wiring from Application.cls to UnitOfWork.cls (user mode enforcement unchanged from master)
  • Add polymorphic invoicing via InvoiceTargets__mdt
  • Update README and scratch org configuration for API 67+

This change is Reviewable

CI deploy fails when __tests__ bundles reference @lwc/engine-dom, which is not valid org metadata.

@daveespo daveespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daveespo reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: 2 of 186 files reviewed, 5 unresolved discussions (waiting on afawcett, ImJohnMDaniel, and john-storey-devops).


README.md line 33 at r1 (raw file):

------------------

This sample uses **concrete** Domain, Selector, and Service classes with static `newInstance()` factories and `@TestVisible` mock hooks for unit tests. There is no `Application.cls` dependency-injection factory — see [Apex Enterprise Patterns: Recent Updates and Thoughts on the Application Class](https://andyinthecloud.com/2026/04/13/apex-enterprise-patterns-recent-updates-and-thoughts-on-the-application-class/) for background on this approach.

Why keep the Interfaces for service/domain/selector in the project if you're taking the concrete implementation approach?


sfdx-source/apex-common-samplecode/main/classes/UnitOfWork.cls line 39 at r1 (raw file):

	 * @return Unit of Work instance.
	 */
	public static fflib_ISObjectUnitOfWork newInstance() {

Hmm. Seeing this in real life doesn't look like what I expected ... notably, you are hard-coding the list of SObjects that are part of UOW which doesn't lend itself well to being able to reuse this UOW across functional domains. Selectors and Domains are tied to their underlying SObjects but the UOW is used across the Services and by hardcoding this list here, it seems like we fell back toward the 'Application is the God class' problem .. we just moved it into UOW

I guess I wonder why we don't take the list of SObjects as the argument to newInstance()


sfdx-source/apex-common-samplecode/main/classes/actions/ApplyDiscount.cls line 45 at r1 (raw file):

		List<Result> results = new List<Result>();
		for (Request request : requests) {
			OpportunitiesService.newInstance().applyDiscounts(

Is this DML in a loop?


sfdx-source/apex-common-samplecode/main/classes/controllers/OpportunityCreateInvoicesController.cls line 36 at r1 (raw file):

	 * (for example 10 for 10%).
	 */
	public Decimal DiscountPercentage { get; set; }

lower case D


sfdx-source/apex-common-samplecode/main/classes/controllers/OpportunityApplyDiscountsController.cls line 35 at r1 (raw file):

	 * Discount percent entered on the list page (for example 10 for 10%).
	 */
	public Decimal DiscountPercentage { get; set; }

lowercase D

Aligns the sample factory with the concrete-class approach discussed in PR review.
@afawcett

afawcett commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor Author

@daveespo Good catch — I meant the sample app’s own domain/selector/service interfaces, which this PR removes. The remaining interfaces are from fflib-apex-common (fflib_ISObjectUnitOfWork, etc.).

We can’t remove those without breaking the library contract, and they’re still useful where you want to mock at the framework boundary. The sample’s shift is to drop the extra sample-layer interfaces and Application.cls DI — concrete services with newInstance() + @TestVisible mocks instead.

On UnitOfWork.newInstance() returning fflib_ISObjectUnitOfWork vs fflib_SObjectUnitOfWork: agreed — for consistency with the concrete approach, UnitOfWork.newInstance() will return fflib_SObjectUnitOfWork (updated in latest commit).

@afawcett

afawcett commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor Author

@daveespo I see the concern about the hardcoded SObject list feeling like a smaller Application class — but a per-service newInstance(List<SObjectType>) creates a different problem: you lose the ability to pass a single Unit of Work between services in one transaction (e.g. discounting and invoicing in the same commit).

The central registry does mean a missing type surfaces as a compile-time dependency that ripples through the codebase. I think that’s less common as the app evolves than the old Application.cls failure mode — different call sites quietly constructing incompatible UOW instances and only failing at runtime when a register/commit crosses service boundaries.

So the trade-off here is one app-level registry (now in UnitOfWork.cls) to support shared transactions, with the central list as the single place to register types. I've added a note on the class documenting the per-service alternative.

@afawcett

afawcett commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor Author

@daveespo Good catch — the earlier version did call the service inside the loop.

It now builds a Map<Id, Decimal> from the requests, makes one applyDiscounts(...) call (one query, one commit), then maps results back for Flow. The loops are adapter-only; no DML in the loop.

@afawcett

afawcett commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor Author

@daveespo Addressed — renamed DiscountPercentage to discountPercentage on both Visualforce controllers (OpportunityApplyDiscountsController, OpportunityCreateInvoicesController). The VF pages were already binding to {!discountPercentage}.

@john-storey-devops

john-storey-devops commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

While considering current SOLID practices regarding this PR, two points have come to light: interfaces belong where variation genuinely exists, and constructor injection is the mechanism that actually satisfies dependency inversion.

My remaining reservation is how dependencies get substituted in tests. @TestVisible private static mock keeps the lookup inside the class, so a caller can't see or control what the service depends on. Constructor injection should expose those dependencies in the signature, while newInstance() is the default composition and the entry point — same testability, no ambient statics.

This also bears on the UnitOfWork.cls discussion above. Each service currently calls UnitOfWork.newInstance() internally and commits its own, so the shared-transaction case the class doc describes is met by combining operations in one method rather than by passing an instance. Constructor injection would let the entry point build one Unit of Work and hand it to both services — which is the capability the central registry is there to support.

Consider the following as an example of Apex following modern SOLID patterns:

// Supporting multiple processors 
// like Visa, American Express and Discover
// just as examples.
public interface ICardProcessor
{
    ProcessorResult charge(Payment__c payment);
}


public inherited sharing class PaymentService
{
    private final ICardProcessor processor;
    private final PaymentsSelector selector;
    private final fflib_ISObjectUnitOfWork uow;

    // Entry point for unit tests
    public PaymentService(ICardProcessor processor,
                          PaymentsSelector selector,
                          fflib_ISObjectUnitOfWork uow)
    {
        this.processor = processor;
        this.selector  = selector;
        this.uow       = uow;
    }

    // Entry point for business logic.
    public static PaymentService newInstance(ICardProcessor processor)
    {
        return new PaymentService(processor,
                                  PaymentsSelector.newInstance(),
                                  UnitOfWork.newInstance());
    }

    public void capture(Set<Id> paymentIds)
    {
        List<Payment__c> payments = selector.selectById(paymentIds);
        for (Payment__c payment : payments)
        {
            ProcessorResult result = processor.charge(payment);
            payment.Status__c = result.isApproved ? 'Captured' : 'Declined';
            uow.registerDirty(payment);
        }
        uow.commitWork();
    }
}


@IsTest
private class PaymentServiceTest
{
    @IsTest
    private static void captureMarksApprovedPaymentAsCaptured()
    {
        // Given
        ...

        // When
        new PaymentService(processorMock, selectorMock, uowMock).capture(paymentIds);

        // Then
        ...
    }
}

@john-storey-devops john-storey-devops left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ImJohnMDaniel ImJohnMDaniel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming that if you wanted developers to use newInstance() methods in selectors and domains, then you would want the constructors to be private instead of public. Is that what you were thinking?

See README.md Architecture Notes
Make it clearer the connection between the two
'service' to 'opportunitiesSvc' improves readability of the code, e.g. opportunitiesSvc.applyDiscountsAndCreateInvoices
@afawcett

Copy link
Copy Markdown
Contributor Author

@john-storey-devops thanks I made most of your changes here. Services and Controllers now use constructor injection. Domains and UnitOfWOrk still use property injection - as they are stateful (domain within a method, uow's within a service method) and we have no factories for them in this basic sample representation.

@afawcett

Copy link
Copy Markdown
Contributor Author

sfdx-source/apex-common-samplecode/main/classes/controllers/OpportunityApplyDiscountsController.cls line 35 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

lowercase D

Done.

@afawcett

Copy link
Copy Markdown
Contributor Author

sfdx-source/apex-common-samplecode/main/classes/controllers/OpportunityCreateInvoicesController.cls line 36 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

lower case D

Done.

@afawcett

Copy link
Copy Markdown
Contributor Author

sfdx-source/apex-common-samplecode/main/classes/actions/ApplyDiscount.cls line 45 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

Is this DML in a loop?

Done
I bulkified the service method to support this.

@afawcett

Copy link
Copy Markdown
Contributor Author

README.md line 33 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

Why keep the Interfaces for service/domain/selector in the project if you're taking the concrete implementation approach?

Done The sample-layer IOpportunitiesService / IOpportunities / IOpportunitiesSelector types are gone; callers use the concrete classes. What remains are fflib’s interfaces (fflib_ISObjectUnitOfWork, etc.) and ISupportInvoicing, which stays because Opportunity, DeveloperWorkItem, and TrainingWorkItem actually vary.

@afawcett

Copy link
Copy Markdown
Contributor Author

sfdx-source/apex-common-samplecode/main/classes/UnitOfWork.cls line 39 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

Hmm. Seeing this in real life doesn't look like what I expected ... notably, you are hard-coding the list of SObjects that are part of UOW which doesn't lend itself well to being able to reuse this UOW across functional domains. Selectors and Domains are tied to their underlying SObjects but the UOW is used across the Services and by hardcoding this list here, it seems like we fell back toward the 'Application is the God class' problem .. we just moved it into UOW

I guess I wonder why we don't take the list of SObjects as the argument to newInstance()

The hardcoded list is not a smaller Application. Application referenced every service, selector, and domain class, so one compile error cascaded through deploy; this list is only SObjectType tokens, so a broken Apex class does not take UnitOfWork with it. We keep one app-level list so every service method gets a consistent UoW (newInstance() per method, then commit). newInstance(List<SObjectType>) would work, but each call site would have to pass the types and mismatches would only fail at commit. Tests still substitute via UnitOfWork.mock.

@afawcett

Copy link
Copy Markdown
Contributor Author

@ImJohnMDaniel not what we were thinking — we want newInstance() at entry points, but constructors stay visible so Stub API / constructor injection still work (it cannot mock classes that only have private constructors).

public class InvoiceFactory
{
private fflib_ISObjectUnitOfWork unitOfWork;
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Indenting. On this line and numerous following.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-storey-devops will fix - thx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to place this class in the layer taxonomy the sample teaches. Is it a coarse- or fine-grained service? It resolves implementations from custom metadata, which is an Application concern, and it also performs selection, which is a service concern — so it reads as neither, crossing several fflib principles at once.

Why isn't it in one of the architectural directories? It sits in main/classes/ root, outside service/, selectors/ and domains/. Separation of concerns is the first lesson this repo teaches, and a reader navigating by folder finds this class outside the scheme with no stated layer.

In a sample, the layer a class belongs to is the lesson. Could we either place it in a layer, or name it explicitly as composition infrastructure and say which grain of service is meant to consume it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-storey-devops yes I'll clarify its learning purpose better and place it per your suggestion. Its really a little added extract to show how a localised DI factory can be built within a service to make part of that services logic extensible.

Comment thread README.md Outdated
Architecture Notes
------------------

This sample uses **concrete** Domain, Selector, and Service classes. Constructors take collaborators; `newInstance()` is the default composition. Prefer `X.newInstance()` at entry points over `new X()`. Use the constructor to inject mocks (during Apex Tests) or to compose deliberately. Service methods that persist call `UnitOfWork.newInstance()` so each method gets a fresh Unit of Work; tests set `UnitOfWork.mock`. `X.newInstance()` is also the single place a later metadata-driven factory would resolve which service, selector, or domain implementation to construct.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The principle we discussed in Slack is that newInstance() is the canonical composition. It calls the appropriate constructor with the appropriate default configuration. The public constructors are the injection seam. They let tests supply mocks and exercise a class in isolation and let business logic compose something different for special circustances.

In this repo, Services and selectors follow that, while Controllers and invocable actions don't. The code paints an inconsistent message, while the default composition ends up with no static newInstance() method.

Granted, Visualforce list controllers are a fair exception.

This line states the rule for one shape only, so the README and the code disagree on which classes get a factory and which get a constructor pair. The constructor visibility point from @ImJohnMDaniel's question belongs here as well. Constructors stay public so the Stub API can mock them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggled to make newInstance the canonical composition when the platforms runtime entry point is a static method or the default constructor - in other words when the new instance is not a custom Apex call site. @john-storey-devops let me know what you want to see change here- or rather confirm what I think your asking for as being an update to the text here to clarify further why which DI approach (or lack off) is used and the lack of private constructor. If so - I'll craft some updated text and revert back np. :)

daveespo
daveespo previously approved these changes Sep 22, 2026

@daveespo daveespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daveespo partially reviewed 190 files and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on afawcett and ImJohnMDaniel).


sfdx-source/apex-common-samplecode/main/classes/UnitOfWork.cls line 39 at r1 (raw file):

Previously, afawcett (Andrew Fawcett) wrote…

The hardcoded list is not a smaller Application. Application referenced every service, selector, and domain class, so one compile error cascaded through deploy; this list is only SObjectType tokens, so a broken Apex class does not take UnitOfWork with it. We keep one app-level list so every service method gets a consistent UoW (newInstance() per method, then commit). newInstance(List<SObjectType>) would work, but each call site would have to pass the types and mismatches would only fail at commit. Tests still substitute via UnitOfWork.mock.

While I don't agree with the approach, I'm ok approving this. In a project with 60 Custom SObjects plus Standard Objects, this becomes unwieldy to make sure the order is doesn't create problem for objects that have circular references. But this is a sample and is one form of opinion so let's go with it.

@afawcett

afawcett commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

sfdx-source/apex-common-samplecode/main/classes/UnitOfWork.cls line 39 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

While I don't agree with the approach, I'm ok approving this. In a project with 60 Custom SObjects plus Standard Objects, this becomes unwieldy to make sure the order is doesn't create problem for objects that have circular references. But this is a sample and is one form of opinion so let's go with it.

done Gotcha, a single type list does not scale when insert order and circular lookups get messy. The class now documents that, and there is a newInstance(List<SObjectType>) overload for a narrower list. The no-arg method stays as this sample’s default.

@daveespo daveespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daveespo partially reviewed 11 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ImJohnMDaniel and john-storey-devops).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants