CRM-21436: Fix exception on pay later contribution page.

[jitendrapurohit]

Overview

This is an extension of #11286 and fixes exception on a pay later contribution page.

Before

If no payment processor is selected, contribution page displays the following exception.

A payment processor configured for this page might be disabled (contact the site administrator for assistance).

After

Contribution page loads fine with pay later processor.

Technical Details

Assignment of pay_later was done after checking if there are any processors enabled on the contribution page. This PR assigns paylater processor just before the check so that it gets assigned correctly even if none of the payment processor is configured.

Comments

Added unit test.

---

• CRM-21436: Fatal error on contribution page with only pay later enabled.

[eileenmcnaughton]

this looks good to me - code makes sense

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

If the expectation is that an extension is thrown here, this will still pass if one is not. You can add a return in the catch box & a $this->markFail('exception was expected'); to deal with that

[jitendrapurohit]

Oh Thanks, seems I was extra optimistic here ;-)

[eileenmcnaughton]

@jitendrapurohit the failing test is related

[eileenmcnaughton]

(it might be a thing where the test works in isolation but in the test suite perhaps some data from another test interferes)

[jitendrapurohit]

Not sure whats wrong, the test passes on my local individually and also if I run the whole file. Jenkin seems to be failing because the contribution page which is in process is disabled. But is_active param on my test should make sure that it is active.

Only thing I can see is that the contribution_page table might be having some extra rows in it. I've added this table in the teardown() function. Any more pointers?

[seamuslee001]

@jitendrapurohit i have been doing some looking into this on the jenkins test box

So what I have found is that when you run ./tools/scripts/phpunit 'CRM_AllTests' which is essentially what Jenkins does, the test fails because even though $page1['id'] outputs correct id (in this case 1 as shown below) $this->_id in the contribution base is going to 4 and therefor nothing from the contribution_page table gets loaded and therefor no values are loaded and you get the exception, I replicated this on my local as well

Installing core113171_knvo7 database
PHPUnit 4.8.21 by Sebastian Bergmann and contributors.

.............................................................   61 / 1759 (  3%)
.............................................................  122 / 1759 (  6%)
.............................................................  183 / 1759 ( 10%)
.............................................................  244 / 1759 ( 13%)
.............................................................  305 / 1759 ( 17%)
.............................................................  366 / 1759 ( 20%)
..............EArray
(
    [is_error] => 0
    [undefined_fields] => Array
        (
            [0] => is_active
            [1] => title
            [2] => financial_type_id
            [3] => currency
            [4] => goal_amount
            [5] => is_pay_later
            [6] => is_monetary
            [7] => is_email_receipt
            [8] => receipt_from_email
            [9] => receipt_from_name
        )

    [version] => 3
    [count] => 1
    [id] => 1
    [values] => Array
        (
            [1] => Array
                (
                    [id] => 1
                    [title] => Test Contribution Page
                    [intro_text] =>
                    [financial_type_id] => 1
                    [payment_processor] =>
                    [is_credit_card_only] =>
                    [is_monetary] => 1
                    [is_recur] =>
                    [is_confirm_enabled] =>
                    [recur_frequency_unit] =>
                    [is_recur_interval] =>
                    [is_recur_installments] =>
                    [adjust_recur_start_date] =>
                    [is_pay_later] => 1
                    [pay_later_text] =>
                    [pay_later_receipt] =>
                    [is_partial_payment] =>
                    [initial_amount_label] =>
                    [initial_amount_help_text] =>
                    [min_initial_amount] =>
                    [is_allow_other_amount] =>
                    [default_amount_id] =>
                    [min_amount] =>
                    [max_amount] =>
                    [goal_amount] => 100
                    [thankyou_title] =>
                    [thankyou_text] =>
                    [thankyou_footer] =>
                    [is_email_receipt] => 1
                    [receipt_from_name] => Ego Freud
                    [receipt_from_email] => yourconscience@donate.com
                    [cc_receipt] =>
                    [bcc_receipt] =>
                    [receipt_text] =>
                    [is_active] => 1
                    [footer_text] =>
                    [amount_block_is_active] =>
                    [start_date] =>
                    [end_date] =>
                    [created_id] =>
                    [created_date] =>
                    [currency] => NZD
                    [campaign_id] =>
                    [is_share] =>
                    [is_billing_required] =>
                )

        )

)
14Array
(
    [custom_post_id] =>
    [custom_pre_id] =>
)
..```

[eileenmcnaughton]

maybe it would help to load the form 'Main' rather than it's parent? There is a helper 'getForm' which might set things that matter

[mlutfy]

I deployed the patch to our production testers, and so far so good.

[eileenmcnaughton]

@jitendrapurohit if you are still struggling with the test after looking again today let's split this into 2 prs - a fix that I will merge against the rc and the unit test against master. We are getting tight on the rc timeframe now & I agree this is an essential patch to be in there. (not that I don't want the test finished - just creating an option if you think it might take longer than we really want this to be pending merging)

[mlutfy]

I notice that this variable is not used in this class. Is it used anywhere else? i.e. any risk of breaking something else?

Also I notice that most protected variables start with _, but there are exceptions. Are there coding guidelines on this?

(this is mostly out of curiosity, I would not block merging)

[seamuslee001]

@mlutfy it was an oldschool way of showing something was private https://stackoverflow.com/questions/663350/whats-the-deal-with-a-leading-underscore-in-php-class-methods

[eileenmcnaughton]

Probably better to move any changes that are more clean up to a pr against master

[eileenmcnaughton]

ie. this one could be used by any class that inherits from CRM_Core_Form so it might be better on master - the one below is fine as is as it is easy by reading it to feel confident about renaming that var

[jitendrapurohit]

Thanks @seamuslee001 that is much helpful, and I'm still thinking why teardown() function wasn't able to handle that. Anyway, I've separated the unit test as mentioned by @eileenmcnaughton and have submitted it against master #11334. I can take a look later this week, as now we've steps for replicating it on my local :-).

Will also consider @mlutfy comment about the variable on the master PR.

[eileenmcnaughton]

@jitendra - if you want to pull out that very first change (the one to the name of the form property) I'll give this merge-on-pass & we can look at that line on master

[jitendrapurohit]

I thought I included that in my previous commit - done now.

[eileenmcnaughton]

I just merged it - we've seen the test results enough times to know it's only the new test failing. Will up-merge to master now. Thanks @jitendrapurohit - I think this & the one you did earlier are solid fixes that improve the robustness & logic of the code