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

Add billingblock region to event registration thankyou to match contribution thankyou #13762

Merged

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Mar 4, 2019

Overview

This allows us to use the same code in payment processor extensions for the "confirm" and "thankyou" section of the payment process for both contributions and event registrations.

Before

If you want to alter/replace the billingblock on event registration confirm/thankyou pages you have to target it via jquery or alterContent hook. But for contribution thankyou you just replace the region with one that displays billing information relevant to the payment processor.

After

Same method to replace billingblock for both event and contribution thankyou workflows. Simpler code, more consistency...

Technical Details

Insert crmRegion into the smarty templates (I did consider different crmRegion names for the event pages but you can differentiate by the form class name if you need to and we need more consistency for payment processors so you can just write it once rather than write it differently for each workflow!

Comments

@seamuslee001 @jitendrapurohit Either of you able to take a look. Most of the PR is just formatting (indenting), the only change is adding the crmRegion tags to the two event pages so should not break anything - and existing extensions that target those regions will not be affected as they still need to add event form class name to actually change the region.

@civibot
Copy link

civibot bot commented Mar 4, 2019

(Standard links)

@civibot civibot bot added the master label Mar 4, 2019
@mattwire
Copy link
Contributor Author

mattwire commented Mar 4, 2019

@JoeMurray
Copy link
Contributor

Event payment stuff got a rewrite first to support partial payments without this being extended to contributions. Not sure if / how this PR is handling that difference / affected by it.

@mattwire
Copy link
Contributor Author

mattwire commented Mar 4, 2019

@JoeMurray Thanks for flagging. I'm pretty certain that will make no difference for this PR (as there is no change unless a payment processor extension specifically overrides the billing block).

@eileenmcnaughton
Copy link
Contributor

I pulled this down & realised the bulk of the changes were just whitespace formatting - I opened this to cover that #13782 - if you drop that file out it will be cleaner.

@eileenmcnaughton
Copy link
Contributor

As to the actual billing block chunks I have 2 reservations

  1. the names of the billing blocks are tied to the contribution page - I understand the convenience of similarity but it makes the convention kinda odd- existing ones are documented https://docs.civicrm.org/dev/en/latest/framework/region/

  2. I'm kinda loath to expose this without cleaning it up first. We shouldn't have stuff like

{if $paymentProcessor.payment_type == 2}

hard -coded into the tpl files - we should be iterating through $paymentFields - which comes from CRM_Core_Payment::getPaymentFields for what to display - similar to back when we changed the paymentSetLabel to come from the processor - eileenmcnaughton@9f7f8a5#diff-fc454d9ef40ac8417cb3c056269d4cccR226

@mattwire mattwire force-pushed the event_register_thankyou_region branch from 2b2d4ea to cc885b9 Compare March 7, 2019 19:37
@mattwire
Copy link
Contributor Author

mattwire commented Mar 7, 2019

Ok, I've changed the names of the billingblocks so they are consistent with namings and added a docs PR here: civicrm/civicrm-dev-docs#586
Also a very minor cleanup to the contribution confirm billingblock for consistency - I feel like working on payment_type is out of scope for this PR (and it's not in the event templates)!

@mattwire
Copy link
Contributor Author

@eileenmcnaughton Is this ok to merge now?

@eileenmcnaughton
Copy link
Contributor

I really can't bring myself to merge a change to add a region around a chunk of code that needs to be first cleaned up so that it's determined in the first instance by the payment processor. I'm thinking how best to do it but we also have quite a few things on the go now so I also want to perhaps resolve some of the others first

@eileenmcnaughton
Copy link
Contributor

Ok @mattwire I see your comments on chat. Let's merge this for 5.13 then and continue to work to fix it. I can see that the 5.13 compatibility could be important for some extension writers & we can potentially phase out again

@eileenmcnaughton eileenmcnaughton merged commit bdea2c1 into civicrm:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants