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

CRM-14102 - UI on contribution page config #8180

Closed
wants to merge 2 commits into from
Closed

CRM-14102 - UI on contribution page config #8180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf939ee
CRM-Issue-14102
vijaydwivedi75 Apr 19, 2016
b82d6d3
CRM-14102
vijaydwivedi75 Apr 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CRM/Contribute/Form/ContributionBase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ public function preProcess() {
$isPayLater = CRM_Utils_Array::value('is_pay_later', $this->_values);

if ($isMonetary &&
(!$isPayLater || !empty($this->_values['payment_processor']))
(!$isPayLater && !empty($this->_values['payment_processor']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we have started to treat $payLater as a payment processor (the Manual class) in some flows - so I would want to be very careful about this change

Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is that it should have been valid to have $isMonetary==TRUE && $isPayLater==TRUE && empty($this->_values['payment_processor']) up until now, and that going forward, $isPayLater==TRUE should ensure that $this->_value['payment_processor'] holds the new Manual process class. It seems like you now want this part of the code to make $isPayLater==TRUE to cause $this->_value['payment_processor'] to be filled. Is that what you are aiming at here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we are populating $this->_value['payment_processor'] for pay_later - & using the manual processor to provide pieces of logic rather than lots of if ($payLater) THEN type logic.

I introduced that in 4.7 to deal with some specific issues and because I feel that as a direction it provides potential for more simplicity

Copy link
Member

Choose a reason for hiding this comment

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

The issue with 'Execute real time monetary transactions' = FALSE and pay_later= TRUE setting where no membership and related contribution was created earlier, was fixed in https://github.com/civicrm/civicrm-core/pull/8575/files#diff-99d052aa6a966c5a2cc89d05f057d22dR234.

) {
$this->_paymentProcessorIDs = explode(
CRM_Core_DAO::VALUE_SEPARATOR,
Expand Down
3 changes: 1 addition & 2 deletions CRM/Contribute/Form/ContributionPage/Amount.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ public function buildQuickForm() {

$this->addElement('checkbox', 'amount_block_is_active', ts('Contribution Amounts section enabled'), NULL, array('onclick' => "showHideAmountBlock( this, 'amount_block_is_active' );"));

$this->addElement('checkbox', 'is_monetary', ts('Execute real-time monetary transactions'));

$paymentProcessor = CRM_Core_PseudoConstant::paymentProcessor();
$recurringPaymentProcessor = array();

Expand Down Expand Up @@ -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'));
Copy link
Contributor

@Edzelopez Edzelopez May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JoeMurray JoeMurray May 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
if (empty($fields['pay_later_receipt'])) {
$errors['pay_later_receipt'] = ts('Please enter the instructions to be sent to the contributor when they choose to \'pay later\'.');
Expand Down