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#1721 send email to case follow-up activity assignees #17116

Merged
merged 1 commit into from
May 14, 2020

Conversation

lcdservices
Copy link
Contributor

Overview

When creating a case activity follow-up activity with an assignee, the assignee is not sent an email.

Before

Follow-up assignee does not receive an email.

After

Assignee should receive an email

Technical Details

Assignee email processing is simply missing. This mostly follows the pattern found in CRM_Activity_Form_Activity::processActivity() -- with adaptation to the case context.

@civibot
Copy link

civibot bot commented Apr 20, 2020

(Standard links)

@civibot civibot bot added the master label Apr 20, 2020
@demeritcowboy
Copy link
Contributor

Thanks for the PR. This is similar to #16800, so I have the same comments as there, but it's non-blocking.

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
      Some people might start getting emails who didn't before, but should be ok.
    • [r-doc] PASS
    • [r-run] PASS
      • Confirmed problem exists before. Works after.
  • Developer standards
    • [r-tech] Comment
      The same comment I made on Respect 'Donot notify activity type' setting #16800 that this should be fine for short-term but a better long-term way would be to move it out of the form to a BAO or somewhere central so that it can be consolidated with the regular activity form and then also have the api (and hence webform) use it too when activities are created.
    • [r-code] PASS
      • It ends up not mattering but line 660 should probably be $followupStatus.
    • [r-maint] Undecided
      Also copy/pasting my same comment from the other PR: It's hard to write a test for this but as noted in r-tech I think it should be moved out of the forms, and then that would also make it easier to write a test.
    • [r-test] PASS

@eileenmcnaughton
Copy link
Contributor

I'm going to merge this - @demeritcowboy's concerns about mtce are valid. However, I read the review as it does fix a bug & since it's been 23 days since the review I feel I should make a call based on what has been done

@eileenmcnaughton eileenmcnaughton merged commit da80ef3 into civicrm:master May 14, 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