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#2895 handle case ids passed via url #21801

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#2895 handle case ids passed via url

Before

After

Technical Details

Comments

@demeritcowboy does this work?

@civibot
Copy link

civibot bot commented Oct 12, 2021

(Standard links)

@civibot civibot bot added the 5.43 label Oct 12, 2021
protected function getRowsForEmails(): array {
$formattedContactDetails = [];
$contactIDSFromUrl = CRM_Utils_Request::retrieve('cid', 'CommaSeparatedIntegers', $this);
$caseIDs = explode(CRM_Utils_Request::retrieve('caseid', 'String', $this), '');
Copy link
Contributor

Choose a reason for hiding this comment

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

This explode statement looks wrong but in any case (pun) I don't think it's worth trying to support multiple case ids in the url as per https://lab.civicrm.org/dev/core/-/issues/2463 and that other PR a week or so ago. It hasn't worked properly since at least 5.22 and after https://lab.civicrm.org/dev/core/-/issues/1750 it stopped working (by the same shop that introduced it).

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 OK - if I can ignore that it gets easy

Related issue - I was thinking about removing the call to caseTokens in favour of using the payment processor token list

downside - doesn't filter custom fields by case type
upside - will show any tokens registered via token processor implementations & is more standardised

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't personally use it. I mean custom fields should be listed in the dropdown (as opposed to removing all custom fields from the dropdown), but I don't have a strong opinion on whether it should be filtered by case type. I'd maybe lean towards that it should stay filtered, and also if you're making a generic template you would either need to not use custom fields or create some custom tokens, so having the full list in the dropdown wouldn't necessarily help. It looks like it was part of some paid work for PDFs? https://issues.civicrm.org/jira/browse/CRM-17606

$caseID = (count($caseIDs) === 1) ? $caseIDs[0] : NULL;

if (!empty($contactIDSFromUrl) && !$caseID) {
$contactIDS = explode($contactIDSFromUrl, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

explode again

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy so I think you are saying all we need to do is this?

@demeritcowboy
Copy link
Contributor

Thanks this should work for 5.43 just it gets the count of sent emails wrong in the status popup but I think is mergeable. I'll see if I can resurrect that test I closed - I probably just need to set the url param.

@demeritcowboy demeritcowboy merged commit 9f9a016 into civicrm:5.43 Oct 13, 2021
@eileenmcnaughton eileenmcnaughton deleted the case1 branch October 13, 2021 00:05
@eileenmcnaughton
Copy link
Contributor Author

phew - getting there!

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