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

dev/core#5335 - Only show pcp section on online receipt if there is a pcp #30628

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

demeritcowboy
Copy link
Contributor

Overview

https://lab.civicrm.org/dev/core/-/issues/5335

Before

To reproduce you can set up an online contribution page but it's easier to see the problem like so:

  1. Create a contribution in the backend for someone with an email.
  2. Make a soft credit on it.
  3. Save.
  4. On the person's contributions tab click on More at the far right and choose Send Receipt. This will use the online template.
  5. Send the email.
  6. Notice the email contains a Personal Campaign Pages section and says "Display in honor roll: No". This happens even if there are no pcp pages or config at all in the system.

After

Better.

Technical Details

The logic is backwards. See ticket.
Also when pcp_id is null the find() would pick up the first arbitrary pcp page when there is one. null doesn't mean none it means wildcard.

Comments

Slightly easier to review with whitespace hidden.

Copy link

civibot bot commented Jul 6, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Jul 6, 2024
Copy link

civibot bot commented Jul 6, 2024

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/5335

@demeritcowboy
Copy link
Contributor Author

demeritcowboy commented Jul 6, 2024

The test fail is CRM_Core_Payment_AuthorizeNetIPNTest::testIPNPaymentMembershipRecurSuccessNoLeakage Honor . not found in [big long email], but I think the test was confused. It's testing an in memory of soft credit. It shouldn't say Honor, and only had that word because it was incorrectly including the pcp section and outputting "Display In Honor Roll". It wasn't displaying "In Honor Of" anyway. It does say "In Memory Of" and is correctly testing for that.

// Only display PCP block in receipt if enabled for the PCP page
if (!empty($pcpDAO->is_honor_roll)) {
$pcpParams['pcpBlock'] = TRUE;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@demeritcowboy I am just wondering is pcpBlock assigned as FALSE by default at all anywhere? just thinking it would be good to have it always assigned to the template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

up a few lines it is set to null

@stesi561
Copy link
Contributor

stesi561 commented Aug 5, 2024

Sorry got interrupted saving so I don't close the window and loose this!

  • General standards
    • Explain
      • PASS : The goal/problem/solution have been adequately explained in the PR and Gitlab.
    • User impact
      • PASS: The change fixes a bug.
    • Documentation
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • UNREVIEWED
      • PASS:
      • ISSUE:
  • Developer standards
    • Technical impact (r-tech)
      • UNREVIEWED
      • PASS: The change preserves compatibility with existing callers/code/downstream.
      • PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
      • ISSUE: The change potentially affects compatibility, and the risks have not been sufficiently managed.
      • COMMENTS:
    • Code quality (r-code)
      • UNREVIEWED
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
      • ISSUE: Something was unclear to me.
      • ISSUE: The approach should be different.
      • COMMENTS:
    • Maintainability (r-maint)
      • UNREVIEWED
      • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
      • PASS: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
      • ISSUE: The change does not sufficiently improve test coverage.
      • COMMENTS:
    • Test results (r-test)
      • UNREVIEWED
      • PASS: The test results are all-clear.
      • PASS: The test results have failures, but these have been individually inspected and found to be irrelevant.
      • ISSUE: The test failures need to be resolved.
      • COMMENTS:

@mattwire mattwire merged commit 8399e99 into civicrm:master Aug 26, 2024
1 check passed
@demeritcowboy demeritcowboy deleted the pcp branch August 26, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants