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

Payment transaction should be customizable using a template #4156

Closed
Viicos opened this issue Apr 11, 2024 · 8 comments · Fixed by #4168
Closed

Payment transaction should be customizable using a template #4156

Viicos opened this issue Apr 11, 2024 · 8 comments · Fixed by #4168
Assignees
Labels
bug Something isn't working needs-backport Fix must be backported to stable release branch owner: utrecht topic: betalen Category from PvE
Milestone

Comments

@Viicos
Copy link
Contributor

Viicos commented Apr 11, 2024

Discussed with Utrecht (Taiga 200), and the solution as described with the number of attempts wasn't done conform spec of this Github issue:

"we kunnen de prefix met het jaartal er wel afhalen, maar wat na het zaaknummer komt staat nu als volgt gedefinieerd OF-nummer/xxx. Dat moet OF-nummer_xxx zoals ook in het betreffende github issue stond"

[We can remove the prefix with the year, but what comes after the case number is now defined as follows OF-number/xxx. This must be OF-number_xxx as stated in the relevant GitHub issue.]

Originally posted by @alextreme in #3793 (comment)


Since 2.6.0, the payment reference now follows this pattern:

{prefix}/{public_registration_reference}/{payment.pk}

For example, with a prefix of {year}: 2024/OF-123456/945

  • OF-123456 is a registration reference generated in OF, but can be a zaak number as well.
  • 945 is a unique incrementing ID (the database PK), which creates confusion as this is NOT the payment retry attempt number.

This must be OF-number_xxx

According to #3793 this wasn't a strong requirement, and we had a checkbox stating Determine if an attempt postfix is needed (for example when you cancel the first payment, and make another attempt).

@alextreme is there a specific reason to have it in the _XXX format? Should it be the retry attempt number? I think we went for the payment database PK to be certain it would generate a unique number, but probably this could work as well with the payment retry?

@sergei-maertens
Copy link
Member

sergei-maertens commented Apr 11, 2024

Yeah the wording of the original issue does not make clear that the suffix with underscore is a hard requirement. I find those numbers to be quite ugly, and I'm concerned what will happen when another organisation uses a separator other than _, then things are not going to work for them.

The Taiga issue mentions a SAP integration, so I assume some splitting on the reference number is being performed in there, based on the _ suffix.

I'm also concerned about the PK value vs. attempt increment - the latter makes it a lot harder to guarantee unique order IDs being sent to the payment provider and I'd really prefer if we can stick to our PK values as suffixes.

edit: I've asked for clarification in the Taiga issue

@alextreme
Copy link
Contributor

I'm not going to interfere in this, in my eyes a difference between explicit and implicit requirements. The original issue was a Utrecht-labelelled issue so I'd only choose to deviate if there's a good reason and if Utrecht doesn't have a problem with the solution, but this is for you to discuss with @joeribekker

@sergei-maertens
Copy link
Member

Clarified via Taiga - we can keep using the PK as suffix.

@Viicos let's change the separator to an underscore instead of / (only the last one is fine, so a number like 2024/OF-1234_42 is okay).

If other organizations need a different suffix separator, we can then add envvar/global configuration support for it.

@sergei-maertens sergei-maertens added bug Something isn't working needs-backport Fix must be backported to stable release branch labels Apr 11, 2024
@sergei-maertens sergei-maertens added this to the Release 2.6.4 milestone Apr 11, 2024
@joeribekker
Copy link
Contributor

joeribekker commented Apr 11, 2024

Ik wil deze heel even in overleg gooien.

Ik wil niet dat Open Formulieren afdwingt dat alle gemeenten het Utrecht formaat moeten gebruiken en ik wil niet deze impliciete afspraak met Utrecht over het formaat straks per ongeluk wordt gewijzigd en zaken stuk gaan omdat Utrecht er business rules aan heeft hangen.

Dus: Dit formaat moet configureerbaar zijn. En sterker nog, ik wil geen wijziging in het formaat by default. Straks heeft een andere gemeente er ook business rules aan hangen.

Just support

PAYMENT_REFERENCE={year}/{ref}/{uid}.

Or something

@Viicos
Copy link
Contributor Author

Viicos commented Apr 11, 2024

Just support

PAYMENT_REFERENCE={year}/{ref}/{uid}.

Or something

I think I like this solution, it makes things a bit cleaner, as you are not limited to specifying a prefix and this could cover extra requirements from municipalities (e.g. if we ever get a request to have the payment reference as {ref}--{ref}/{uid} or whatever). But we should be extra careful to validate the provided "template", as it could lead to duplicate refs issues if uid is not used.

@sergei-maertens
Copy link
Member

To be discussed tomorrow :)

@Viicos
Copy link
Contributor Author

Viicos commented Apr 12, 2024

Refinement:
the global configuration prefix field should be rename to be a template:

  • it can include the {year}, {reference} and {uid} placeholders (do not use .format but .replace is fine).
  • {uid} should be present to avoid duplicates
  • Already existing instances migrating to 2.6.x should have a default value of {year}/{uid} (for backwards compatibility). If different value than {year} (the default) is set, it should be concatenated with /{uid}.
  • New instances should have a default value of {year}/{reference}/{uid}

@sergei-maertens
Copy link
Member

Note that there was no separator at all between the prefix and order ID, see

def create_public_order_id_for(payment: "SubmissionPayment") -> str:

Viicos added a commit that referenced this issue Apr 15, 2024
Viicos added a commit that referenced this issue Apr 15, 2024
Viicos added a commit that referenced this issue Apr 15, 2024
@Viicos Viicos linked a pull request Apr 15, 2024 that will close this issue
sergei-maertens added a commit that referenced this issue Apr 15, 2024
…emplate-2

[#4156] Use a template for payment reference
sergei-maertens pushed a commit that referenced this issue Apr 15, 2024
sergei-maertens added a commit that referenced this issue Apr 15, 2024
…te-forward-port

[#4156] Switch from prefix to template field
@Viicos Viicos changed the title Payment transaction should include a retry suffix Payment transaction should be customizable using a template Apr 15, 2024
@Viicos Viicos closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment