Attachment trigger work not persisting

Question

i am trying to append the case id to attachment using trigger i have written correct logic but not getting any error and not appending case id to the attachment

here is my code

Trigger:

trigger AttachmentTrigger on Attachment (after insert,after update) {

AttachmentTriggerHandler handler = new AttachmentTriggerHandler();

if(Trigger.isAfter && (Trigger.isInsert || Trigger.isUpdate)){
    handler.appendingCaseNumberToAttachment(trigger.new);

}

}

Handler :

/*public class AttachmentTriggerHandler {

public void  appendingCaseNumberToAttachment(list<Attachment> attaches){

    list<Attachment> listOfAttachments = new list<Attachment>();
    set<Id> parentIds = new set<Id>();
    for(Attachment newAttachments:attaches){
        system.debug('attaches====>'+newAttachments);
        if(((String)newAttachments.ParentID).startswith('500')){
            parentIds.add(newAttachments.ParentID);
            System.debug('parentIds===='+parentIds);

        }
        }
        Map<id,Case> casemap=new map<Id,Case>([select id,CaseNumber,(SELECT id,Name from Attachments) from case where id IN:parentIds]);
        for(Case cc:casemap.values()){
        for(Attachment attach:cc.Attachments){
            System.debug('Attachments========'+attach);
            attach.name=cc.CaseNumber+'-'+attach.Name;
            listOfAttachments.add(attach);
            //Attachments.Name = Attachments.Name+'-'+Attachments.(case.id).ParentId;

        }
       } 

}

}*/

public class AttachmentTriggerHandler {

public void  appendingCaseNumberToAttachment(list<Attachment> attaches){

    list<Attachment> listOfAttachments = new list<Attachment>();
    set<Id> parentIds = new set<Id>();
    for(Attachment newAttachments:attaches){
        system.debug('attaches====>'+newAttachments);
        if(((String)newAttachments.ParentID).startswith('500')){
            parentIds.add(newAttachments.ParentID);
            System.debug('parentIds===='+parentIds);

        }
    }


    Map<id,Case> casemap=new map<Id,Case>([select id,CaseNumber,(SELECT id,Name from Attachments) from case where id IN:parentIds]);
    for(Case cc:casemap.values()){
        for(Attachment attach:cc.Attachments){
            System.debug('Attachments========'+attach);
            attach.name=cc.CaseNumber+'-'+attach.Name;
            listOfAttachments.add(attach);
            //Attachments.Name = Attachments.Name+'-'+Attachments.(case.id).ParentId;

        }
    }

   update listOfAttachments;
}

}

Please suggest me where i am doing mistake

Answer 1

There are several things wrong with your code. The first one is a big no-no. You have a SELECT statement inside of your for loop. This will potentially hit governor limits if your trigger fires with a sufficiently large number of records.

The second is that you never persist your changes to the database.

This is a potential correct code for you.

public class AttachmentTriggerHandler {

    public void  appendingCaseNumberToAttachment(list<Attachment> attaches){

        list<Attachment> listOfAttachments = new list<Attachment>();
        set<Id> parentIds = new set<Id>();
        for(Attachment newAttachments:attaches){
            system.debug('attaches====>'+newAttachments);
            if(((String)newAttachments.ParentID).startswith('500')){
                parentIds.add(newAttachments.ParentID);
                System.debug('parentIds===='+parentIds);

            }
        }


        Map<id,Case> casemap=new map<Id,Case>([select id,CaseNumber,(SELECT id,Name from Attachments) from case where id IN:parentIds]);
        for(Case cc:casemap.values()){
            for(Attachment attach:cc.Attachments){
                System.debug('Attachments========'+attach);
                attach.name=cc.CaseNumber+'-'+attach.Name;
                listOfAttachments.add(attach);
                //Attachments.Name = Attachments.Name+'-'+Attachments.(case.id).ParentId;

            }
        }

       update listOfAttachments;
    }
}

The code above assumes you're running in an after context. If you are in a before context, you wouldn't need to use listOfAttachments and you could update the attaches list directly (with no need for DML) to see the attachment names updated.

PS: As Phil pointed out, if this is an Attachment trigger, you may run into exceptions while trying to update the records already in the trigger. But without seeing your code and getting more info about your logic I am hesitant to update the code any further.