You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Keep e-mailed invoices in Downloads and delete them with the e-mail - #1010
The order placed, paid and completed e-mails can attach a PDF invoice. By default the invoice was written as a file to wwwroot/assets/files/exportimport. With AttachPdfInvoiceToBinary on it went to Downloads instead, and the order completed e-mail then produced both.
Neither copy was ever removed: not when the e-mail was deleted, not when all e-mails were cleared, and not when a customer deleted their account. An invoice holds customer data, so it should live in the application's file storage and go away with its e-mail.
Solution
Always Downloads. The three handlers use SaveOrderToBinary. PrintOrderToPdf and the AttachPdfInvoiceToBinary setting are removed.
New field QueuedEmail.InvoiceDownloadId. It marks the one download an e-mail owns. AttachedDownloads cannot do this, because it also holds the message template's attachment, which many e-mails share. The send task adds the invoice to the e-mail's attachments.
Deleted with the e-mail.QueuedEmailService deletes the invoice when one e-mail is deleted, when selected e-mails are deleted, in DeleteAllEmails and in DeleteCustomerEmail. The invoice goes first, so a delete that is interrupted can be repeated. Only the e-mails read at the start are deleted with their invoices, so an e-mail queued in the meantime keeps its invoice and stays in the queue. A file that cannot be deleted is logged and does not stop the rest or the account deletion.
Deleted with the customer. A handler on EntityDeleted<Customer> deletes the invoice e-mails of that customer's orders through DeleteOrderInvoiceEmails. It reads only order ids and includes deleted orders. An invoice goes to the order's billing address, which may not be the account's e-mail, and only My account calls DeleteCustomerEmail. The handler also covers an admin, a company owner, the API and OIDC deleting a customer. Other e-mails of a customer deleted that way stay in the queue, as they did before.
Requeue makes a copy through CopyInvoiceDownload, so each e-mail owns its download and no reference counting is needed. The requeued e-mail keeps the order reference, so it is found when the customer is deleted.
No e-mail, no invoice. When no e-mail is queued (inactive template, no recipient, or an error while building or queuing it), OrderMessageProviderService deletes the invoice it was given. A file that cannot be deleted there is logged. InsertQueuedEmail also deletes the invoice of an e-mail it could not store, which covers the copy made by a requeue. One limit: if a plugin's EntityInserted<QueuedEmail> handler throws after the e-mail is stored, the order message path still deletes the invoice, and that e-mail goes out without it. No handler like that exists in this repository.
An invoice does not stop the order. When the invoice cannot be generated, the paid and completed handlers log it and send the e-mail without it, as the placed handler already did. The store owner and vendor messages, loyalty points and gift vouchers still run.
The new queries read only ids. The ones in DeleteAllEmails and DeleteCustomerEmail scan what the delete next to them removes anyway, and DeleteOrderInvoiceEmails runs only when a customer is deleted, so there is no new index. The invoice downloads have no DownloadGuid, so the storefront download action does not serve them.
No new resources, settings or migrations. The unused resource Admin.Settings.Order.AttachPdfInvoiceToBinary is removed from DefaultLanguage.xml.
Breaking changes
For plugins and custom code, all of which need a rebuild:
IPdfService.PrintOrderToPdf is removed, and PdfService no longer takes IWebHostEnvironment.
IOrderMessageProviderService: SendOrderPlacedCustomerMessage, SendOrderPaidCustomerMessage and SendOrderCompletedCustomerMessage take invoiceDownloadId instead of attachmentFilePath, attachmentFileName and attachments.
IMessageProviderService.SendNotification has a new optional parameter at the end, so a custom implementation must add it.
IQueuedEmailService has new members CopyInvoiceDownload and DeleteOrderInvoiceEmails, and IDownloadService has DeleteDownloadById. The QueuedEmailService, OrderMessageProviderService, OrderStatusCommandHandler and ProcessOrderPaidCommandHandler constructors changed.
OrderSettings.AttachPdfInvoiceToBinary and its model property are removed. A stored value is ignored on load and dropped on the next save.
Upgrading in place:
E-mails queued before the upgrade keep working. QueuedEmail.AttachmentFilePath and the file branch of EmailSender stay, so they still read their files.
Files already in wwwroot/assets/files/exportimport are not touched. Remove the order_*.pdf files once those e-mails have been sent.
Invoices stored earlier through AttachedDownloads stay when their e-mail is deleted, because they cannot be told apart from template attachments.
Testing
Unit tests pass: Grand.Business.Messages.Tests (108), Grand.Business.Storage.Tests (81), Grand.Business.Checkout.Tests (499), Grand.Business.Common.Tests (171), Grand.Modules.Tests (346), Grand.Mapping.Tests (237). The full solution builds. The other test projects were not run.
A throwaway program, not part of this PR, ran the real services against LiteDB and MongoDB: queue, send task with a fake sender, requeue copy, and the three kinds of delete, including an e-mail document without the new field. All checks passed on both. A second one ran DeleteOrderInvoiceEmails, DeleteCustomerEmail and DeleteAllEmails against both databases with a fake download service, including an e-mail queued while the invoices were being deleted, which kept its invoice. On MongoDB it also covered a document without the new field, and the event handler itself: invoices of the customer's live and deleted orders went, another customer's stayed. Real SMTP delivery, the event being raised in a running application and the admin screens below were not run.
In Admin > Settings > Sales > Order settings, turn on the PDF invoice for the order placed e-mail. The "Attach PDF invoice to binary" option is gone, also in the store panel.
Place an order and let the queued e-mail send. It arrives with the invoice, and no new file appears in wwwroot/assets/files/exportimport. The QueuedEmail document has InvoiceDownloadId, pointing at a Download document.
Add an attachment to that message template and place another order. The e-mail carries both files, and InvoiceDownloadId holds only the invoice.
In System > Queued emails, open the e-mail and requeue it. The new e-mail has a different InvoiceDownloadId, the same Reference and ObjectId, and sends with the invoice.
Delete the original e-mail. Its Download document and file are gone, while the requeued e-mail's invoice and the template attachment remain.
Use "Delete all". Every invoice download is gone and the template attachment remains.
Sign in as a customer with an invoice e-mail and delete the account from My account. That customer's invoices are gone and other customers' invoices remain.
Place an order with a billing e-mail different from the account's, then delete that customer in Admin > Customers. The invoice e-mail and its Download document are gone.
Deactivate the message template and place an order. No e-mail is queued and no new Download document remains.
An invoice holds customer data, so it belongs in the application's file
storage, with a lifetime tied to the e-mail it was generated for. Until now
it depended on a setting, and neither kind of copy was ever removed.
AttachedDownloads cannot carry this: it also holds the message template's
attachment, which many e-mails share. InvoiceDownloadId marks the one
download an e-mail owns, so deleting the e-mail can delete it safely. A
requeued e-mail gets its own copy for the same reason.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Queue-deletion races can orphan PDFs containing customer data, while unhandled invoice-cleanup failures can block loyalty-point awards and gift-voucher activation after an order is marked Complete.
The reason will be displayed to describe this comment to others. Learn more.
Not changed. This catch is a deliberate best-effort boundary: PDF composition and file storage fail with many unrelated exception types, and none of them may stop the surrounding work. The exception is logged, and the same pattern is already used in OrderNotificationCommandHandler and across the business layer.
The reason will be displayed to describe this comment to others. Learn more.
Not changed. This catch is a deliberate best-effort boundary: PDF composition and file storage fail with many unrelated exception types, and none of them may stop the surrounding work. The exception is logged, and the same pattern is already used in OrderNotificationCommandHandler and across the business layer.
The reason will be displayed to describe this comment to others. Learn more.
Not changed. This catch is a deliberate best-effort boundary: PDF composition and file storage fail with many unrelated exception types, and none of them may stop the surrounding work. The exception is logged, and the same pattern is already used in OrderNotificationCommandHandler and across the business layer.
The reason will be displayed to describe this comment to others. Learn more.
Not changed. This catch is a deliberate best-effort boundary: PDF composition and file storage fail with many unrelated exception types, and none of them may stop the surrounding work. The exception is logged, and the same pattern is already used in OrderNotificationCommandHandler and across the business layer.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Approval is blocked by unresolved correctness and reliability issues in customer-deletion cleanup, including missed soft-deleted orders, excessive order materialization, cross-customer invoice deletion, and orphaned copied downloads on insertion failure.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Two moderate active findings leave uploaded invoice files orphaned when metadata insertion or bucket deletion fails, so the change is not approval-ready.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Clean up copied invoice files when metadata insertion fails
The same upload-before-insert failure also applies to the requeue copy: InsertDownload can upload the copied bytes and then throw while inserting its metadata. In that case no copy id is returned, so Requeue never reaches InsertQueuedEmail and its rollback cannot run; the copied invoice is left unowned. Make CopyInvoiceDownload (or the underlying download insert) clean up the uploaded object when metadata insertion fails.
Preserve download metadata when bucket deletion fails
DeleteDownloadById delegates to DeleteDownload, which removes the Download document before deleting its bucket object. DeleteInvoices catches a bucket-delete failure and then removes the queued email, so a transient storage failure (or a process interruption between these operations) loses the only object id and leaves the invoice file orphaned permanently. Delete the file before removing its metadata, or otherwise preserve/retry the object id when file deletion fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Type: bugfix
Issue
The order placed, paid and completed e-mails can attach a PDF invoice. By default the invoice was written as a file to
wwwroot/assets/files/exportimport. WithAttachPdfInvoiceToBinaryon it went to Downloads instead, and the order completed e-mail then produced both.Neither copy was ever removed: not when the e-mail was deleted, not when all e-mails were cleared, and not when a customer deleted their account. An invoice holds customer data, so it should live in the application's file storage and go away with its e-mail.
Solution
SaveOrderToBinary.PrintOrderToPdfand theAttachPdfInvoiceToBinarysetting are removed.QueuedEmail.InvoiceDownloadId. It marks the one download an e-mail owns.AttachedDownloadscannot do this, because it also holds the message template's attachment, which many e-mails share. The send task adds the invoice to the e-mail's attachments.QueuedEmailServicedeletes the invoice when one e-mail is deleted, when selected e-mails are deleted, inDeleteAllEmailsand inDeleteCustomerEmail. The invoice goes first, so a delete that is interrupted can be repeated. Only the e-mails read at the start are deleted with their invoices, so an e-mail queued in the meantime keeps its invoice and stays in the queue. A file that cannot be deleted is logged and does not stop the rest or the account deletion.EntityDeleted<Customer>deletes the invoice e-mails of that customer's orders throughDeleteOrderInvoiceEmails. It reads only order ids and includes deleted orders. An invoice goes to the order's billing address, which may not be the account's e-mail, and only My account callsDeleteCustomerEmail. The handler also covers an admin, a company owner, the API and OIDC deleting a customer. Other e-mails of a customer deleted that way stay in the queue, as they did before.CopyInvoiceDownload, so each e-mail owns its download and no reference counting is needed. The requeued e-mail keeps the order reference, so it is found when the customer is deleted.OrderMessageProviderServicedeletes the invoice it was given. A file that cannot be deleted there is logged.InsertQueuedEmailalso deletes the invoice of an e-mail it could not store, which covers the copy made by a requeue. One limit: if a plugin'sEntityInserted<QueuedEmail>handler throws after the e-mail is stored, the order message path still deletes the invoice, and that e-mail goes out without it. No handler like that exists in this repository.The new queries read only ids. The ones in
DeleteAllEmailsandDeleteCustomerEmailscan what the delete next to them removes anyway, andDeleteOrderInvoiceEmailsruns only when a customer is deleted, so there is no new index. The invoice downloads have noDownloadGuid, so the storefront download action does not serve them.No new resources, settings or migrations. The unused resource
Admin.Settings.Order.AttachPdfInvoiceToBinaryis removed fromDefaultLanguage.xml.Breaking changes
For plugins and custom code, all of which need a rebuild:
IPdfService.PrintOrderToPdfis removed, andPdfServiceno longer takesIWebHostEnvironment.IOrderMessageProviderService:SendOrderPlacedCustomerMessage,SendOrderPaidCustomerMessageandSendOrderCompletedCustomerMessagetakeinvoiceDownloadIdinstead ofattachmentFilePath,attachmentFileNameandattachments.IMessageProviderService.SendNotificationhas a new optional parameter at the end, so a custom implementation must add it.IQueuedEmailServicehas new membersCopyInvoiceDownloadandDeleteOrderInvoiceEmails, andIDownloadServicehasDeleteDownloadById. TheQueuedEmailService,OrderMessageProviderService,OrderStatusCommandHandlerandProcessOrderPaidCommandHandlerconstructors changed.OrderSettings.AttachPdfInvoiceToBinaryand its model property are removed. A stored value is ignored on load and dropped on the next save.Upgrading in place:
QueuedEmail.AttachmentFilePathand the file branch ofEmailSenderstay, so they still read their files.wwwroot/assets/files/exportimportare not touched. Remove theorder_*.pdffiles once those e-mails have been sent.AttachedDownloadsstay when their e-mail is deleted, because they cannot be told apart from template attachments.Testing
Unit tests pass:
Grand.Business.Messages.Tests(108),Grand.Business.Storage.Tests(81),Grand.Business.Checkout.Tests(499),Grand.Business.Common.Tests(171),Grand.Modules.Tests(346),Grand.Mapping.Tests(237). The full solution builds. The other test projects were not run.A throwaway program, not part of this PR, ran the real services against LiteDB and MongoDB: queue, send task with a fake sender, requeue copy, and the three kinds of delete, including an e-mail document without the new field. All checks passed on both. A second one ran
DeleteOrderInvoiceEmails,DeleteCustomerEmailandDeleteAllEmailsagainst both databases with a fake download service, including an e-mail queued while the invoices were being deleted, which kept its invoice. On MongoDB it also covered a document without the new field, and the event handler itself: invoices of the customer's live and deleted orders went, another customer's stayed. Real SMTP delivery, the event being raised in a running application and the admin screens below were not run.wwwroot/assets/files/exportimport. TheQueuedEmaildocument hasInvoiceDownloadId, pointing at aDownloaddocument.InvoiceDownloadIdholds only the invoice.InvoiceDownloadId, the sameReferenceandObjectId, and sends with the invoice.Downloaddocument and file are gone, while the requeued e-mail's invoice and the template attachment remain.Downloaddocument are gone.Downloaddocument remains.