delete_attachment Loads Full CRM Note Documents in Loop

[mrrobot47]

Metadata

• File(s): next_crm/api/activities.py:681-701
• Category: Database/API
• Severity: Medium
• Effort to Fix: Low
• Estimated Performance Gain: 10-20%

Problem Description

The delete_attachment() function loads full CRM Note documents using frappe.get_doc() in a loop when checking for attachment references. This is inefficient when a lead/opportunity has many notes.

Impact Analysis

• CPU Impact: Medium - Full document deserialization per note
• Memory Impact: Medium - Full documents loaded into memory
• Database Impact: Medium - N+1 queries for document loading
• User Experience Impact: Low - Only affects delete operations

Code Location

# next_crm/api/activities.py:681-701
def delete_attachment(filename, doctype=None, docname=None):
    deleted = False

    if doctype and docname:
        notes = frappe.get_all(
            "CRM Note",
            filters={"parenttype": doctype, "parent": docname},
            fields=["name"],
        )

        for note in notes:
            note_doc = frappe.get_doc("CRM Note", note.name)  # N+1 pattern
            original_count = len(note_doc.custom_note_attachments)

            updated_attachments = [
                row
                for row in note_doc.custom_note_attachments
                if row.filename != filename
            ]

            if len(updated_attachments) != original_count:
                note_doc.set("custom_note_attachments", updated_attachments)
                note_doc.save()
                deleted = True

Root Cause

1. frappe.get_doc() in a loop instead of targeted query
2. Full document loaded when only checking/modifying child table
3. No early exit when attachment is found and deleted

Proposed Solution

Use direct database operations to find and remove attachment references:

@frappe.whitelist()
def delete_attachment(filename, doctype=None, docname=None):
    """
    Delete a file attachment by its File.name. If doctype & docname are provided,
    also remove its reference from CRM Notes' custom_note_attachments child table.
    """
    deleted = False

    if doctype and docname:
        # First, check if any CRM Note references this attachment
        # Use direct query instead of loading full documents
        attachment_refs = frappe.db.sql("""
            SELECT nca.name, nca.parent
            FROM `tabNCRM Attachments` nca
            INNER JOIN `tabCRM Note` cn ON cn.name = nca.parent
            WHERE cn.parenttype = %s
            AND cn.parent = %s
            AND nca.filename = %s
        """, (doctype, docname, filename), as_dict=True)

        if attachment_refs:
            # Delete the attachment references directly
            for ref in attachment_refs:
                frappe.db.delete("NCRM Attachments", {"name": ref["name"]})
            deleted = True

    # Delete the file
    try:
        frappe.delete_doc("File", filename)
        deleted = True
    except frappe.DoesNotExistError:
        frappe.throw(_("File with ID '{0}' not found.").format(filename))
    except frappe.LinkExistsError:
        frappe.throw(
            _("Cannot delete file because it's still linked with another document.")
        )
    except Exception as e:
        frappe.log_error(f"Failed to delete file: {e}", "Delete Attachment Error")
        frappe.throw(_("An unexpected error occurred while deleting the file."))

    if deleted:
        return {"message": _("File deleted successfully.")}
    else:
        frappe.throw(
            _("File was not deleted. Possibly already removed or not linked correctly.")
        )

Implementation Steps

1. Replace the loop with a SQL JOIN query to find attachment references
2. Delete child table rows directly instead of loading parent documents
3. Reduce from O(n) document loads to 2 database operations
4. Test with notes having attachments

Related Issues

• Similar pattern exists in other delete operations