[D8] Ubercart DPS Payment

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxJingQian2781691git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi

I'm the maintainer of existing UC PaymentExpress (D7 only release for now) and would be happy to see this submitted to the UC PaymentExpress project if that's an option. I'm open to additional maintainers who have demonstrated enthusiasm for contributing, and Jing Qian has established that with the work shown here.

Thanks!

I think it's fair to note that from the pareview scan:

Issues with the library (DPSApi/PxPay_Curl.ph) that is code supplied verbatim from the gateway providers specifically for incorporating into projects like this uses a different code standard, and it's more beneficial to maintenance and security to keep it untouched than it is to rewrite it in this case.

75 | WARNING | t() calls should be avoided in classes, use dependency
| | injection and $this->t() instead

"$this->t(" is being used in the lines it identifies, as as far as I can tell, this seems to be a false positive.

I don't know the recommended way to rewrite

28 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead

But I hope someone else can suggest the fix - if it's important.

The other complaints about SSL authentication and too-short commit messages are totally valid though.
You can't change old commit messages, but look at improving them going forwards.

I've run through and tested the module. Configuration seemed to work fine, the instructions were adequate.

I should only note that since the "Cart" submodule is not listed as a dependency, even though the project page states it is, I had to enable it manually. This is probably not something that really concerns this module though, since "Cart" is listed as something that must be enabled for the entire UC bundle to work.

Tested checkout success and failure. Both worked as expected. Success recorded the payment and the failure sent me back to the checkout.

I'm afraid I can't add much to the PAReview comments, but a manual review of the code does review some inconsistent use of the single and double quotes. I believe that policy is to use single quotes unless double quotes are required. Array notation is also not consistent, array() and [] are both used. I suggest standardising on the newer brackets.

Some nitpicking for the grammar used in the failure messages (2 places): "... please make sure the card detail is correct" should read "... please make sure the card details are correct". Plural, not singular.

None of these things are really showstoppers though, so I am setting to RTBC. If someone has any more knowledge about how to deal with the remaining complaints raised by PAReview then we await your input.

I should also note that since it seems this sandbox will be merged as 8.x-1.x into the Ubercart PaymentExpress module, the namespaces used here should probably be changed to match those of that module at some point (before the merge).

Used Simplytest.me to spool up a clean test environment. Followed instructions provided and had no issues except for messing up a copy and paste of the PX UID (which I didn't discover until the checkout). Nothing here that would be a review blocker but might be worthwhile adding some form of debug control as a nice to have. (The logs were a long way from helpful if I was to rely on them for some detail about the issue).

Tested checkout success and failure with a multitude of different card types, including ALL common declines. Where there were failures I was redirected back to the checkout page with an appropriate message (agree with @dieuwe's grammar nitpicks above) and successful payments were lodged correctly into the ordering system.

On subsequent checkouts there was an issue whereby using "Saved addresses" didn't appear to correctly populate the appropriate fields, but I believe this is unrelated to the particular project.

Sounds like we are almost ready to go.

What's the status RE taking over the https://www.drupal.org/project/uc_paymentexpress namespace?
My understanding is that:

• @xurizaemon has indicated he's happy to give it to @Jing Qian to take forward (which is excellent)
• On the strength of that endorsement + the above reviews, @Jing Qian can be promoted for that project immediately.

• Shifting the current module into that project as a new 8.x branch takes a little git wrangling (which I'm not an expert in) and renaming, but would be the best long-term result.
• There is no immediate expectation of an upgrade path between the 7.x and 8.x branch. (IMO) Re-configuring such a thing during a migrate will need manual intervention anyway.

Proposal:

• Promote @Jing Qian = YES
• Grant @Jing Qian maintenance of The existing module = @xurizaemon - will you do the honours please?
• Further work = Jing branches the 7.x to 8.x (use git checkout --orphan 8.x-1.x) and integrates this sandbox there.

Alternative

:

• Promote @Jing Qian = YES
• Allow this project to be promoted to "Full project"
• Update project page to refer to this as a successor.

(I prefer option A, but will take input on this)

Disclosure @Jing and I both work at Sparks Interactive, and occasionally even touch the same projects.

To avoid any questions about the system and our part in it, I would prefer another project reviewer to weigh in and actually pull the trigger and complete the promotion process - which is ready to go, whichever way the old/new module decision (above) goes.

(If not, I'll bump it through after a week anyway, per normal d.o policy for unopposed RTBC applications)

This has been RTBC unopposed since August 24.
Per the policy, and the discussions above, let's do this.

Jing, Please check in with xurizaemon about the next steps for project transferral.

------------------

Thanks for your contribution, Jing!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Jing, you've got full access to the project at https://www.drupal.org/project/uc_paymentexpress now.

Thanks!