Resolve notices if first donation amount and date columns were disabled

[mfb]

Overview

Contribution and Membership Details expects some columns are present, but they could have been disabled. This results in error messages like Notice: Undefined index: first_donation_first_donation_date in CRM_Report_Form_Member_ContributionDetail->alterDisplay()

Before

PHP notices, one or two per row (could be thousands of notices)

After

No notices!

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jitendrapurohit]

Tested.

Before -

After -

No notices displayed.

On a quick skimming of the code, not sure if these assignments are actually useful or is a part of the old deprecated code? $pcid, $fDate, $fAmt don't seem to be used anywhere else in the file. Are they required variables @mfb?

[mfb]

I didn't look at what these variables are used for, but they seemed to be used:

              if ($pcid == $contactId) {
                $rows[$rowNum]['first_donation_first_donation_amount'] = $fAmt;
                $rows[$rowNum]['first_donation_first_donation_date'] = $fDate;

[jaapjansma]

Betty and I are reviewing PR's and we came across this PR and we are missing a way how to test this functional. @mfb could you provide a step by step approach.

@jitendrapurohit could you explain what you have done? The results are clear but we cannot reproduce the same test

[jitendrapurohit]

To reproduce, you can simply open Contribution and Membership detail report - https://dmaster.demo.civicrm.org/civicrm/report/instance/24?reset=1&output=criteria

From the task action dropdown => select "Export as CSV" and then reload the page. I was also able to replicate this on dmaster.

[jaapjansma]

Thanks @jitendrapurohit

[jaapjansma]

Betty and I have reviewed this PR below our report.

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
    • COMMENTS: Thanks @jitendrapurohit for the quick response.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We used the description given by @jitendrapurohit and the notices where gone when this PR was applied.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
  • Code quality (r-code)
    • ISSUE: I have added a code review about the style. To me it seems not using the CiviCRM code style. But I am happy to have this merged.
  • Maintainability (r-maint)
    • PASS: This change does not require any tests.
  • Test results (r-test)
    • PASS: The test results are all-clear.

@eileenmcnaughton or @mattwire we think this PR is good enough to be merged. However we do think there is a code style issue but that is not in the way of merging this.