Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Prometee opened this issue Apr 30, 2019 · 3 comments
Open

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

Prometee opened this issue Apr 30, 2019 · 3 comments
Labels
Feature New feature proposals.

Comments

@Prometee
Copy link
Contributor

Prometee commented Apr 30, 2019

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 ?

@Prometee Prometee changed the title [Feature] Add ability to resend failed generated PDF [Feature] Add ability to resend automatically failed generated PDF Apr 30, 2019
@bartoszpietrzak1994
Copy link
Contributor

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
Copy link
Contributor Author

Prometee commented May 6, 2019

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

@bartoszpietrzak1994
Copy link
Contributor

Hi Francis!

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

@bartoszpietrzak1994 bartoszpietrzak1994 added the Feature New feature proposals. label May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals.
Projects
None yet
Development

No branches or pull requests

2 participants