Salesforce.com Trigger: Copy Contact Lead Source to Opportunity

I'm stuck on the syntax (and maybe the logic) of writing this Salesforce.com Trigger. I want the trigger to check to see if a primary contact is listed in the ContactRoles on the Opportunity. If there's a primary listed, I need to lookup the LeadSource from the corresponding contact and insert that value in the the Lead Source of the Opportunity.

Any hints or tips are greatly appreciated!

trigger UpdateContactLeadSource on Opportunity (after insert, after update) {

//Declare the Lead Source Variable which will hold the contact's lead source
string leadsource;

// See if there is a primary contact listed on the Opportunity
for (Opportunity o : Trigger.new) {
OpportunityContactRole[] contactRoleArray =
   [select ContactID, isPrimary from OpportunityContactRole where OpportunityId = :o.id ORDER BY isPrimary DESC, createdDate];

    // If the there is a primary contact, then...
    if (contactRoleArray.size() > 0) {

    // Lookup ContactID on the Contacts table to find the lead source
    for (Contact contact : [SELECT LeadSource FROM Contact WHERE Contact.Id = :OpportunityContactRole.ContactId LIMIT 1]) 

    // Store the actual lead source in the leadsource variable
    { Contact.LeadSource = leadsource;}
    update Opportunity.LeadSource = leadsource; }
}
}

Solution

There are couple of seriously bad things in your code. Hope you won't feel offended, it's a nice requirement and a learning opportunity too...

after insert doesn't make any sense here. By very definition if you've just finished inserting this Opportunity it won't have any contact roles in it yet.*
after update is OK-ish. before update would be nicer because you'd just fill in the value and you'll get the save to database for free.
You might have to replicate this logic to similar trigger on OpportunityContactRole I think? Instead of duplicating code - use some class that could be called from both places?
I'm assuming you'd want to fill it in only if LeadSource on Opportunity would be null?
You have selects in a loop, that's a very bad practice ;) Your trigger isn't "bulk", it might fail when somebody updates 10 opportunities in one transaction (for example by using Data Loader) because we have a limit of max 20 queries in triggers.
You don't need ORDER BY, LIMIT 1 etc - Salesforce will protect you and allow only 1 contact to be primary. Even if you'd want to load them with such mistake with Data Loader.
It seems like you'd benefit from reading about how relationships work in Salesforce. The "dot notation" and subqueries look a bit weird compared to normal SQL languages but you can use them to greatly simplify your code. Will help if you did some object oriented programming in the past. Here's some guide and here are official docs

TL;DR

trigger fillLeadSource on Opportunity (before update) {
    /*  I'm assuming you want to fill LeadSource in only if it was blank. 
        If that's not the case - just delete this first part of code and in the query instead of ":oppsToFill" bind to ":trigger.new" or
        ":trigger.newMap.keyset()".
        (I know they look weird but you can do it, bind objects/collections in queries that expect Ids / collections of Ids)
    */
    Set<Id> oppsToFill = new Set<Id>();
    for(Opportunity o : trigger.new){
        if(o.LeadSource == null) {
            oppsToFill.add(o.Id);
        }
    }

    // Now we'll select all possible contacts wasting only 1 query.
    if(!oppsToFill.isEmpty()){
        List<OpportunityContactRole> roles = [SELECT OpportunityId, Contact.Name, Contact.LeadSource
            FROM OpportunityContactRole
            WHERE isPrimary = true AND Contact.LeadSource != null AND OpportunityId IN :oppsToFill];

        if(!roles.isEmpty()){
            for(OpportunityContactRole ocr : roles){
                Opportunity oppToBeFilled = trigger.newMap.get(ocr.OpportunityId);
                System.debug('Changing lead source on ' + oppToBeFilled.Name + ' from ' + oppToBeFilled.LeadSource + ' to ' + ocr.Contact.LeadSource + ' (thx to ' + ocr.Contact.Name + ' being the Primary Contact).');
                oppToBeFilled.LeadSource = ocr.Contact.LeadSource;
            }
        }
    }
    // That's it. If there was a primary contact with Lead source, data will be copied over.
    // If there was no primary contact or he didn't have the source filled in - tough luck, we did our best.
    // Since it's before update, you get save to database for free.
}

Fine print to #1, for advanced scenarios ;) You could have Contact Roles right after saving if you have another after insert trigger that'd be adding them somewhere... But that'd be very messy (depending on which trigger fired first I'd say you'd get a really interesting random behaviour, good luck debugging). If you do have more than one trigger on given object with same firing keyword in it it's advised to have only 1 trigger and be in full control of the order of actions. See http://www.embracingthecloud.com/2010/07/08/ASimpleTriggerTemplateForSalesforce.aspx for more.

EDIT to answer question from comment re #3

You'd need similar but not identical code in a new trigger (in this case it doesn't matter much whether it's before or after - we need to explicitly update Opportunities). It's a bit worse here also because the fields you want to look at aren't directly available - you have access to OpportunityId, ContactId but not Contact.LeadSource. Something like this should do the trick:

trigger ContactRoleRollup on OpportunityContactRole(after insert, after update){
    Map<Id,Id> oppToContactMap = new Map<Id, Id>();
    for(OpportunityContactRole ocr : trigger.new){
        if(ocr.isPrimary){
            oppToContactMap.put(ocr.OpportunityId, ocr.ContactId);
        }
    }
    if(!oppToContactMap.isEmpty()){
        List<Opportunity> oppsToUpdate = [SELECT Id FROM Opportunity WHERE LeadSource = null AND Id IN :oppToContactMap.keyset()];
        Map<Id, Contact> contacts = [SELECT Id, LeadSource FROM Contact WHERE LeadSource != null AND Id IN :oppToContactMap.values()];

        for(Opportunity o : oppsToUpdate){
            Id contactId = oppToContactMap.get(o.Id);
            Contact c = contacts.get(contactId);
            if(c != null){
                o.LeadSource = c.LeadSource;
            }
        }
        update oppsToUpdate;
    }
}

It gets interesting here because this update will fire our old trigger on opportunities. It should be fine if you have left my "skip if leadSource is filled in" but still you might want to explore 2 things:

Use some helper static flag in a class, call it "skipTriggerOnOpps" you'd be setting this flag in trigger on OpportunityContactRoles and wrapping up whole code of Opportunity trigger in it so it won't execute if we already took care of lead source sync.

Theoretically you could just "touch" the Opportunities without changing anything (treat the old trigger as a benefit, not unwanted side effect in this case). For me it'd look a bit too magical but if it's well commented what's going on here it might lead to less code, less logic duplication, less unit tests... It will work as long as it's after trigger so the query for contact roles we've just modified will see new values. It'd have to look like that

trigger ContactRoleRollup on OpportunityContactRole(after insert, after update){
    Set<Id> oppIds = new Set<Id>();
    for(OpportunityContactRole ocr : trigger.new){
        if(ocr.isPrimary){
            oppIds.add(ocr.OpportunityId);
        }
    }
    if(!oppIds.isEmpty()){
        update [SELECT Id FROM Opportunity WHERE LeadSource = null AND Id IN :oppIds];
        // That's it. Call update directly on returned list without changing anything, let the other trigger worry about the logic
    }
}