Eliminate N+1 query pattern in attachment deletion operations

[Copilot]

The delete_attachment() and delete_attachments_from_crm_notes() functions were loading full CRM Note documents in a loop to check/remove attachment references, causing N+1 queries when leads/opportunities have many notes.

Changes

• Replaced document loading loops with SQL JOIN queries: Single query retrieves all attachment references instead of O(n) frappe.get_doc() calls
• Direct child table deletion: Batch delete operations on tabNCRM Attachments instead of loading parent documents and calling save()
• Reduced database operations: From 2n+1 queries (n get_all + n get_doc + 1 delete) to 2 queries (1 SQL query + 1 batch delete)

Before

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 queries
    # Filter and update attachments
    note_doc.save()  # N more queries

After

attachment_refs = frappe.db.sql("""
    SELECT nca.name 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)

frappe.db.delete("NCRM Attachments", {"name": ("in", attachment_ids)})  # 1 batch operation

Expected performance gain: 10-20% on attachment delete operations with proportionally higher gains as note count increases.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>delete_attachment Loads Full CRM Note Documents in Loop</issue_title>
<issue_description>

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</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes delete_attachment Loads Full CRM Note Documents in Loop #333

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.