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

Some suggestions #1

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Some suggestions #1

wants to merge 15 commits into from

Conversation

ekes
Copy link

@ekes ekes commented Mar 22, 2022

First: many thanks for posting this module and your work here. Getting it on d.o would also be great. Got us out of a tight turn round situation.

In using it we encountered a few issues which were fixed with the code in the PR:

  • Because it includes dealing with the old unmaintained library code from SagePay itself it also changes the line endings to standardise the repo;
  • After that most changes there are to harden up it's XML handling. It rather liked being able to send & in particular when they must be encoded;
  • SKUs valid characters are even more restricted, so there's some code there to prevent anything SagePay with choke on;
  • Reductions weren't being applied to the basket so that's added;
  • For anything else that isn't being applied to the basket there's additional code to check that commerce agrees with the basket total, if they aren't the same, the commerce total is used and the difference logged;
  • It adds the Order Number, or Order ID, in the VendorTxID, making it easier to cross reference payments. By default the Order Number isn't generated till success is returned, so Order ID is sent. If you want to it's possible to add a 'pane' into the order flow and generate an order number before going to SagePay.
  • Finally, while SagePay Opayo was having it's little several hour outage I quickly added some code to the failure return handler to make the messaging more correct. I'm guessing the original (upgraded) code had one handler, so it's kinda copied over.

Hope this helps to harden the module up a touch.

ekes added 15 commits March 9, 2022 12:17
commit 48399616e226016f4debe48e8d04e4efafc1a77d
Author: Rob Mills <[email protected]>
Date:   Tue May 5 17:00:13 2015 +0100
commit ffa5e275b063f3282cc3d8871513d0a6a4431555
Author: Rob Mills <[email protected]>
Date:   Tue May 5 17:17:13 2015 +0100
commit 1207938a220ad05a202890a0ae9491c990756482
Author: Rob Mills <[email protected]>
Date:   Mon May 11 11:45:18 2015 +0100
commit f6de8f885fafb08ac4ed67c9aa074e3ad9261ff7
Author: Rob Mills <[email protected]>
Date:   Thu May 7 16:54:39 2015 +0100
commit 8d5f5040832f4255eda6822ff703aadf4717a0cb
Author: Rob Mills <[email protected]>
Date:   Mon May 11 12:40:13 2015 +0100
Mainly if you put a & into createElement it throws a fatal.
If for whatever reason the basket doesn't get the correct total, trust
the total from commerce. The checkout won't have the order details, but
will have the commerce total to bill.
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.

1 participant