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

CRM-21264 - Fix tabular formatting of contributions in merged letters grouped by contact #11820

Merged
merged 3 commits into from
Mar 19, 2018

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Mar 15, 2018

Overview

This change makes it easier for users to generate PDF thank-you letters for contributions that are grouped by contact with details of multiple contributions in one page, displayed in table cells

Before

PDFs generated with these repro steps didn't display contribution fields within table cells.

image

After

The table cells in the PDF display as expected.

image

Technical Details

  • Tests added which fail before the fix is made and pass afterwards
  • More notes about regex in commit message

Comments

Test currently fails on "simplest TRUE" case added, which demonstrates
the bug described in CRM-21264. Also added other test cases which
current fail as well.
* Improve docblock
* Get rid of "dontCare" since that argument is optional as of PHP 5.4
  http://php.net/manual/en/function.preg-match-all.php#refsect1-function.preg-match-all-changelog
* Move regex to its own line to improve readability
* Escape dot in token since it will be used in a regex (for good
  measure)
* Add `(?![\w-])` to ensure that `td` is not followed by other
  characters that would make it a different element.
* Instead of the first `.+?`, use `((?!</td>).)*` which matches zero or
  more characters but refuses to consume if it encounters `</td>`
* Escape curly braces (for good measure)
* Instead of the second `.+?`, use `.*?` so that it will match if there
  are zero characters. (Note: this change is really the crux of the
  issue, as described in CRM-21264).
* Complete then closing `</td>` (for good measure)
@eileenmcnaughton
Copy link
Contributor

I replicated this & agree this fixes it. I didn't fully work through the regex because I was happy with the UI results & the unit test. This PR is multiple commits but each commit stands alone as a discrete change & is clearly explained in the commit message

@eileenmcnaughton eileenmcnaughton merged commit 1142ce8 into civicrm:master Mar 19, 2018
@eileenmcnaughton
Copy link
Contributor

Nice work @seanmadsen - up to your usual high standard of explanation & documentation!

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

Successfully merging this pull request may close these issues.

3 participants