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

CRM-20610 make it possible to enable the payment form #11748

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 2, 2018

Overview

This makes is possible via hook or by changing core to switch to using the payment form for payments without actually making that change

Before

Contribution form is unchanged

After

Contribution form is unchanged UNLESS you use a hook or we agree to switch core to turn on the change by adding one additional tpl var - see https://github.com/civicrm/civicrm-core/pull/10776/files#diff-0573f78042d230ffc1d5e61c1749ae77R235

Technical Details

This is the material change from #10776 without resolving the decision as to whether we we 'turn this change on' - or allow people to opt in. One idea I have is that the LineItemEdit extension ALSO turn this on (by the above line to that extension). My feeling is that we should be moving towards a) shipping line item edit enabled in all dev builds with a view to doing so in core (or at least new installs) once we are comfortable and b) turning this on in all dev builds with a view to turning on for new installs once we are comfortable - ie. roll out to devs first & then work outwards.

Both the line item edit form and the payment edit are IMHO very important for getting us out of our whackamole on bugs that relate to the contribution form & unfortunately I think stalling on #10766 cost us heavily but @monishdeb I think you can merge this & then we can discuss - with @JoeMurray @lcdservices @colemanw & others whether to add to line item edit or find another way to grandfather it in

Comments

Screenshots & detail are in the mentioned PR


@lcdservices
Copy link
Contributor

+1 to get this merged. Personally I think the option should also be enabled without requiring additional steps. But I understand if we want to take baby steps with this.

@eileenmcnaughton
Copy link
Contributor Author

Right - I'm on the fence - but I kinda like the idea of having it enabled for at least one release for devs before making it more aggressive. In this context (ie. a temporary transitional setting) I'm inclined to use a define('USE_PAYMENT_FORM') as opposed to a setting per se. Not sure if there is a strong case for that - a setting just feels a bit more like a permanent decision

@JoeMurray
Copy link
Contributor

I like the idea of baby steps starting with devs and a Define.

@monishdeb
Copy link
Member

Happy with the patch :) Tested on backoffice form esp. to ensure there isn't unwanted side effect. Hence merging.

@monishdeb monishdeb merged commit 4b10b2f into civicrm:master Mar 2, 2018
@eileenmcnaughton eileenmcnaughton deleted the payment branch March 2, 2018 22:04
@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
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.

6 participants