mollie / mollie-api-python

Mollie API client for Python
http://www.mollie.com
BSD 2-Clause "Simplified" License
113 stars 55 forks source link

No longer possible to retrieve settlements by bank reference using mollie-api-python client 3.5.0 #335

Closed SalimAtMollie closed 10 months ago

SalimAtMollie commented 10 months ago

Issue

When they get the settlement with the bank reference , an error is thrown.

What happens

settlement = mollie_client.settlements.get("02893031.2309.10")

File "/home/erik/.local/share/virtualenvs/oakk_ordering-0m3EknA8/lib/python3.10/site-packages/mollie/api/resources/settlements.py", line 35, in get

   self.validate_settlement_id(settlement_id)

 File "/home/erik/.local/share/virtualenvs/oakk_ordering-0m3EknA8/lib/python3.10/site-packages/mollie/api/resources/settlements.py", line 28, in validate_settlement_id

   raise IdentifierError(

mollie.api.error.IdentifierError: Invalid settlement ID: '02893031.2309.10'. A settlement ID should start with 'stl_' , be 'next' or 'open' or contain a valid bank reference.

What should happen

The function should retrieve the settlement, instead of throwing that error.

Steps to reproduce

  1. Get an API key of a merchant with a settlement.
  2. Get a settlement reference number.
  3. Run this snippet of code
#Import Mollie API client for python
from mollie.api.client import Client

#Initialize client and set API key
mollie_client = Client()
mollie_client.set_api_key(API_KEY)

settlement = mollie_client.settlements.get(SETTLEMENT_REF)  #THIS LINE THROWS ERROR

Hypothesis

Looking at the code it seems like although they implemented the settlement.py so that they allow settlement reference numbers, it seems like they are still using a function found in base.py (line 82) which still (regex) checks for a prefix in the reference number and throws an “identifier error”.

whyscream commented 10 months ago

Hi @SalimAtMollie ,

Thank for opening an issue. Looking at your error, I'm seeing that the error message that you're getting, contains the specific text used in the override that is implemented in the Settlements resource (be 'next' or 'open' or contain a valid bank reference.), so there is no code from base.py involved. It would have surprised me when otherwise, as this specific scenario is under test.

However, the bank reference that you're using (02893031.2309.10) does not match the regular expression that is implemented. As noted in the code, the reference is based on a definition by Mollie, but in the commit where that was added, there is no reference to that actual definition (in Mollie docs, for example).

Did you make up this reference, or is this live data? If it is live, I'll have to check with Mollie what the correct definition is for a bank reference, and update the check.

fjbender commented 10 months ago

We don't have an official definition in the docs it seems, but looking at the reference @SalimAtMollie shared (which is a valid identifier) it seems the definition we use in this project is too narrow, as we do have 8 digit customer IDs. I think we should expand the regex accordingly.

@SalimAtMollie Would you have an exact specification for the bank reference that we could put on the docs?

whyscream commented 10 months ago

@fjbender This 'definition' as referered to in the code, might be the result of a discussion on Slack back in 2019... :) Would be nice to see an actual definition, if there's any.

SalimAtMollie commented 10 months ago

Hey @whyscream thanks for the reply!

Makes sense that the regex implemented doesn't work for this settlement ref. I have some more clarification on the formatting of the bank reference number:

The components are separated by a dot.

However, to make sure they are always the same length, the first part is fixed to 8 digits. So if the org ID is 12345, we'll format it as 00012345. This will once again change once we have merchants with 9-digit IDs, where the first part will be fixed to 9 digits. So if the org ID is 12345, we'll format it as 000012345.

In this case, I think we should adjust the regex to allow for more than 4 digits before the first dot:

BANK_REFERENCE_REGEX = re.compile(r'^\d{4,}\.\d{4}\.\d{2}$', re.ASCII)

Please let me know if you have any questions!

whyscream commented 10 months ago

Resolved in release 3.6.0