change the way street and number are serialized to attempt a problem …#92
change the way street and number are serialized to attempt a problem …#92
Conversation
…that occurs on some orders where the housenumber is not attached
|
@vernondegoede can you review this commit? |
| def serialize_address(address) | ||
| { | ||
| streetAndNumber: [address[:address1], address[:address2]].join(" "), | ||
| streetAndNumber: "#{address[:address1]} #{address[:address2]}".strip, |
There was a problem hiding this comment.
According to the Spree documentation, address2 is a second address ("Suite 13") instead of a house number. Can you elaborate on the issue you're trying to solve exactly?
By using join you're actively trying to prevent whitespace, in case there is no address2. Using this solution you'll most likely always have whitespace, which you then strip.
Also, please add tests.
There was a problem hiding this comment.
I got contacted about housenumbers not being added to the serialized address send to klarna (and mollie) in some cases.
Generally testing the feature did not result in finding anomalies. Housenumbers in address2 were always added to the serialized order.
However, by using an order known to have been send without an address to mollie I could reproduce the issue.
There are no apparent anomalies with this order, or the order's address to be exact. The address1 contained a streetname and address2 contained a string value of 6.
By instead of composing an array of either string or nil values and joining them using the join method, using string interpolation and stripping eventual whitespace the issue with this specific order was solved.
The tests currently in place for serializing the address do cover this code.
Since the code change essentially should have the same outcome as the code before the pr I'd like to test this in production to see if this resolved the issue for more clients.
Any extra eyes on the issue are also appreciated 😉
There was a problem hiding this comment.
Sorry, I still don't see the exact issue with this line of code. I believe the line was better before this change.
If the test data is the same as the data you used to solve this issue in production, something else is most likely going on. If the data in production is different, please add a different test case to cover that situation as well, and we should be seeing a difference when running the tests.
Re: the order: feel free to send the order number to my email address and I'll take a look at the order data.
|
After some further investigating by @peterberkenbosch and I, it turns out the issue @Oldharlem was experiencing would not have been solved by this PR. The webshop which was not submitting house numbers was running an old version of the gateway, specifically one which does not include 4383f46. The easiest solution is upgrading the gateway. Thanks everyone who helped figure this out. This PR can be closed in my opinion. |
3 participants
…that occurs on some orders where the housenumber is not attached