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

Update membership offline receipt to run off WorkflowTemplate & tokens #25930

Merged
merged 2 commits into from
Apr 30, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 27, 2023

Overview

Update membership offline receipt to run off WorkflowTemplate & tokens

Before

The offline membership receipt is heavily dependent on form assigns that we are trying to deprecate (the dreaded dataArray and the overly-deep lineItems as well as an assign for every membership & contribution value.)

It was never very pretty - no change

image

After

Tokens used where possible, otherwise standardised variables.

In addition code is added to permif previewing in the Message Admin screen

Technical Details

There is no intended formatting change from this - it is part of the attempt to standardise our templates & decouple them from form logic

Comments

@demeritcowboy I am trying to sort out that online receipt - but figured maybe getting the offline one sorted first would make it a bit easier (since at least then I add in the examples) - are you able to take a look once this is passing? I'd ideally like to make changes to the membership receipts in the same release (5.61) as the membership expiration labels change

@civibot
Copy link

civibot bot commented Mar 27, 2023

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

CRM_Member_Form_MembershipRenewalTest::testSubmitRecur
Exception: CRM_Core_Exception: "Invalid token filter: ["raw"]"
#0 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/Civi/Token/TokenProcessor.php(383): Civi\Token\TokenProcessor->filterTokenValue("$ 0.00", (Array:1), Object(Civi\Token\TokenRow), "text/plain")
#1 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/Civi/Token/TokenProcessor.php(447): Civi\Token\TokenProcessor->Civi\Token{closure}("{contribution.total_amount|raw}", "contribution", "total_amount", (Array:1))
#2 internal function: Civi\Token\TokenProcessor->Civi\Token{closure}((Array:4))
#3 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/Civi/Token/TokenProcessor.php(431): preg_replace_callback(";{([\w]+).([\w:.]+)(?:|(\w+(?::[\w": %-_()[]+/#@!,.?]*)?))?};", Object(Closure), "***********************************************************\n\n{ts}Test-drive...")

@eileenmcnaughton eileenmcnaughton force-pushed the mem_receipt_offline branch 9 times, most recently from de7ddef to 22476eb Compare March 31, 2023 16:57
@agileware-justin
Copy link
Contributor

Good one @eileenmcnaughton 🥇

@eileenmcnaughton
Copy link
Contributor Author

@agileware-justin looks like it needs someone to review it to get it over the line....

@seamuslee001 once this & a couple of follow ons are merging a bunch of php8.2 test fails should fall away

@agileware-justin
Copy link
Contributor

Back from hols tomorrow

@agileware-justin
Copy link
Contributor

I should be able to test this today and let you know how it goes

@eileenmcnaughton
Copy link
Contributor Author

Thanks @agileware-justin - after this is merged I'll try to do the same to the online one - which has an extra challenge or 2 (damn you secondary contribution)

@eileenmcnaughton
Copy link
Contributor Author

@agileware-justin is this still on your list?

@agileware-justin
Copy link
Contributor

Yes and no - really need the Contribution Receipt Online message tempate as that's more in line with a current project.

My testing would be based on Action Provider and Contribution Page - more interested in seeing it work with Action Provider as that's how we tend to do these things days.

@eileenmcnaughton
Copy link
Contributor Author

@agileware-justin the changes in this PR are already made on the online contribution receipt - the membership receipts & event receipts are outstanging (including in the latter case fixing the secondary participants)

<tr>
<td {$labelStyle}>
{ts}Amount Before Tax:{/ts}
{ts}Amount Before Tax{/ts}:
Copy link
Contributor

Choose a reason for hiding this comment

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

The colon should be inside the ts like before - see https://docs.civicrm.org/dev/en/latest/translation/#include-typography-in-strings. I know there are some others in here where it comes after, but they shouldn't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks @demeritcowboy - I fixed this string across the various templates

@@ -87,6 +89,7 @@ private function getOrder(): ?CRM_Financial_BAO_Order {
* Get bool for whether a line item breakdown be displayed.
*
* @return bool
* @noinspection PhpUnused
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several of these added in this PR. Does it turn off "unused variable" IDE warnings? Do we want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy yeah - it turns off IDE warnings in cases where the IDE can't reasonably figure out how something is called (ie through listeners & stuff)

@agileware-justin
Copy link
Contributor

agileware-justin commented Apr 28, 2023

Did a quick review @eileenmcnaughton 😄 - have not applied patch and used on a test site yet tho.

<tr>
<td {$labelStyle}>
{ts}Amount Before Tax:{/ts}
{ts}Amount Before Tax:{/ts}
Copy link
Contributor

Choose a reason for hiding this comment

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

And another -> {ts}Amount before Tax:{/ts}

Copy link
Contributor

Choose a reason for hiding this comment

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

@agileware-justin See earlier comment: #25930 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@demeritcowboy apologies - I didn't read this document properly: "Typography is different in different languages and thus must be translated along with the string" https://docs.civicrm.org/dev/en/latest/translation/#include-typography-in-strings

Learnt something new and I suspect that all our extensions are not compliant with that rule.

So yes, my previous comments were wrong!

Copy link
Contributor

Choose a reason for hiding this comment

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

{ts}All Good{/ts}!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agileware-justin yeah I thought outside was better too - until @demeritcowboy pointed it out

@demeritcowboy
Copy link
Contributor

I'll give it a little run but since I don't use memberships anywhere and there's way too many variations to test this properly, I'll just focus on:

  • Does it work in a basic scenario and look roughly like before.
  • What happens if you have an older custom message template - does it still work.

@demeritcowboy
Copy link
Contributor

If you are using an older template, and you have taxes, you get an undefined var $total_amount, and all the receipt shows for monies is "Total Tax Amount $5.00"

So possibly this should be added to the list of updated message templates that display during upgrade so people know they HAVE to update.

Oddly both with the old and new template it never shows "Amount before tax". Maybe out of scope since it seemed wrong before too. (There are also several other things that seem wrong before and after.)

Also this will need to run regen eventually.

But otherwise a basic run works.

@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy - I'm thinking that issue with $total_amount would pre-date this? My preference would be to force an update on this template when the online one is also done - but if it has changed with this PR obviously it would need to be done with the merge of this PR

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy if I convert the membership page to use a price set I see the amount before tax

image

@demeritcowboy
Copy link
Contributor

Ok with a price set it does display Amount Before Tax. By the way I'm not using a contribution page since this is the offline receipt. What I'm doing:

  • Memberships - New Membership (which doesn't exist in recent versions which is confusing but you can get to the same page from an existing contact in the actions dropdown)
  • Fill it out. Leave the defaults, just picking the membership type.
  • Check the box for Send Receipt.
  • Look at the receipt.

For the total_amount I see it only happens if you are resending the receipt by editing an existing membership and checking the box, and yes it does seem to happen both before and after.

Given how many things are a bit messed up with this to begin with, I'd say let's merge it. I'll put a PR for yet another thing wrong since it's an easy fix.

@demeritcowboy demeritcowboy merged commit 5b4708f into civicrm:master Apr 30, 2023
@eileenmcnaughton eileenmcnaughton deleted the mem_receipt_offline branch April 30, 2023 22:32
@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy I think there is some copy & paste at play - althought I would argue there should be so much copy & paste that the online & offline are the same....

@agileware-justin
Copy link
Contributor

@eileenmcnaughton yes, both the offline and online message templates should be the same for contributions. Good point to raise with the product manager at the next roadmap meeting.

@eileenmcnaughton
Copy link
Contributor Author

@agileware-justin lol - I don't think that is at the 'roadmap' level - just one of a gazillion things that would be nice to do & fall into the scratch-your-itch priority list

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