diff --git a/skills/trigger-refactor-pipeline/SKILL.md b/skills/trigger-refactor-pipeline/SKILL.md deleted file mode 100644 index dbd6185..0000000 --- a/skills/trigger-refactor-pipeline/SKILL.md +++ /dev/null @@ -1,191 +0,0 @@ ---- -name: trigger-refactor-pipeline -description: "Refactor Salesforce triggers into handler patterns with automated test generation and deployment. Use when modernizing legacy triggers with DML/SOQL in loops or inconsistent patterns." -license: Apache-2.0 -compatibility: Requires Salesforce CLI, Python 3.9+ -metadata: - author: afv-library - version: "1.0" -allowed-tools: Bash Read Write ---- - -## When to Use This Skill - -Use this skill when you need to: -- Modernize legacy triggers with DML/SOQL operations inside loops -- Refactor triggers that lack clear separation of concerns -- Implement bulk-safe patterns in existing trigger code -- Generate comprehensive test coverage for refactored triggers - -## Prerequisites - -Before starting, ensure you have: -1. Salesforce CLI installed and authenticated to your target org -2. Python 3.9 or higher installed -3. The baseline trigger deployed (see Setup section) - -## Setup - -Deploy the baseline anti-pattern trigger to analyze and refactor: - -```apex -// ❌ Anti-pattern: all logic stuffed into the trigger, with DML/SOQL in loops. -trigger OpportunityTrigger on Opportunity (before insert, before update, after update) { - // BEFORE INSERT: validate Closed Won w/ low Amount - if (Trigger.isBefore && Trigger.isInsert) { - for (Opportunity o : Trigger.new) { - if (o.StageName == 'Closed Won' && (o.Amount == null || o.Amount < 1000)) { - o.addError('Closed Won opportunities must have Amount ≥ 1000.'); - } - } - } - - // BEFORE UPDATE: if Stage changed, overwrite Description - if (Trigger.isBefore && Trigger.isUpdate) { - for (Opportunity o : Trigger.new) { - Opportunity oldO = Trigger.oldMap.get(o.Id); - if (o.StageName != oldO.StageName) { - o.Description = 'Stage changed from ' + oldO.StageName + ' to ' + o.StageName; - } - } - } - - // AFTER UPDATE: when Stage becomes Closed Won, create a follow-up Task - if (Trigger.isAfter && Trigger.isUpdate) { - for (Opportunity o : Trigger.new) { - Opportunity oldO = Trigger.oldMap.get(o.Id); - if (o.StageName == 'Closed Won' && oldO.StageName != 'Closed Won') { - Task t = new Task( - WhatId = o.Id, - OwnerId = o.OwnerId, - Subject = 'Send thank-you', - Status = 'Not Started', - Priority = 'Normal', - ActivityDate = Date.today() - ); - insert t; // ❌ DML in a loop - } - } - } -} -``` - -Deploy this to your org: -```bash -sf project deploy start --source-dir force-app/main/default/triggers -``` - -## Step 1: Analyze the Trigger - -Run the analysis script to identify anti-patterns and generate a report: - -```bash -python scripts/analyze_trigger.py OpportunityTrigger -``` - -The script will output: -- **DML in loops** - Line numbers where DML operations occur inside iteration -- **SOQL in loops** - Line numbers where SOQL queries occur inside iteration -- **Missing bulkification** - Areas where collection-based processing is needed -- **Complexity score** - Overall trigger complexity rating (1-10) -- **Recommended approach** - Suggested handler pattern based on trigger contexts - -Review the analysis report before proceeding to refactoring. - -## Step 2: Review Handler Patterns - -Consult the [handler patterns reference](references/handler_patterns.md) to understand: -- **Single-responsibility handlers** - One handler class per trigger context -- **Unified handler approach** - Single handler with context methods -- **Bulk collection strategies** - How to aggregate DML/SOQL outside loops -- **Best practices** - Error handling, test boundaries, deployment order - -Choose the pattern that best fits your trigger's complexity and team conventions. - -## Step 3: Refactor the Trigger - -Create the handler class using the appropriate pattern from the reference guide: - -1. **Extract logic** into handler methods with descriptive names -2. **Implement bulk-safe collections** for DML operations -3. **Add proper error handling** using try-catch or Database methods -4. **Update the trigger** to delegate only, passing Trigger context variables -5. **Preserve behavior** - ensure the refactored code produces identical results - -The trigger should be reduced to simple delegation: - -```apex -trigger OpportunityTrigger on Opportunity (before insert, before update, after update) { - OpportunityTriggerHandler handler = new OpportunityTriggerHandler(); - - if (Trigger.isBefore && Trigger.isInsert) { - handler.beforeInsert(Trigger.new); - } - - if (Trigger.isBefore && Trigger.isUpdate) { - handler.beforeUpdate(Trigger.new, Trigger.oldMap); - } - - if (Trigger.isAfter && Trigger.isUpdate) { - handler.afterUpdate(Trigger.new, Trigger.oldMap); - } -} -``` - -## Step 4: Generate Tests - -Use the test template from `assets/test_template.apex` to scaffold your test class: - -1. **Copy the template** and rename for your handler -2. **Implement setup methods** to create test data -3. **Write unit tests** covering each handler method: - - Positive cases with valid data - - Negative cases with invalid data - - Boundary conditions -4. **Add bulk tests** with 200+ records to verify bulkification -5. **Test mixed scenarios** where only some records qualify for logic - -Required test coverage: -- Each handler method must have at least 2 test methods (positive + negative) -- At least one bulk test with 200+ records -- Overall code coverage must be 100% - -## Step 5: Deploy and Validate - -Deploy the refactored trigger, handler, and tests: - -```bash -# Deploy all components -sf project deploy start --source-dir force-app/main/default - -# Run tests -sf apex test run --class-names OpportunityTriggerHandlerTest --result-format human --code-coverage - -# Verify no regressions -sf apex test run --test-level RunLocalTests --result-format human -``` - -Validation checklist: -- [ ] All new tests pass with 100% coverage -- [ ] No new governor limit warnings in debug logs -- [ ] Existing functionality remains unchanged -- [ ] Deployment to production planned with rollback strategy - -## Troubleshooting - -**Issue**: Tests fail with "System.LimitException: Too many DML statements" -- **Solution**: Ensure handler methods collect DML operations and execute outside loops - -**Issue**: Code coverage below 100% -- **Solution**: Add negative test cases and verify all conditional branches are tested - -**Issue**: Behavior differs from original trigger -- **Solution**: Review Trigger context variables (new, old, oldMap) are passed correctly to handler - -## Next Steps - -After successful refactoring: -1. Document the new handler pattern in your team's wiki -2. Update code review checklist to enforce handler patterns for new triggers -3. Identify other legacy triggers for refactoring using this skill -4. Consider implementing a trigger framework if managing many triggers diff --git a/skills/trigger-refactor-pipeline/assets/test_template.apex b/skills/trigger-refactor-pipeline/assets/test_template.apex deleted file mode 100644 index fbed093..0000000 --- a/skills/trigger-refactor-pipeline/assets/test_template.apex +++ /dev/null @@ -1,321 +0,0 @@ -/** - * Test class template for trigger handlers - * - * INSTRUCTIONS: - * 1. Replace [ObjectName] with your SObject (e.g., Opportunity) - * 2. Replace [HandlerClass] with your handler class name - * 3. Implement the setupTestData() method with your test records - * 4. Add specific test methods for each handler method - * 5. Ensure 100% code coverage - */ -@IsTest -private class [ObjectName]TriggerHandlerTest { - - /** - * Setup test data that all test methods can use - */ - @TestSetup - static void setupTestData() { - // TODO: Create test records here - // Example: - // List testOpps = new List(); - // for (Integer i = 0; i < 10; i++) { - // testOpps.add(new Opportunity( - // Name = 'Test Opp ' + i, - // StageName = 'Prospecting', - // CloseDate = Date.today().addDays(30), - // Amount = 5000 - // )); - // } - // insert testOpps; - } - - /** - * Test beforeInsert handler - Positive case - */ - @IsTest - static void testBeforeInsert_Positive() { - Test.startTest(); - - // TODO: Create valid test records - // List testRecords = new List{ - // new Opportunity( - // Name = 'Valid Opp', - // StageName = 'Prospecting', - // CloseDate = Date.today().addDays(30), - // Amount = 10000 - // ) - // }; - - // Insert should succeed - // insert testRecords; - - Test.stopTest(); - - // TODO: Add assertions - // List inserted = [SELECT Id, Name FROM Opportunity WHERE Name = 'Valid Opp']; - // System.Assert.areEqual(1, inserted.size(), 'Should insert 1 record'); - } - - /** - * Test beforeInsert handler - Negative case - */ - @IsTest - static void testBeforeInsert_Negative() { - Test.startTest(); - - Boolean exceptionThrown = false; - - try { - // TODO: Create invalid test records that should fail validation - // List testRecords = new List{ - // new Opportunity( - // Name = 'Invalid Opp', - // StageName = 'Closed Won', - // CloseDate = Date.today(), - // Amount = 500 // Below minimum - // ) - // }; - - // insert testRecords; - - } catch (DmlException e) { - exceptionThrown = true; - // TODO: Assert error message - // System.Assert.isTrue(e.getMessage().contains('must have Amount'), - // 'Should throw validation error'); - } - - Test.stopTest(); - - // System.Assert.isTrue(exceptionThrown, 'Should have thrown an exception'); - } - - /** - * Test beforeUpdate handler - Positive case - */ - @IsTest - static void testBeforeUpdate_Positive() { - // TODO: Query existing test data from @TestSetup - // List testOpps = [SELECT Id, StageName, Description FROM Opportunity LIMIT 1]; - - Test.startTest(); - - // TODO: Update records to trigger handler logic - // testOpps[0].StageName = 'Qualification'; - // update testOpps; - - Test.stopTest(); - - // TODO: Add assertions - // Opportunity updated = [SELECT Description FROM Opportunity WHERE Id = :testOpps[0].Id]; - // System.Assert.isTrue(updated.Description.contains('Stage changed'), - // 'Description should be updated'); - } - - /** - * Test beforeUpdate handler - Negative case - */ - @IsTest - static void testBeforeUpdate_Negative() { - // TODO: Implement negative test for beforeUpdate - Test.startTest(); - - // TODO: Attempt update that should fail - - Test.stopTest(); - - // TODO: Assert failure occurred - } - - /** - * Test afterInsert handler - Positive case - */ - @IsTest - static void testAfterInsert_Positive() { - Test.startTest(); - - // TODO: Create and insert records - - Test.stopTest(); - - // TODO: Query for related records created by handler - // List createdTasks = [SELECT Id FROM Task]; - // System.Assert.areEqual(1, createdTasks.size(), 'Should create 1 task'); - } - - /** - * Test afterUpdate handler - Positive case - */ - @IsTest - static void testAfterUpdate_Positive() { - // TODO: Query existing test data - // List testOpps = [SELECT Id, StageName FROM Opportunity LIMIT 1]; - - Test.startTest(); - - // TODO: Update to trigger after-update logic - // testOpps[0].StageName = 'Closed Won'; - // update testOpps; - - Test.stopTest(); - - // TODO: Query for side effects (e.g., Tasks created) - // List tasks = [SELECT Id, Subject FROM Task WHERE WhatId = :testOpps[0].Id]; - // System.Assert.areEqual(1, tasks.size(), 'Should create 1 task'); - // System.Assert.areEqual('Send thank-you', tasks[0].Subject); - } - - /** - * Test bulk operations - 200+ records - * Critical for validating bulkification - */ - @IsTest - static void testBulkInsert() { - Test.startTest(); - - // TODO: Create 200+ records - // List bulkOpps = new List(); - // for (Integer i = 0; i < 200; i++) { - // bulkOpps.add(new Opportunity( - // Name = 'Bulk Opp ' + i, - // StageName = 'Prospecting', - // CloseDate = Date.today().addDays(30), - // Amount = 10000 - // )); - // } - - // insert bulkOpps; - - Test.stopTest(); - - // TODO: Assert all records inserted successfully - // List inserted = [SELECT Id FROM Opportunity WHERE Name LIKE 'Bulk Opp%']; - // System.Assert.areEqual(200, inserted.size(), 'Should insert all 200 records'); - } - - /** - * Test bulk update with mixed scenarios - * Some records qualify for logic, others don't - */ - @IsTest - static void testBulkUpdate_Mixed() { - // Create test data - List testOpps = new List(); - for (Integer i = 0; i < 50; i++) { - testOpps.add(new Opportunity( - Name = 'Bulk Update Opp ' + i, - StageName = 'Prospecting', - CloseDate = Date.today().addDays(30), - Amount = 10000 - )); - } - insert testOpps; - - Test.startTest(); - - // Update half to trigger logic, half to not trigger - for (Integer i = 0; i < testOpps.size(); i++) { - if (Math.mod(i, 2) == 0) { - // TODO: Set condition that triggers handler logic - // testOpps[i].StageName = 'Closed Won'; - } else { - // TODO: Set condition that doesn't trigger handler logic - // testOpps[i].Amount = 15000; - } - } - - // update testOpps; - - Test.stopTest(); - - // TODO: Assert only qualifying records triggered side effects - // List tasks = [SELECT Id FROM Task WHERE WhatId IN :testOpps]; - // System.Assert.areEqual(25, tasks.size(), 'Should create tasks for 25 qualifying records'); - } - - /** - * Test governor limits are not exceeded - */ - @IsTest - static void testGovernorLimits() { - Test.startTest(); - - // TODO: Create maximum allowed records - // List maxOpps = new List(); - // for (Integer i = 0; i < 200; i++) { - // maxOpps.add(new Opportunity( - // Name = 'Limit Test ' + i, - // StageName = 'Closed Won', - // CloseDate = Date.today(), - // Amount = 10000 - // )); - // } - - // insert maxOpps; - - Test.stopTest(); - - // Assert we're under governor limits - System.Assert.isTrue(Limits.getDmlStatements() < Limits.getLimitDmlStatements(), - 'Should not exceed DML statement limit'); - System.Assert.isTrue(Limits.getQueries() < Limits.getLimitQueries(), - 'Should not exceed SOQL query limit'); - } - - /** - * Test with null or empty collections - * Ensures handler doesn't break with edge cases - */ - @IsTest - static void testWithEmptyCollection() { - Test.startTest(); - - // TODO: Call handler methods with empty lists - // OpportunityTriggerHandler handler = new OpportunityTriggerHandler(); - // handler.beforeInsert(new List()); - - Test.stopTest(); - - // If we get here without exception, test passes - System.Assert.isTrue(true, 'Handler should handle empty collections gracefully'); - } - - /** - * Helper method to create test data inline - * Use when @TestSetup is not sufficient - */ - private static List createTestOpportunities(Integer count) { - List testOpps = new List(); - - for (Integer i = 0; i < count; i++) { - testOpps.add(new Opportunity( - Name = 'Test Opp ' + i, - StageName = 'Prospecting', - CloseDate = Date.today().addDays(30), - Amount = 10000 - )); - } - - return testOpps; - } - - /** - * Helper method to assert task creation - */ - private static void assertTasksCreated(List oppIds, Integer expectedCount, String subject) { - List tasks = [ - SELECT Id, Subject, WhatId - FROM Task - WHERE WhatId IN :oppIds - ]; - - System.Assert.areEqual(expectedCount, tasks.size(), - 'Should create ' + expectedCount + ' task(s)'); - - if (expectedCount > 0) { - System.Assert.areEqual(subject, tasks[0].Subject, - 'Task subject should match'); - } - } -} diff --git a/skills/trigger-refactor-pipeline/references/handler_patterns.md b/skills/trigger-refactor-pipeline/references/handler_patterns.md deleted file mode 100644 index 12762dd..0000000 --- a/skills/trigger-refactor-pipeline/references/handler_patterns.md +++ /dev/null @@ -1,442 +0,0 @@ -# Trigger Handler Patterns Reference - -This guide covers common patterns for refactoring Salesforce triggers into handler classes with bulk-safe operations. - -## Pattern 1: Simple Handler Class - -**Best for**: Triggers with 1-3 contexts and straightforward logic. - -### Structure - -```apex -public class OpportunityTriggerHandler { - - public void beforeInsert(List newRecords) { - validateClosedWonAmount(newRecords); - } - - public void beforeUpdate(List newRecords, Map oldMap) { - updateDescriptionOnStageChange(newRecords, oldMap); - } - - public void afterUpdate(List newRecords, Map oldMap) { - createTasksForClosedWon(newRecords, oldMap); - } - - // Private helper methods below - private void validateClosedWonAmount(List opportunities) { - for (Opportunity opp : opportunities) { - if (opp.StageName == 'Closed Won' && - (opp.Amount == null || opp.Amount < 1000)) { - opp.addError('Closed Won opportunities must have Amount ≥ 1000.'); - } - } - } - - private void updateDescriptionOnStageChange( - List newRecords, - Map oldMap - ) { - for (Opportunity opp : newRecords) { - Opportunity oldOpp = oldMap.get(opp.Id); - if (opp.StageName != oldOpp.StageName) { - opp.Description = 'Stage changed from ' + - oldOpp.StageName + ' to ' + opp.StageName; - } - } - } - - private void createTasksForClosedWon( - List newRecords, - Map oldMap - ) { - List tasksToInsert = new List(); - - for (Opportunity opp : newRecords) { - Opportunity oldOpp = oldMap.get(opp.Id); - - // Check if stage changed to Closed Won - if (opp.StageName == 'Closed Won' && - oldOpp.StageName != 'Closed Won') { - - tasksToInsert.add(new Task( - WhatId = opp.Id, - OwnerId = opp.OwnerId, - Subject = 'Send thank-you', - Status = 'Not Started', - Priority = 'Normal', - ActivityDate = Date.today() - )); - } - } - - // Bulk DML outside loop - if (!tasksToInsert.isEmpty()) { - insert tasksToInsert; - } - } -} -``` - -### Trigger Delegation - -```apex -trigger OpportunityTrigger on Opportunity ( - before insert, before update, after update -) { - OpportunityTriggerHandler handler = new OpportunityTriggerHandler(); - - if (Trigger.isBefore) { - if (Trigger.isInsert) { - handler.beforeInsert(Trigger.new); - } else if (Trigger.isUpdate) { - handler.beforeUpdate(Trigger.new, Trigger.oldMap); - } - } - - if (Trigger.isAfter && Trigger.isUpdate) { - handler.afterUpdate(Trigger.new, Trigger.oldMap); - } -} -``` - -## Pattern 2: Handler with Database Methods - -**Best for**: When you need granular error handling and partial success. - -### Key Features - -- Uses `Database.insert()` instead of `insert` for partial saves -- Returns `Database.SaveResult` for error handling -- Logs errors without stopping execution - -### Example - -```apex -private void createTasksForClosedWon( - List newRecords, - Map oldMap -) { - List tasksToInsert = new List(); - - for (Opportunity opp : newRecords) { - Opportunity oldOpp = oldMap.get(opp.Id); - - if (opp.StageName == 'Closed Won' && - oldOpp.StageName != 'Closed Won') { - - tasksToInsert.add(new Task( - WhatId = opp.Id, - OwnerId = opp.OwnerId, - Subject = 'Send thank-you', - Status = 'Not Started', - Priority = 'Normal', - ActivityDate = Date.today() - )); - } - } - - if (!tasksToInsert.isEmpty()) { - Database.SaveResult[] results = Database.insert(tasksToInsert, false); - - // Log errors without stopping execution - for (Integer i = 0; i < results.size(); i++) { - if (!results[i].isSuccess()) { - System.debug('Failed to create task: ' + results[i].getErrors()); - } - } - } -} -``` - -## Pattern 3: Handler with Maps for Lookups - -**Best for**: When you need to query related records for processing. - -### Key Features - -- Pre-queries related records using Sets -- Uses Maps for O(1) lookups instead of nested loops -- Avoids SOQL in loops - -### Example - -```apex -private void enrichOpportunitiesWithAccountData(List opportunities) { - // Collect Account IDs - Set accountIds = new Set(); - for (Opportunity opp : opportunities) { - if (opp.AccountId != null) { - accountIds.add(opp.AccountId); - } - } - - // Single SOQL query outside loop - Map accountMap = new Map([ - SELECT Id, Name, Industry, AnnualRevenue - FROM Account - WHERE Id IN :accountIds - ]); - - // Use Map for O(1) lookup - for (Opportunity opp : opportunities) { - if (opp.AccountId != null && accountMap.containsKey(opp.AccountId)) { - Account acc = accountMap.get(opp.AccountId); - // Process with account data - opp.Description = 'Account Industry: ' + acc.Industry; - } - } -} -``` - -## Pattern 4: Unified Handler Framework - -**Best for**: Complex triggers with many contexts and cross-cutting concerns. - -### Structure - -```apex -public abstract class TriggerHandler { - - protected Boolean isBefore; - protected Boolean isAfter; - protected Boolean isInsert; - protected Boolean isUpdate; - protected Boolean isDelete; - protected Boolean isUndelete; - - public void run() { - isBefore = Trigger.isBefore; - isAfter = Trigger.isAfter; - isInsert = Trigger.isInsert; - isUpdate = Trigger.isUpdate; - isDelete = Trigger.isDelete; - isUndelete = Trigger.isUndelete; - - if (isBefore) { - if (isInsert) beforeInsert(); - if (isUpdate) beforeUpdate(); - if (isDelete) beforeDelete(); - } - - if (isAfter) { - if (isInsert) afterInsert(); - if (isUpdate) afterUpdate(); - if (isDelete) afterDelete(); - if (isUndelete) afterUndelete(); - } - } - - protected virtual void beforeInsert() {} - protected virtual void beforeUpdate() {} - protected virtual void beforeDelete() {} - protected virtual void afterInsert() {} - protected virtual void afterUpdate() {} - protected virtual void afterDelete() {} - protected virtual void afterUndelete() {} -} -``` - -### Concrete Handler - -```apex -public class OpportunityTriggerHandler extends TriggerHandler { - - private List newRecords; - private List oldRecords; - private Map newMap; - private Map oldMap; - - public OpportunityTriggerHandler() { - this.newRecords = (List) Trigger.new; - this.oldRecords = (List) Trigger.old; - this.newMap = (Map) Trigger.newMap; - this.oldMap = (Map) Trigger.oldMap; - } - - protected override void beforeInsert() { - validateClosedWonAmount(); - } - - protected override void beforeUpdate() { - updateDescriptionOnStageChange(); - } - - protected override void afterUpdate() { - createTasksForClosedWon(); - } - - // Private helper methods omitted for brevity -} -``` - -### Trigger Delegation - -```apex -trigger OpportunityTrigger on Opportunity ( - before insert, before update, after update -) { - new OpportunityTriggerHandler().run(); -} -``` - -## Best Practices - -### 1. Bulkification - -Always process records in collections: - -```apex -// ✓ Good: Collect DML outside loop -List tasksToInsert = new List(); -for (Opportunity opp : opportunities) { - tasksToInsert.add(new Task(...)); -} -if (!tasksToInsert.isEmpty()) { - insert tasksToInsert; -} - -// ✗ Bad: DML inside loop -for (Opportunity opp : opportunities) { - insert new Task(...); // SOQL/DML in loop! -} -``` - -### 2. Defensive Null Checks - -```apex -// ✓ Good: Check for null before accessing -if (opp.AccountId != null && accountMap.containsKey(opp.AccountId)) { - Account acc = accountMap.get(opp.AccountId); - // Safe to use acc -} - -// ✗ Bad: Assumes data exists -Account acc = accountMap.get(opp.AccountId); -String industry = acc.Industry; // NullPointerException risk -``` - -### 3. Clear Method Names - -```apex -// ✓ Good: Descriptive, verb-noun pattern -private void validateClosedWonAmount(List opportunities) -private void createTasksForClosedWon(List opportunities) - -// ✗ Bad: Vague or unclear -private void validate(List opportunities) -private void doStuff(List opportunities) -``` - -### 4. Single Responsibility - -Each handler method should do one thing: - -```apex -// ✓ Good: Separate concerns -private void validateClosedWonAmount(List opportunities) -private void validateRequiredFields(List opportunities) -private void calculateDiscounts(List opportunities) - -// ✗ Bad: One method does everything -private void processOpportunities(List opportunities) -``` - -### 5. Test Boundaries - -Structure code to make testing easier: - -```apex -// ✓ Good: Public method for testing, private for implementation -@TestVisible -private void createTasksForClosedWon( - List newRecords, - Map oldMap -) { - // Implementation -} -``` - -## Deployment Order - -When deploying refactored triggers: - -1. Deploy handler class(es) first -2. Update trigger to use handler -3. Deploy test class -4. Run all tests before production deployment -5. Monitor debug logs for 24-48 hours after production deployment - -## Rollback Strategy - -Keep the old trigger code commented out or in version control: - -```apex -trigger OpportunityTrigger on Opportunity (...) { - // New handler approach - new OpportunityTriggerHandler().run(); - - /* OLD CODE - REMOVE AFTER 1 WEEK IF NO ISSUES - if (Trigger.isBefore && Trigger.isInsert) { - for (Opportunity o : Trigger.new) { - // old logic - } - } - */ -} -``` - -## Common Pitfalls - -### Pitfall 1: Recursive Triggers - -**Problem**: Handler calls DML which triggers the same trigger again. - -**Solution**: Use static flag to prevent recursion: - -```apex -public class OpportunityTriggerHandler { - private static Boolean isExecuting = false; - - public void beforeUpdate(List newRecords, Map oldMap) { - if (isExecuting) return; - - isExecuting = true; - try { - // Your logic here - } finally { - isExecuting = false; - } - } -} -``` - -### Pitfall 2: Mixed Context Logic - -**Problem**: Before-context logic mixed with after-context logic. - -**Solution**: Keep context methods separate and focused: - -```apex -// ✓ Good: Separate methods per context -public void beforeUpdate(List newRecords, Map oldMap) -public void afterUpdate(List newRecords, Map oldMap) - -// ✗ Bad: Mixed logic in one method -public void handleUpdate(List newRecords, Map oldMap) -``` - -### Pitfall 3: Over-Engineering - -**Problem**: Using complex framework for simple triggers. - -**Solution**: Choose the right pattern for your complexity: -- 1-3 contexts with simple logic → Simple Handler (Pattern 1) -- 3-5 contexts with moderate complexity → Handler with Database Methods (Pattern 2) -- 5+ contexts with cross-cutting concerns → Unified Framework (Pattern 4) - -## Additional Resources - -- [Apex Developer Guide: Trigger and Bulk Request Best Practices](https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_triggers_bulk_requests.htm) -- [Apex Enterprise Patterns](https://github.com/apex-enterprise-patterns) -- [Trigger Framework Comparison](https://github.com/kevinohara80/sfdc-trigger-framework) diff --git a/skills/trigger-refactor-pipeline/scripts/analyze_trigger.py b/skills/trigger-refactor-pipeline/scripts/analyze_trigger.py deleted file mode 100644 index f3aef3f..0000000 --- a/skills/trigger-refactor-pipeline/scripts/analyze_trigger.py +++ /dev/null @@ -1,258 +0,0 @@ -#!/usr/bin/env python3 -""" -Analyze Salesforce Apex triggers for common anti-patterns. - -Usage: - python analyze_trigger.py - -Requirements: - - Salesforce CLI authenticated to target org - - Python 3.9+ -""" - -import sys -import subprocess -import json -import re -from typing import List, Dict, Tuple - -class TriggerAnalyzer: - def __init__(self, trigger_name: str): - self.trigger_name = trigger_name - self.trigger_body = "" - self.issues = { - 'dml_in_loops': [], - 'soql_in_loops': [], - 'missing_bulk': [], - } - self.complexity_score = 0 - - def retrieve_trigger(self) -> bool: - """Retrieve trigger source code from Salesforce org.""" - try: - cmd = [ - 'sf', 'apex', 'get', 'trigger', - '--trigger-name', self.trigger_name, - '--json' - ] - result = subprocess.run(cmd, capture_output=True, text=True) - - if result.returncode != 0: - print(f"Error retrieving trigger: {result.stderr}") - return False - - data = json.loads(result.stdout) - self.trigger_body = data.get('result', {}).get('body', '') - return True - - except Exception as e: - print(f"Failed to retrieve trigger: {e}") - return False - - def analyze_dml_in_loops(self) -> None: - """Detect DML operations inside for loops.""" - lines = self.trigger_body.split('\n') - in_loop = False - loop_start = 0 - - dml_patterns = [ - r'\binsert\s+', r'\bupdate\s+', r'\bdelete\s+', - r'\bundelete\s+', r'\bupsert\s+' - ] - - for i, line in enumerate(lines, 1): - # Track loop entry - if re.search(r'\bfor\s*\(', line): - in_loop = True - loop_start = i - - # Track loop exit - if in_loop and line.strip() == '}': - # Check if this closes the loop (simplified heuristic) - in_loop = False - - # Check for DML in loop - if in_loop: - for pattern in dml_patterns: - if re.search(pattern, line): - self.issues['dml_in_loops'].append({ - 'line': i, - 'code': line.strip(), - 'loop_start': loop_start - }) - - def analyze_soql_in_loops(self) -> None: - """Detect SOQL queries inside for loops.""" - lines = self.trigger_body.split('\n') - in_loop = False - loop_start = 0 - - soql_pattern = r'\[SELECT\s+' - - for i, line in enumerate(lines, 1): - if re.search(r'\bfor\s*\(', line): - in_loop = True - loop_start = i - - if in_loop and line.strip() == '}': - in_loop = False - - if in_loop and re.search(soql_pattern, line, re.IGNORECASE): - self.issues['soql_in_loops'].append({ - 'line': i, - 'code': line.strip(), - 'loop_start': loop_start - }) - - def analyze_bulkification(self) -> None: - """Check for proper bulkification patterns.""" - # Simple heuristic: if we have DML in loops, bulkification is missing - if self.issues['dml_in_loops']: - self.issues['missing_bulk'].append({ - 'message': 'DML operations should be collected and executed outside loops', - 'affected_lines': [issue['line'] for issue in self.issues['dml_in_loops']] - }) - - if self.issues['soql_in_loops']: - self.issues['missing_bulk'].append({ - 'message': 'SOQL queries should be moved outside loops or use Maps for lookups', - 'affected_lines': [issue['line'] for issue in self.issues['soql_in_loops']] - }) - - def calculate_complexity(self) -> None: - """Calculate overall complexity score (1-10).""" - score = 1 - - # Add points for each issue type - score += len(self.issues['dml_in_loops']) * 2 - score += len(self.issues['soql_in_loops']) * 2 - score += len(self.issues['missing_bulk']) * 1 - - # Count trigger contexts - contexts = len(re.findall(r'Trigger\.(isBefore|isAfter)', self.trigger_body)) - score += contexts - - # Count lines of code (normalized) - loc = len([l for l in self.trigger_body.split('\n') if l.strip()]) - score += min(loc // 10, 3) - - self.complexity_score = min(score, 10) - - def recommend_approach(self) -> str: - """Recommend refactoring approach based on analysis.""" - if self.complexity_score <= 3: - return "Simple handler class with separate methods for each trigger context" - elif self.complexity_score <= 6: - return "Handler class with bulkified collections and helper methods" - else: - return "Unified handler framework with separate concern classes (validation, DML, etc.)" - - def generate_report(self) -> None: - """Print analysis report.""" - print("\n" + "="*70) - print(f"TRIGGER ANALYSIS REPORT: {self.trigger_name}") - print("="*70 + "\n") - - print(f"Complexity Score: {self.complexity_score}/10") - print(f"Recommended Approach: {self.recommend_approach()}\n") - - # DML in loops - if self.issues['dml_in_loops']: - print("⚠️ DML OPERATIONS IN LOOPS:") - for issue in self.issues['dml_in_loops']: - print(f" Line {issue['line']}: {issue['code']}") - print(f" └─ Loop started at line {issue['loop_start']}") - print() - else: - print("✓ No DML operations found in loops\n") - - # SOQL in loops - if self.issues['soql_in_loops']: - print("⚠️ SOQL QUERIES IN LOOPS:") - for issue in self.issues['soql_in_loops']: - print(f" Line {issue['line']}: {issue['code']}") - print(f" └─ Loop started at line {issue['loop_start']}") - print() - else: - print("✓ No SOQL queries found in loops\n") - - # Bulkification - if self.issues['missing_bulk']: - print("⚠️ BULKIFICATION RECOMMENDATIONS:") - for issue in self.issues['missing_bulk']: - print(f" • {issue['message']}") - print(f" Affected lines: {', '.join(map(str, issue['affected_lines']))}") - print() - else: - print("✓ Bulkification patterns look good\n") - - print("="*70) - print("NEXT STEPS:") - print("1. Review the handler patterns reference guide") - print("2. Create handler class with bulk-safe collections") - print("3. Extract trigger logic into handler methods") - print("4. Generate comprehensive tests using the template") - print("="*70 + "\n") - - -def main(): - if len(sys.argv) < 2: - print("Usage: python analyze_trigger.py ") - sys.exit(1) - - trigger_name = sys.argv[1] - - print(f"Analyzing trigger: {trigger_name}...") - - analyzer = TriggerAnalyzer(trigger_name) - - # For demo purposes, use inline example if retrieval fails - if not analyzer.retrieve_trigger(): - print("⚠️ Could not retrieve from org. Using example trigger for demonstration.\n") - # Use the example from the SKILL.md - analyzer.trigger_body = """trigger OpportunityTrigger on Opportunity (before insert, before update, after update) { - if (Trigger.isBefore && Trigger.isInsert) { - for (Opportunity o : Trigger.new) { - if (o.StageName == 'Closed Won' && (o.Amount == null || o.Amount < 1000)) { - o.addError('Closed Won opportunities must have Amount ≥ 1000.'); - } - } - } - if (Trigger.isBefore && Trigger.isUpdate) { - for (Opportunity o : Trigger.new) { - Opportunity oldO = Trigger.oldMap.get(o.Id); - if (o.StageName != oldO.StageName) { - o.Description = 'Stage changed from ' + oldO.StageName + ' to ' + o.StageName; - } - } - } - if (Trigger.isAfter && Trigger.isUpdate) { - for (Opportunity o : Trigger.new) { - Opportunity oldO = Trigger.oldMap.get(o.Id); - if (o.StageName == 'Closed Won' && oldO.StageName != 'Closed Won') { - Task t = new Task( - WhatId = o.Id, - OwnerId = o.OwnerId, - Subject = 'Send thank-you', - Status = 'Not Started', - Priority = 'Normal', - ActivityDate = Date.today() - ); - insert t; - } - } - } -}""" - - # Run analysis - analyzer.analyze_dml_in_loops() - analyzer.analyze_soql_in_loops() - analyzer.analyze_bulkification() - analyzer.calculate_complexity() - - # Generate report - analyzer.generate_report() - - -if __name__ == '__main__': - main()