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

Add email link in case summary #16959

Merged
merged 1 commit into from
Apr 8, 2020
Merged

Conversation

ray-wright
Copy link
Contributor

Overview

Currently, to email a case client it takes several clicks, I'm proposing a quicker (one-click) method to email the case client.

Before

The two options to email a client are:

  • Scroll down, open the roles section, locate the client relationship and click the email icon. Email pop-up screen is launched with the primary email address filled in.
  • Select 'email action from add activity drop-down, type in client's name in the To field, select desired email address
    image

After

Click the email icon next to the client's name at the top of the screen. Email pop up screen is prefilled with the primary email address.
image

Comments

I recognize that the action is the exact same as if you had opened the roles section and clicked the email icon. But in my opinion (and because we have open custom data on our cases) that is a bit lengthy of a process. If we are already providing the phone in the summary section if felt natural to include an email link as well.

@civibot
Copy link

civibot bot commented Apr 2, 2020

(Standard links)

@civibot civibot bot added the master label Apr 2, 2020
@eileenmcnaughton
Copy link
Contributor

Seems simple & sensible - @demeritcowboy if you are happy with this I will merge

@demeritcowboy
Copy link
Contributor

Hi yes I was going to take a look. Using   seems wrong but I'm guessing it's copy/paste from somewhere. Will take a look.

@demeritcowboy
Copy link
Contributor

So I know this seems like the simplest PR, and conceptually it is and I think it's a fine idea, just there's a couple technical issues.

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
  • Developer standards
    • [r-tech] PASS
    • [r-code] Issue
      • Regarding the title attribute:
        • The email needs to be escaped for security reasons.
        • You don't want to have the email address itself as part of the translated string.
        • So it should be title="{ts 1=$client.email|escape}Email: %1{/ts}".
      • &nbsp; is almost never used anymore because it's difficult to override/theme properly. There's probably different opinions on what to use instead, but here because I think it looks fine even without the space, I personally would add a span around the <a> like <span class="crm-case-client-email">..<a blah blah>..</a><i blah blah>..</i></span> and then if people think it's not "spacey" enough then they can add their own css override.
      • While not a blocker, if it's going to be done for single clients then there's also multi-client in the code block just above. There's probably going to be different opinions on how to make that look "nice", so could be left for a future PR if someone wants to do that.
    • [r-maint] N/A
    • [r-test] PASS

Otherwise looks good!

@ray-wright
Copy link
Contributor Author

Thank you @demeritcowboy for the detailed explanation, very helpful for me. I've revised the PR based on your suggestions. Since we don't use multi-client, I scrolled right past that section of code. I don't mind writing a separate PR for multi clients, but since that section is already formatted differently I would like to find (and this may be you) someone to run the overall formatting by.

@demeritcowboy
Copy link
Contributor

jenkins retest this please

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Apr 3, 2020

Thanks this looks good EXCEPT there's one thing I missed yesterday. Using a hardcoded "3" for the activity type will work almost all the time but it should be looked up. I don't see any variables in the template already that you could use, but what you can do is:

{crmAPI var='email_type_id' entity='OptionValue' action='getsingle' return="value" name="Email" option_group_id="activity_type"}

and then use it in the url as atype=`$email_type_id.value` instead of atype=3

Also are you set up with git now so you're able to squash/rebase commits?

@ray-wright
Copy link
Contributor Author

I'm not quite set up with git that I can squash/rebase commits. I'm am working on it (and learning the workflow better) but at the moment I am still working directly in the git hub interface when submitting PRs. After I get this last part correct, I can submit a fresh PR - not the easiest way to go about it but it'd be clean at least.

@demeritcowboy
Copy link
Contributor

Ok I can send you some git steps in chat, since I can see an ampersand went missing between "add" and "atype", so one more round!

@demeritcowboy
Copy link
Contributor

This looks good now. I think it's ok to merge.

@eileenmcnaughton
Copy link
Contributor

Good work!

@eileenmcnaughton eileenmcnaughton merged commit 8df5988 into civicrm:master Apr 8, 2020
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