4

I have a Schedulable class that does multiple rollups from my Opportunities to their respective Accounts. This seems to work fine until recently I noticed some issues.

Essentially my Schedulable class is laid out like this:

Updated Added Complete Class

global class ScheduledOpportunityToAccountRollup implements Schedulable {

    /* Unique list of accounts to be rolled up */
    Set<String> account_list = new Set<String>();
    /* Unique list of unpaid invoice opportunities to be rolled up */
    Set<AggregateResult> unpaid_invoices = new Set<AggregateResult>();

    global void execute(SchedulableContext sc) {
        scheduleAccountsRevenueRollup();
        scheduleAveragePastDueInvoiceRollup();
    }

    private void scheduleAccountsRevenueRollup(){
        getAccountRevenueList();
        if(!account_list.isEmpty()) OpportunityUtils.rollupAccountRevenues(account_list);
    }

    private void scheduleAveragePastDueInvoiceRollup(){
        getAccountUnpaidInvoiceSet();
        if(!unpaid_invoices.isEmpty()) OpportunityUtils.rollupAverageDaysPastDue(unpaid_invoices);
    }

    /**
     * GET ACCOUNT REVENUE LIST
     * Opportunities must have Labor Services and either be paid or past due in order to be included in this scheduled batch
     */
    private void getAccountRevenueList(){
        List<Account> accounts = new List<Account>([SELECT Id FROM Account WHERE Total_Balance2__c != 0 OR Current_or_Previous_Customer__c != null]);
        for(Account a : accounts){
            account_list.add(a.Id);
        }
    }

    /**
     * GET ACCOUNT UNPAID INVOICE SET
     * Opportunities must have Labor Services past due in order to be included in this scheduled batch
     */
    private void getAccountUnpaidInvoiceSet(){
        unpaid_invoices = new Set<AggregateResult>([
            SELECT AccountId, COUNT(Id) Invoices, AVG(Days_Past_Due__c) Average, MAX(Days_Past_Due__c) Max
            FROM Opportunity
            WHERE IsDeleted = FALSE 
            AND Invoice_Num__c != '' 
            AND Is_Paid__c = FALSE
            GROUP BY AccountId 
        ]);
    }

}

When this runs I have issue with some accounts not being rolled up in the 2nd method scheduleAveragePastDueInvoiceRollup(). So when troubleshooting I removed the first one scheduleAccountsRevenueRollup() and this time everything in scheduleAveragePastDueInvoiceRollup() worked exactly as expected.

SO, question is, am I not allowed to run multiple methods that each have their own transactions? At the moment I'm not seeing script errors from any governing limits, just finding issues on accounts that aren't being rolled up.

  • Could you elaborate around what you mean that each method has its own transaction? I.e. are you using Database.setSavePoint() etc... – Daniel Ballinger May 26 '15 at 22:27
  • I just mean that one Schedulable class is running but multiple operations are happening. The first method does a SOQL and then after some logic runs an UPDATE on accounts. The 2nd SOQL essentially does the same thing afterwards. When I run them together I see no errors but I find accounts untouched that should have been. So I'm trying to figure out what may be limiting what I'm attempting. – Xtremefaith May 26 '15 at 22:35
  • It would appear that although there are no fatal errors, I do have some close to limit notifications in the debug log, so perhaps its not a matter of their being anything wrong with my concept here, its just too exhaustive on the resources in one attempt... – Xtremefaith May 26 '15 at 23:13
  • In theory it should be possible for both methods to make SOQL calls and then perform DML operations. Are you doing any callouts? These would need to occur before any DML operations. Are you explicitly checking any Limits methods and skipping functionality? Maybe the second SOQL query is being affected by the DML operations of the first? Are you using any static members to bypass functionality? – Daniel Ballinger May 27 '15 at 0:16
  • I updated post to show complete class to help answer your questions. Yes I use static members to bypass functionality. Each rollup has its own separate utility method that creates a collection of accounts to be updated with rolled up values from the Opportunities list, then each has its own DML UPDATE Accounts – Xtremefaith May 27 '15 at 0:40
2

A single schedulable function is a single transaction. If you really wanted to isolate the functions from each other, you could use a schedulable batchable class. Here's an implementation:

public class ScheduledOpportunityToAccountRollup implements Schedulable, Database.Batchable<Integer> {
    public void execute(SchedulableContext sc) {
        Database.executeBatch(this, 1);
    }

    public Integer[] start(Database.BatchableContext context) {
        return new Integer[] { 1, 2 };
    }

    public void execute(Database.BatchableContext context, Integer[] scope) {
        if(scope[0] == 1) {
            scheduleAccountsRevenueRollup();
        } else {
            scheduleAveragePastDueInvoiceRollup();
        }
    }

    public void finish(Database.BatchableContext context) {
    }
    // Don't forget to include your other functions here
}

General Optimizations

Don't use class-level variables when they belong in a function. Use return values if you really want to split your function up into smaller parts. For example, you could write:

Set<Id> getAccountRevenueList(){
    return new Map<Id, Account>(
        [SELECT Id FROM Account WHERE Total_Balance2__c != 0 OR Current_or_Previous_Customer__c != null]
    ).keySet();
}

We can use the Map(SObject[]) constructor to automatically get the ID values from the query without a loop or even a temporary variable.

From there, you can simply:

void scheduleAccountsRevenueRollup() {
    OpportunityUtils.rollupAccountRevenues(
        getAccountRevenueList()
    );
}

This method shouldn't be responsible for seeing if the list is empty. Your utility class should be willing to accept an empty collection and return early if it has nothing to do.

Now, we've doubled up on your governor limits (because each phase runs in a separate transaction, and guaranteed to be in order), and we've prevented exceptions from one function harming the other-- this is no longer an all or nothing transaction.

Finally, note that I've dropped the global values to public (because there's no reason for this class to be global), and dropped the "private" keyword on the functions that are used locally, since the language automatically makes unmarked functions and variables private.


As far as why "some things may not be working" correctly, the AggregateResult query only returns rows that match at least one record. If there are zero values, they won't be included, so you necessarily need two parameters to your utility class: the list of all ID values that you expect, and the AggregateResult collection from the query.

  • I've never attempted this, could you explain the scope? Do I need to keep adding to the start() integer if I ever add more methods? When I schedule this I expect it will run execute(), as you have it written, will that only set the scope to 1 so only scheduleAccountsRevenueRollup would run? Then I would just need to add Database.executeBatch(this, 2); to make the other method run? – Xtremefaith May 27 '15 at 18:59
  • 1
    Database.executeBatch accepts two parameters: the class to run, and the number of records to process per batch method. In this design, the second parameter should always be 1. Scope is an array of values for a particular chunk of the batch. In this case, scope[0] is a selector for which function to run. This produces two separate transactions where one can fail independently of the other. The start method returns two integers {1, 2}, such that on the first pass, function 1 will run, and on the second pass, function 2 will run. You can expand this design indefinitely. – sfdcfox May 27 '15 at 19:25
  • Any thoughts on this: System.UnexpectedException: No more than one executeBatch can be called from within a testmethod. I didn't change my testMethod, basically looks like this: String jobId = System.schedule('TEST', CRON_EXP, new ScheduledOpportunityToAccountRollup() ); – Xtremefaith May 27 '15 at 20:59
  • For testing purposes, you'll probably have to rig an alternate start method, because of general governor limits. Something like: @TestVisible Integer[] testStep; public Integer[] start(Database.BatchableContext context) { return testStep == null? new Integer[] { 1, 2 }: testStep; } From there, alter your test method: ScheduledOpportunityToAccountRollup r = new ScheduledOpportunityToAccountRollup(); testStep = new Integer[] { 1 }; System.schedule(r, 1); – sfdcfox May 27 '15 at 21:06
  • Also, by virtue of this rule, you'll need two test methods-- replace the { 1 } with { 2 } for the other method. – sfdcfox May 27 '15 at 21:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.