Sylius / InvoicingPlugin

Generate an Invoice for every placed order
MIT License
79 stars 82 forks source link

[Feature] Add ability to resend automatically failed generated PDF #127

Open Prometee opened 5 years ago

Prometee commented 5 years ago

I know this is possible through the admin order area, but this is the use case :

If we want to extends this plugin and give us the ability to build invoice PDF through something else than wkhtmltopdf like an external API (Quickbooks, SageOne, etc) we have to be able to stop the invoice mail sending if something get wrong generating an Exception. But this Exception will stop the order process and we have to take care about our customers experience first.

What I suggest is to give us the ability to return null when using Sylius\InvoicingPlugin\Generator\InvoicePdfFileGeneratorInterface::generate(string $invoiceId): InvoicePdf

So if the PDF is not generated (is null) we don't send the mail.

But we have to store somewhere the fact we don't send it ($invoice->setPdfSent(true);) and also retry to send it later with a Command like sylius-invoicing:generate-invoices --only-not-sent=1.

What do you think of this feature ? Do you see any enhancement to add to this feature ?

bartoszpietrzak1994 commented 5 years ago

Hi Francis!

While I think that we should make the invoice sending related abstraction more customizable, I'm not convinced that allowing null to be returned from the generate method is the best possible solution.

The business value that you want to achieve here is to allow the pdf file generator to fail during invoice generation and do not let this failure stop the whole order payment process. That sounds reasonable to me.

Hence, the cleanest solution would IMO look like this:

try {
    $pdfInvoice = $this->invoicePdfFileGenerator->generate($invoice);
} catch (InvoiceFileGenerationFailed $exception) {
    $invoice->setPdfSent(false);

    return;
}

I agree that being able to generate (and probably send) the pdf files for Invoices with pdfSent flag set to false using CLI would be a welcome change. However, I need to clarify that right now generating an invoice means only that the object with certain data is stored in the database. The pdf file is generated and sent to the customer on demand (this behavior will be refactored in a while so the pdf file will be created right after Invoice entity creation).

That being said, the sylius-invoicing:generate-invoices command is used to create invoices for orders that had been placed before the plugin's installation. Making it possible to generate pdf files for invoices with pdfSent flag set to false would require another command to be created, i. e sylius-invoicing:generate-pdf-files.

We, as the Sylius Core Team, will talk about this issue soon and come back with some output :)

Prometee commented 5 years ago

Great answer I totally agree with you. This plugin is very useful, can't wait to see it updated ;)

bartoszpietrzak1994 commented 5 years ago

Hi Francis!

We've refined that issue and put it in our backlog. We'll work on it soon :)