efdevcon / pretix-attestation-placeholder-plugin

Pretix plugin to support placeholders for attestations
MIT License
2 stars 7 forks source link

Model for attestation link and Email Render #19

Closed Nurts closed 3 years ago

Nurts commented 3 years ago

What was wrong ?

How it was fixed?

Example of generated email:

Screenshot from 2021-03-16 01-20-07

Nurts commented 3 years ago

Hi @ligi , I have several things that I am unsure about in this PR.

ligi commented 3 years ago

First I had to change the code in generate_link function from email = order_position.attendee_email to email = order_position.order.email. Because attendee_email was not an obligatory field, and if it was None, it failed to generate. But I am not sure if this is correct.

I think we should use order_position.attendee_email here and fail if it does not exist. For the attestation it is important that the attendee email is used and we set it to mandatory in pretix settings. If the plugin can set it that would be even more ideal - but I think this is the way to go now.

Also I am not sure about the text which results from render function in OrderAttestationPlaceholder, because there might be multiple tickets in an order, I had to render multiple links

we will use the placeholder in "Text sent to attendees" so AFAIK there should be only one

Also If something goes wrong with generation, then the order payment fails, because of the error in register_order_placed signal. Should the signal pass even if the generation fails or is the current approach is correct ?

What error cases do you see here? I think we should pass - but also will bring it up with the PM who makes the decision

Nurts commented 3 years ago

Thank you for the reply

What error cases do you see here? I think we should pass - but also will bring it up with the PM who makes the decision

generate_link function can raise ValueErrors, for example if key file was not found

ligi commented 3 years ago

Thanks - I still think we should pass here - but log a big red warning.

The generated text should then be something like "Could not generate attestation URL - please contact support@devcon.org"

what else is open from your side before this PR can merge from Draft to Mergeable?