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.

Sign up for GitHub

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

change the way street and number are serialized to attempt a problem … #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

change the way street and number are serialized to attempt a problem … #92

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/spree/mollie/order_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def prepare_line_items

def serialize_address(address)
{
streetAndNumber: [address[:address1], address[:address2]].join(" "),
streetAndNumber: "#{address[:address1]} #{address[:address2]}".strip,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

city: address[:city],
postalCode: address[:zip],
country: address[:country],
Expand Down