Govukpay integration#5907
Conversation
5af80d7 to
d20e1c0
Compare
dracos
left a comment
There was a problem hiding this comment.
Hi, Thanks for looking at this! Is this for any use in particular, or just to add the general functionality?
Do you know how to use git rebase? For example, your "Add tests for Integrations::GOVUKPay" commit could be rebased/squashed with the commit that added the code, so that the code+tests for that code are in the same commit. And similar for other test-only commits. And then the two "fix" commits can presumably be squashed into the commits they are fixing. Bu that can be done at the end/by us, no biggie.
Could you explain the two fix commits a bit more?
"Replace HTTP::Request::Common::POST with explicit HTTP::Request->new to avoid header mangling when Content_Type/Content params are mixed with custom headers." - this shouldn't be needed, as far as I'm aware, what was going wrong?
And the other fix commit says "Add parentheses to JSON::MaybeXS::true() and JSON::MaybeXS::false() calls in both test files to avoid compilation errors." but the code isn't adding brackets, it's changing them to scalar references, which doesn't feel right; what compilation error were you getting before?
The docs are written like they're a commit message, rather than docs (e.g. "The CheckPayments cron script has been updated" doesn't make sense for someone reading this after the fact, as documentation).
I've made a few other small comments. I don't have access to a test GOV.UK Pay account to try it out, but in general this looks pretty good.Thanks again for your contribution
d3ba7d0 to
c23df25
Compare
c23df25 to
9f57611
Compare
|
Hi, Thanks for the thoughtful review and comments. This PR is just to add general functionality and as a learning opportunity of both git and the codebase. When I used HTTP::Request::Common::POST the header that was getting sent was garbled, I don't know why. The other issue was caused by originally having Bareword "JSON::MaybeXS::true" not allowed while "strict subs" in use at t/app/controller/waste_govukpay.t line 152. but changed to scalar references instead for simplicity, should have amended the commit message too I've made changes to the md doc and I think I've done the rebase correctly. |
This adds GOV.UK Pay as an alternative payment gateway alongside the existing SCP (Capita) and Adelante integrations. GOV.UK Pay is free for government and local authority use, making it attractive for councils that don't already have an SCP or Adelante contract.
Has the code POD documentation been added or updated?
Yes, Pod and .md included.
Whether this PR should include changes to any public documentation, or the FAQ;
No Public front end changes
All cobrand-specific commits start their commit message with the cobrand in square brackets;
Nothing is Cobrand-specific altho tests are based on cobrands
Is new functionality tested? CodeCov will warn you about the diff coverage, but won’t complain about e.g. new files;
Test files created based upon existing tests.
Will cobrand-specific changes require additional work to ensure consistent behaviour on www.fixmystreet.com?
The GOVUKPay role is only activated when a cobrand explicitly does with 'FixMyStreet::Roles::Cobrand::GOVUKPay' and provides govukpay_api_key in COBRAND_FEATURES.
Are the changes tested for accessibility?
Not applicable
Have you updated the changelog? If this is not necessary, put square brackets around this: skip changelog
No