Offline Membership creation and renewal does not attach Invoice to emails

[shaneonabike]

Overview

It took me a while and some serious debugging to figure out what was going on here, but I finally got it.

When Memberships are renewed offline (and created offline) no Invoice is actually attached. The culprit is

 $isAttachPDFInvoice = !empty($params['isEmailPdf']) && !empty($params['contributionId']);

Ref here

This was introduced (checking the contributionId) to prevent empty invoices via this merge in #24065. When I add the contributionId then things are happy again. We only noticed this problem after upgrading a client from 5.65 => 5.73.

I verified that this is also the case on New Membership (offline) since the Contribution ID is not attached to the params sent either. There might be other places??

Before

No attachment (invoice.pdf) was added to the outgoing email sent for offline Memberships or Membership Renewals.

To reproduce:

1. Create a membership and assign that to a Regular user to expire this year
2. Go to the contact record of that user and click on Memberships
3. From the three dot menu on the Membership select Renew
   
4. Fill out the details to renew the Membership for renewal
5. Check Send Confirmation and Receipt and choose an appropriate Receipt From
   
6. Click Renew
7. You will receive the main main email but no attachment named Invoice.pdf

After

Attachments are not being properly generated and attached.

Repeat the steps above and this time around you will receive the email template for Membership (offilne) renewal and the Invoice.pdf attachment.

Technical Details

It might be better to actually check if the value exists in the modelProps array instead, but I don't know if every call to sendTemplates actually includes this array. It definitely would reduce duplicating data being sent and keep things cleaner.

       'modelProps' => [
          'userEnteredText' => $this->getSubmittedValue('receipt_text'),
          'contactID' => $this->_receiptContactId,
          'contributionID' => $this->getContributionID(),
          'membershipID' => $this->getMembershipID(),
        ],

[civibot]

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

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[eileenmcnaughton]

Thanks @shaneonabike - I agree it would be cleaner to check modelProps - we could transition it like this - in sendTemplate

    $contributionID = $params['modelProps']['contributionID'] ?? NULL;
    if (!empty($params['contributionId']) && !$contributionID) {
      CRM_Core_Error::deprecatedWarning('contribution ID should be set in modelProps rather than params');
      $contributionID = $params['contributionId'];
    }
    $isAttachPDFInvoice = !empty($params['isEmailPdf']) && $contributionID;

[shaneonabike]

@eileenmcnaughton that was almost all the way there. I caught one missing element where the contribution Id was being passed to render the PDF so I changed that to the actual variable setup at the start by your suggestion.
It's working on our live site!

[eileenmcnaughton]

@shaneonabike cool - let's see if the deprecation notice pops up in the test run - otherwise we can merge as is

[eileenmcnaughton]

There are 3 test fails - I'm re-running 2 to check but the style issue is definitely a thing

civilint: civicrm-core#32036

Executed circa 2025-02-10 18:33 -08:00

===========================[ Identify Files ]===========================
PHP Files:

• CRM/Core/BAO/MessageTemplate.php
  ===============================[ php -l ]===============================
  ===============================[ phpcs ]===============================

FILE: .../buildkit/build/build-5/web/src/CRM/Core/BAO/MessageTemplate.php

FOUND 2 ERRORS AFFECTING 2 LINES

421 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
| | found 5
424 | ERROR | [x] Closing brace indented incorrectly; expected 5
| | spaces, found 4

PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Time: 78ms; Memory: 10MB