I think that the code you have right now is actually pretty close. I'm assuming that Commission_Amount__c is just the commission for one Contact, not the sum of all commissions for all Contacts under a given Account.
The main thing I see is that this is a trigger that I would put on Contact rather than Account. The reason being that the thing you're interested in is Commission_Amount__c on the Contact.
The only times that a trigger on the parent object is fired when a child record changes are:
- When you have a rollup summary field on the parent (which isn't possible between Account and Contact)
- When you have code that explicitly performs DML on the parent
The query that you're using is pretty much spot on. Query the Accounts and have a parent-child subquery to isolate the largest value of Commission_Amount__c for each account by using ORDER BY and LIMIT 1.
To handle the second part, "uncheck the checkbox from the other contacts", you need to use a second query to find the Contact record(s) currently checked (so you can uncheck them).
So all said and done, your trigger would look like
// Trigger on Contact instead of Account
// Unfortunately, this isn't likely to be a situation where we can take advantage
// of the benefits of a "before" trigger
trigger CheckPrimary on Contact (after update) {
// The generally accepted approach for getting parent record data is
// 1) Aggregate Ids
// 2) Perform your query
// 3) Process and update records
// One of the things you'll need to worry about is making sure you don't have
// duplicate records that you're trying to update in a single DML.
// You could work around this by using two separate Lists, and two separate
// DML updates, but that's a bit of a waste.
// Queries are our most precious resource, so if there's a way to reduce
// the number of queries we need to run it's something worth considering.
// Performing extra DML can only cause the number of queries used to increase
// (or stay the same), so let's try to do this with a single DML update.
Map<Id, Contact> contactsToUpdateMap = new Map<Id, Contact>();
// Aggregate Ids
Set<Id> accountIdSet = new Set<Id>();
for(Contact c :trigger.new){
accountIdSet.add(c.AccountId);
}
// Perform a query
for(Contact c :[SELECT Id FROM Contact WHERE AccountId IN :accountIdSet AND Primary__c = true]){
// Process records
// There is a chance that the Primary Contact isn't changing
// In that scenario, if we unset Primary__c first, then we can
// overwrite the record contained in the map to change Primary__c back
// to true.
// If we did this the other way around, there would be a chance that no
// contact would be marked as Primary.
c.Primary__c = false;
contactsToUpdateMap.put(c.Id, c);
}
// Perform another query
for(Account a: [SELECT Id, (SELECT Id, Name, Commission_Amount__c FROM Contacts
WHERE Commission_Amount__c != null ORDER BY Commission_Amount__c DESC
LIMIT 1) FROM Account WHERE Id IN :accountIdSet]){
// Process more records
// To access child records from a parent-child subquery, you use the
// "relationship name" (Contacts, in this case)
// Child records are stored in an embedded List<SObject>
// Normally, you'd want to take the safe approach and use a nested loop to
// access the child records, but since each Account will have a single
// child returned it is safe to access it directly
contactsToUpdateMap.put(a.Contacts[0].Id, a.Contacts[0]);
// Non-primitive types (things that aren't Integer, Boolean, String, etc...)
// are stored in collections (maps, lists, sets) as references.
// If you change the record outside of the map, it'll also change the reference
// stored in the map
a.Contacts[0].Primary__c = true;
}
// Since we're in an After Update trigger, we need to perform DML to persist
// our changes.
// Even if this were a "Before Update" trigger, we can only avoid performing
// DML for the records in trigger.new
// We're probably going to work on records outside of those contained
// in trigger.new, so DML would be required anyway.
update contactsToUpdateMap.values();
}
That's close, but it would get you into an infinite loop. The DML update at the end of the trigger would cause the Contact trigger to run again, which would perform another DML, and so on.
To avoid that, you'd need a recursion guard. The simplest recursion guard is just a static Boolean variable in an Apex class. Note that it does need to be an Apex class, because static variables declared in a trigger end up not being very static at all.
So the final result (with fewer comments) would look like
public class ContactRecursionGuard{
public static Boolean isRunning = false;
}
trigger CheckPrimary on Contact (after update) {
// Check the recursion guard
// Do an early "return" if we're already running so we don't loop
// infinitely
if(ContactRecursionGuard.isRunning){ return; }
Map<Id, Contact> contactsToUpdateMap = new Map<Id, Contact>();
Set<Id> accountIdSet = new Set<Id>();
for(Contact c :trigger.new){
accountIdSet.add(c.AccountId);
}
for(Contact c :[SELECT Id FROM Contact WHERE AccountId IN :accountIdSet AND Primary__c = true]){
c.Primary__c = false;
contactsToUpdateMap.put(c.Id, c);
}
for(Account a: [SELECT Id, (SELECT Id, Name, Commission_Amount__c FROM Contacts
WHERE Commission_Amount__c != null ORDER BY Commission_Amount__c DESC
LIMIT 1) FROM Account WHERE Id IN :accountIdSet]){
contactsToUpdateMap.put(a.Contacts[0].Id, a.Contacts[0]);
a.Contacts[0].Primary__c = true;
}
// This is the problematic statement, the one we have to be careful with
// Set the recursion guard just before, and then un-set it immediately after
ContactRecursionGuard.isRunning = true;
update contactsToUpdateMap.values();
ContactRecursionGuard.isRunning = false;
}
Another option, which would make your trigger a little shorter, remove a query, and get rid of the need to use a recursion guard would be to make Primary__c on Contact a formula field (of type checkbox).
If you store the current maximum contact commission on the Account (which is what you'd do with the trigger on Contact), you can reference that in a formula on Contact. You can traverse upwards in object hierarchies in the same way you can with a query. This is what's called a "cross-object formula".
Commission_Amount_c = Account.Max_Commission_Amount__c
That would take care of automatically un-checking the previous "primary" Contacts. The trigger would change slightly (you'd be updating Accounts instead of Contacts), but it would still look familiar.
trigger CheckPrimary on Contact (after update) {
Map<Id, Account> accountsToUpdateMap = new Map<Id, Account>();
Set<Id> accountIdSet = new Set<Id>();
for(Contact c :trigger.new){
accountIdSet.add(c.AccountId);
}
for(Account a: [SELECT Id, (SELECT Id, Name, Commission_Amount__c FROM Contacts
WHERE Commission_Amount__c != null ORDER BY Commission_Amount__c DESC
LIMIT 1) FROM Account WHERE Id IN :accountIdSet]){
a.Max_Commission_Amount__c = a.Contacts[0].Commission_Amount__c;
accountsToUpdateMap.put(a.Id, a);
}
update accountsToUpdateMap.values();
}
