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#616) Fix Print/merge document for memberships: Preview creating activities; standardise preview code for contacts, cases, contributions & memberships #13369

Closed

Conversation

andrewpthompson
Copy link
Contributor

@andrewpthompson andrewpthompson commented Dec 30, 2018

Overview

See https://lab.civicrm.org/dev/core/issues/616. Print/merge document for memberships should not creates activities when the Preview button is clicked. This PR resolves that, making the behaviour for Memberships consistent with that of Contacts.

Before

Activities will be created after clicking Preview:

  1. Find Memberships, use any criteria
  2. Select some memberships and select the 'Print/merge document for memberships'
  3. Click Preview - PDF will be created
  4. Find Activities, search using appropriate criteria (Today) - activities will be seen for this task.

After

Activities will not be created after clicking Preview.

Technical Details

When CRM-16725 / PR #6888 added the Preview button, it did not implement any code in CRM_Member_Form_Task_PDFLetterCommon to make Preview work for Membership search tasks, only Contacts.

The few lines of code that this PR adds were taken from CRM_Contact_Form_Task_PDFLetterCommon::postProcess() (almost verbatim except for the button name).

To help standardise the behaviour, the logic associated with $isLiveMode in CRM_Contact_Form_Task_PDFLetterCommon::postProcess() is moved into a new function CRM_Contact_Form_Task_PDFLetterCommon::isLiveMode(). It is called from CRM_Contact_Form_Task_PDFLetterCommon::createActivities()`, which was already common to the PDF letter forms for memberships, contacts, contributions and cases.

Comments

There is duplication between the contact and membership PDF letter code but it didn't seem straightforward to remove it due to the code for each being based on contact IDs and membership IDs respectively.

@aydun has done some good work in PR #12012 for PDFs for Activities and improving the code structure. I spent a short time looking at this and suspect that it would be best to do similar for Memberships after #12012 is finalised.

Copy link
Contributor

@mattwire mattwire left a comment

Choose a reason for hiding this comment

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

Looking at the code, it looks like the bug may also be present in CRM_Contribution_Form_Task_PDFLetterCommon?

As we are checking for button names this is a source of unreliability in the future, and having that check in three places makes it much harder to resolve. Can you wrap the 4 lines (and if statement) into a separate function on CRM_Contact_Form_Task_PDFLetterCommon that can be used by all?
For example:
public static function isLiveMode($form, $html_message, $contactIDs, $formValues)

This would also make future cleanup easier to do.

CRM/Member/Form/Task/PDFLetterCommon.php Outdated Show resolved Hide resolved
@andrewpthompson andrewpthompson force-pushed the membership_pdf_preview branch 3 times, most recently from 01c8101 to f269670 Compare January 4, 2019 03:22
…letter. Move isLiveMode logic to common function.
@andrewpthompson andrewpthompson changed the title (dev/core#616) Fix for Print/merge document for memberships: Preview creating activities (dev/core#616) Fix Print/merge document for memberships: Preview creating activities; standardise preview code for contacts, cases, contributions & memberships Jan 4, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 25, 2019
I'm closing civicrm#13369 as it is stale, but
I figure it makes sense to pull out this extraction from it. I think the PR then
alters the button names taken into account but I'm not seeking to fix here, just
to do the extraction
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 25, 2019
I'm closing civicrm#13369 as it is stale, but
I figure it makes sense to pull out this extraction from it. I think the PR then
alters the button names taken into account but I'm not seeking to fix here, just
to do the extraction
@eileenmcnaughton
Copy link
Contributor

I'm closing this out because it's stale - but I figured I'd put up the extraction here #14336 & get that merged to make the actual change more readable.

Can you open a new PR for it when you are ready?

I'm also removing the 'needs test' from this PR as on looking it is very 'formy' & as it deals with button names & such like I think no test is oK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants