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

SagePayForm: Use valid delimiter in VendorTxCode #185

Closed
wants to merge 1 commit into from

Conversation

aprofeit
Copy link
Contributor

Problem

I had a creepy feeling after we merged e9be51c. I checked the SagePay docs and realized ; is not a valid character for VendorTxCode. Close one.

Changes

Use - instead.

Review

@ThereExistsX @ivanfer

@@ -252,7 +252,7 @@ def message

# Vendor-supplied code (:order mapping).
def item_id
params['VendorTxCode'].split(';').first
params['VendorTxCode'].rpartition('-').first
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between this and split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a string like abc-123-def split doesn't have the capability of returning abc-123.

irb(main):003:0> "abc-123-def".split('-')
=> ["abc", "123", "def"]
irb(main):004:0> "abc-123-def".split('-', 1)
=> ["abc-123-def"]
irb(main):005:0> "abc-123-def".split('-', 2)
=> ["abc", "123-def"]
irb(main):006:0> "abc-123-def".split('-', 3)
=> ["abc", "123", "def"]
irb(main):007:0> "abc-123-def".rpartition('-')
=> ["abc-123", "-", "def"]
irb(main):008:0>

@ivanfer
Copy link
Contributor

ivanfer commented Dec 16, 2015

nice catch - 🌴

@ThereExistsX
Copy link
Contributor

I was going to mention that you shouldn't split on - for that very reason, good save. You'll need to update the helper tests though.

Since ; is not allowed according to docs
@aprofeit aprofeit force-pushed the adjust-sagepay-vendor-tx-code branch from 45823e5 to 9b39de4 Compare December 16, 2015 19:11
@aprofeit
Copy link
Contributor Author

In the tests I have order-28-1234 as the VendorTxCode on the test requests just to make sure it splits it correctly. Fixed tests, will be green now.

@aprofeit aprofeit closed this in 2bf09b6 Dec 16, 2015
@aprofeit aprofeit deleted the adjust-sagepay-vendor-tx-code branch December 16, 2015 19:21
BrianSigafoos pushed a commit to playpass/offsite_payments that referenced this pull request Dec 2, 2017
Since ; is not allowed according to docs

Closes activemerchant#185
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants