CRM-14102 - UI on contribution page config #8180

vijaydwivedi75 · 2016-04-19T22:27:08Z

CRM-14102: UI on contribution page config -Execute real-time monetary transactions should not be uncheckable if $ configured
As given in link

civicrm-builder · 2016-04-19T22:27:09Z

Can one of the admins verify this patch?

eileenmcnaughton · 2016-04-19T23:36:06Z

ok to test

eileenmcnaughton · 2016-04-19T23:39:52Z

We follow Drupal coding standards - https://www.google.co.nz/#q=drupal+coding+standards&gws_rd=cr and that failed check is a coding standard style issue - seemingly an extra blank line.

Great to see patch for this. Hopefully it can be included in next months code review cycle if we get a good turnout - see https://github.com/civicrm/civicrm-core/pull/8152/files for more about the review cycle

vijaydwivedi75 · 2016-04-20T08:27:36Z

I resolved the coding standard style issue here - the extra blank line on line 87 in CRM/Contribute/Form/ContributionPage/Amount.php

eileenmcnaughton · 2016-04-20T08:47:48Z

Great - that particular one always catches me

Edzelopez · 2016-05-18T11:17:24Z

CRM/Contribute/Form/ContributionPage/Amount.php

@@ -337,6 +335,7 @@ public static function formRule($fields, $files, $self) {
    if (isset($fields['is_pay_later'])) {
      if (empty($fields['pay_later_text'])) {
        $errors['pay_later_text'] = ts('Please enter the text for the \'pay later\' checkbox displayed on the contribution form.');
+        $this->addElement('checkbox', 'is_monetary', ts('Execute real-time monetary transactions'));


@vijaydwivedi75, I'm not entirely sure about the intended functionality here, but creating an element only when the formRule is triggered seems wrong to me. @eileenmcnaughton, as per issue description, when I tried a contribution with pay later membership, without checking 'Execute real time monetary transactions', I got these errors.

Notice: Undefined variable: contribution in CRM_Contribute_BAO_Contribution_Utils::processConfirm() (line 221 of /home/xxx/CRM/Contribute/BAO/Contribution/Utils.php).
Notice: Trying to get property of non-object in CRM_Contribute_BAO_Contribution_Utils::processConfirm() (line 221 of /home/xxx/CRM/Contribute/BAO/Contribution/Utils.php).
Notice: Trying to get property of non-object in CRM_Contribute_Form_Contribution_Confirm->postProcessMembership() (line 1540 of /home/xxx/CRM/Contribute/Form/Contribution/Confirm.php).
Notice: Trying to get property of non-object in CRM_Contribute_Form_Contribution_Confirm->postProcessMembership() (line 1541 of /home/xxx/CRM/Contribute/Form/Contribution/Confirm.php).

No contribution or membership was created.

My fundamental belief is that that checkbox should be deprecated out of the schema. As far as I can tell if $amount > 0 then we should treat it as monetary so I would attempt to calculate that where possible.

Perhaps we could remove the checkbox from the form & set it appropriately on postProcess? It would certainly take some digging to be sure that was safe though

I second removing and setting on postProcess. Although, even if membership amount == 0, unchecking the box did not create contribution or membership for me.

I don't think we should be creating contributions if the amount is $0 - but I suspect that could be open to debate. We definitely SHOULD be creating memberships though.

Whenever we fix something on the front end contribution page we should add a test in this class api_v3_ContributionPageTest or for backoffice this class CRM_Contribute_Form_ContributionTest or the membership class.

Otherwise we can't expect this flakey code to stay fixed.

@eileenmcnaughton My concern is to allow pay later transactions without requiring real-time transactions. The widget itself seems dispensable, if I'm intuiting correctly what you hope to do with a 'Manual' payment processor. If a Manual processor is configured and no processor that accepts real-time payments are configured, then we would have a page that supports pay later payments but not real time payments. If this is the intent/outcome, then we can drop widget entirely, since it is no longer doing anything useful.

I don't have a strong view about having $0 contributions. I think in the abstract it's not a good idea, but I fear in the concrete that it has become embedded in code as a smelly work-around to deal with free memberships and perhaps also 'purchases' of free event tickets.

So, I believe the is_monetary (execute real time transactions) field has been redundant for as long as I can remember - ie. we can calculate whether there is an amount and we can calculate whether 'is_pay_later' is set without this field. It seems to introduce a potential for error with no gain.

In terms of this field the manual processor concept is tangental - ie. we don't need the field with, or without that concept being in play.

Re $0 transactions - I think you are probably right.

JoeMurray · 2016-06-13T16:49:47Z

Heh Vijay as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR?

monishdeb · 2016-06-20T12:08:03Z

@eileenmcnaughton as per the discussion on #8575 do we need to consider $isMonetary here to ensure $this->_value['payment_processor'] is set to manual processor here? as sooner or later is_monetary will be deprecated

+ if(!$isPayLater && !empty($this->_values['payment_processor'])) { that would be the final condition ?

JoeMurray · 2016-06-20T16:42:09Z

@vijaydwivedi75 would you be willing to refactor your PR along the lines suggested by @Edzelopez and @eileenmcnaughton ?

@PalanteJon would you be willing to review this PR, aiming for a narrow fix regarding UI but not changing anything wrt to eliminating $0 contributions etc? Possibly needs some further clarity on what is to be done.

eileenmcnaughton · 2016-06-20T18:41:45Z

My summary of what needs to happen here is that we should either

alter the PR to remove changes to ContributionBase as that issue should have been tackled elsewhere as mentioned by @monishdeb.

Also WRT edsel's comments
"@vijaydwivedi75, I'm not entirely sure about the intended functionality here, but creating an element only when the formRule is triggered seems wrong to me."

that is correct - the formRule should validate not add - and the PR would need to be changed to reflect that.

OR

Closing this PR is also a valid option. In that the work involved to implement no 1 may be more than anyone is up for & progress outside this ticket is bringing us closer to the larger solution of deprecating the parameter. I DO think it would be good to fix the form rule but I don't think this PR as it stands quite does that and I don't think having done this PR to this point should obligate @vijaydwivedi75 to rewrite it as described and if no-one is interested in doing that we should close it out (it's linked from the ticket for tracking)

JoeMurray · 2016-06-21T03:25:56Z

I agree with @eileenmcnaughton 's suggestions on status and possible directions. So @vijaydwivedi75 if you want to rewrite according to new direction, that would be great, but you shouldn't feel compelled to put in all that extra effort. Would be great to hear your thoughts, and if we don't hear one way or the other by the end of the week then we should close this.

litespeedmarc · 2016-09-27T04:02:01Z

We haven't heard from @vijaydwivedi75, and it is now a couple of months. @JoeMurray, do you have access to close this as you had suggested a few months ago?

monishdeb · 2016-09-27T06:36:35Z

Also the PR gone stale. Am closing this PR now

CRM-Issue-14102

cf939ee

civicrm-builder added the master label Apr 19, 2016

vijaydwivedi75 changed the title ~~CRM-Issue-14102~~ CRM-14102 Apr 20, 2016

CRM-14102

b82d6d3

totten mentioned this pull request May 6, 2016

Invitation: CiviCRM Release for June 2016 civicrm/release-management#2

Closed

Edzelopez reviewed May 18, 2016
View reviewed changes

totten changed the title ~~CRM-14102~~ CRM-14102 - UI on contribution page config Jun 11, 2016

JoeMurray mentioned this pull request Jul 7, 2016

Invitation: CiviCRM 4.7.10 release August 2016 civicrm/release-management#4

Closed

monishdeb closed this Sep 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-14102 - UI on contribution page config #8180

CRM-14102 - UI on contribution page config #8180

vijaydwivedi75 commented Apr 19, 2016

civicrm-builder commented Apr 19, 2016

eileenmcnaughton commented Apr 19, 2016

eileenmcnaughton commented Apr 19, 2016

vijaydwivedi75 commented Apr 20, 2016

eileenmcnaughton commented Apr 20, 2016

Edzelopez May 18, 2016 •

edited

Loading

eileenmcnaughton May 18, 2016

eileenmcnaughton May 18, 2016

Edzelopez May 18, 2016

eileenmcnaughton May 18, 2016

JoeMurray May 20, 2016 •

edited

Loading

JoeMurray May 20, 2016

eileenmcnaughton May 21, 2016

JoeMurray commented Jun 13, 2016

monishdeb commented Jun 20, 2016 •

edited

Loading

JoeMurray commented Jun 20, 2016

eileenmcnaughton commented Jun 20, 2016

JoeMurray commented Jun 21, 2016

litespeedmarc commented Sep 27, 2016 •

edited

Loading

monishdeb commented Sep 27, 2016

CRM-14102 - UI on contribution page config #8180

CRM-14102 - UI on contribution page config #8180

Conversation

vijaydwivedi75 commented Apr 19, 2016

civicrm-builder commented Apr 19, 2016

eileenmcnaughton commented Apr 19, 2016

eileenmcnaughton commented Apr 19, 2016

vijaydwivedi75 commented Apr 20, 2016

eileenmcnaughton commented Apr 20, 2016

Edzelopez May 18, 2016 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton May 18, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 18, 2016

Choose a reason for hiding this comment

Edzelopez May 18, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 18, 2016

Choose a reason for hiding this comment

JoeMurray May 20, 2016 • edited Loading

Choose a reason for hiding this comment

JoeMurray May 20, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 21, 2016

Choose a reason for hiding this comment

JoeMurray commented Jun 13, 2016

monishdeb commented Jun 20, 2016 • edited Loading

JoeMurray commented Jun 20, 2016

eileenmcnaughton commented Jun 20, 2016

JoeMurray commented Jun 21, 2016

litespeedmarc commented Sep 27, 2016 • edited Loading

monishdeb commented Sep 27, 2016

Edzelopez May 18, 2016 •

edited

Loading

JoeMurray May 20, 2016 •

edited

Loading

monishdeb commented Jun 20, 2016 •

edited

Loading

litespeedmarc commented Sep 27, 2016 •

edited

Loading