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

Jump to bottom

REF Use the new prepareParamsForPaymentProcessor function in more places #15281

Merged
merged 1 commit into from Oct 8, 2019
Merged

REF Use the new prepareParamsForPaymentProcessor function in more places #15281

merged 1 commit into from Oct 8, 2019

Conversation

mattwire
Copy link
Contributor

Overview

Follow on from #15280

Before

More code duplication

After

More code consolidation

Technical Details

Comments

#15280 should be reviewed and merged first.

@civibot
Copy link

civibot bot commented Sep 11, 2019

(Standard links)

@civibot civibot bot added the master label Sep 11, 2019
@mattwire mattwire changed the title Use the new prepareParamsForPaymentProcessor function in more places REF Use the new prepareParamsForPaymentProcessor function in more places Sep 11, 2019
@eileenmcnaughton
Copy link
Contributor

@mattwire makes sense - wanna rebase it

@mattwire mattwire force-pushed the formatpaymentparams_useprepare branch from 941417d to b1b7f4e Compare September 12, 2019 08:28
@mattwire mattwire mentioned this pull request Sep 12, 2019
Merged
stesi561
stesi561 reviewed Sep 18, 2019
View reviewed changes
CRM/Event/Form/Participant.php
$this->_params["state_province-{$this->_bltID}"] = $this->_params["billing_state_province-{$this->_bltID}"] = CRM_Core_PseudoConstant::stateProvinceAbbreviation($this->_params["billing_state_province_id-{$this->_bltID}"]);
$this->_params["country-{$this->_bltID}"] = $this->_params["billing_country-{$this->_bltID}"] = CRM_Core_PseudoConstant::countryIsoCode($this->_params["billing_country_id-{$this->_bltID}"]);

$this->_params = $this->prepareParamsForPaymentProcessor($this->_params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter that now these are only being set if !empty($params["billing_state_province_id-{$this->_bltID}"]) and similarly for country id?

and that
$params['state_province']
and
$params['country']

are now being set as well? I assume not but just wanted to double check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is the fact that we were not previous testing for empty values the problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stesi561 The fact we weren't checking for empty values would potentially cause PHP notices. And missing params that are now being set means we might now be saving data that might not have been saved previously (or the payment processor had to implement custom handling to look for the parameter) - eg. for Country.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this change won't clash with custom handling in payment processors. The dummy processor was already saving address data. However if this is a change of behaviour I think it's possibly worth referencing in the PR description?

@stesi561
Copy link
Contributor

stesi561 commented Sep 18, 2019 •
edited

The only thing I could spot is that I think there is a slight change in behaviour here - that I'm not sure is important or not. I've commented on the file concerned.

@stesi561
Copy link
Contributor

To r-run this do I need to set up an actual payment processor or will the dummy payment process give enough coverage?

@mattwire
Copy link
Contributor Author

mattwire commented Oct 4, 2019

@stesi561 It should be sufficient to test with the dummy payment processor. Really we are just checking that there is no loss of information being saved and no new PHP notices.

@mattwire
Copy link
Contributor Author

mattwire commented Oct 4, 2019

test this please

@mattwire
Copy link
Contributor Author

mattwire commented Oct 4, 2019

@stesi561 Thankyou for reviewing / testing. I've triggered the test build again if you have time for r-run?

@stesi561
Copy link
Contributor

stesi561 commented Oct 8, 2019

My initial test of http://core-15281-8xhkx.test-1.civicrm.org:8001/civicrm/event/register?reset=1&id=1 while logged in resulted in the below error - this happened before the confirmation page however I can't seem to replicate this again - as either logged in or anon. This error seems to be related to a missing id in the url - the changes don't seem to relate to this so I don't think this is cause for concern.

No warnings/notices showing in watchdog/or on screen.

Address data appears to be saving correctly.

#0 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Core/Error.php(381): CRM_Core_Error::backtrace()
#1 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Utils/Request.php(121): CRM_Core_Error::fatal("Could not find valid value for id")
#2 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Event/StateMachine/Registration.php(52): CRM_Utils_Request::retrieve("id", "Positive", Object(CRM_Event_Controller_Registration), TRUE)
#3 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Event/Controller/Registration.php(47): CRM_Event_StateMachine_Registration->__construct(Object(CRM_Event_Controller_Registration), TRUE)
#4 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Core/Invoke.php(285): CRM_Event_Controller_Registration->__construct("Event Registration", TRUE, "null", NULL, "false")
#5 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Core/Invoke.php(84): CRM_Core_Invoke::runItem((Array:15))
#6 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/CRM/Core/Invoke.php(52): CRM_Core_Invoke::_invoke((Array:3))
#7 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/sites/all/modules/civicrm/drupal/civicrm.module(444): CRM_Core_Invoke::invoke((Array:3))
#8 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/includes/menu.inc(527): civicrm_invoke("event", "register")
#9 /home/jenkins/bknix-dfl/build/core-15281-8xhkx/web/index.php(21): menu_execute_active_handler()
#10 {main}

@stesi561
Copy link
Contributor

stesi561 commented Oct 8, 2019

I've updated original review (r-run and r-explain) .

@seamuslee001 seamuslee001 merged commit b548bf8 into civicrm:master Oct 8, 2019
@mattwire
Copy link
Contributor Author

mattwire commented Oct 8, 2019

@stesi561 Thanks for the review!

@eileenmcnaughton
Copy link
Contributor

This PR caused https://lab.civicrm.org/dev/event/issues/25

@mattwire mattwire deleted the formatpaymentparams_useprepare branch December 23, 2019 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stesi561 stesi561 stesi561 left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
4 participants
@mattwire @eileenmcnaughton @stesi561 @seamuslee001