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

[Ref] intial testing on case tokens, make knownTokens optional #21289

Merged
merged 1 commit into from
Aug 29, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 27, 2021

Overview

[Ref] intial testing on case tokens, make knownTokens optional

Before

$knownTokens is required, in some cases this makes the calling functions unnecessarily complex

After

$knownTokens is optional, test cover

Technical Details

This is a refactoring baby-step

There is a perception in the token code that passing around knownTokens is more performant

This may or may not be true - but since the goal is to migrate the code to the
token processor making the code simpler is
a higher priority as any performance change will change again.

This makes knownTokens optional in the replaceCaseTokens and adds test cover.
It is a step towards simplification in the PdfLetter code
which could be simplified if not needing to pass in knowntokens here

Comments

@civibot
Copy link

civibot bot commented Aug 27, 2021

(Standard links)

@civibot civibot bot added the master label Aug 27, 2021
There is a perception in the token code that passing around knownTokens is more performant

This may or may not be true - but since the goal is to migrate the code to the
token processor making the code simpler is
a higher priority as any performance change will change again.

This makes knownTokens optional in the replaceCaseTokens and adds test cover.
It is a step towards simplification in the PdfLetter code
which could be simplified if not needing to pass in knowntokens here
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @colemanw @demeritcowboy can I get a merge on this - mostly just a test - should be an easy merge

July 23rd, 2021
July 26th, 2021
case details
Ongoing
Copy link
Contributor

Choose a reason for hiding this comment

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

Case status "ongoing" when it has an end date is a little bit unrealistic, but technically possible. Otherwise I think this PR is ok.

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 I just 'grabbed' 'status_id' => 1, - should it change or should we merge this & ignore

Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems unlikely to be used in a way that would enforce that cases with status ongoing should have an end date (or vice-versa). So I'd say it's inconsistent but not "dangerous".

Also noticing that case.contact_id and case.client_id are valid tokens although don't seem to appear in the dropdown on a form. I'm not sure where the dropdown list comes from but I guess that dropdown would be the authority.

@demeritcowboy demeritcowboy merged commit 246f96f into civicrm:master Aug 29, 2021
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.

2 participants