-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ad-hoc email functionality #56
base: main
Are you sure you want to change the base?
Conversation
This includes current progress for ad-hoc email functionality, including: * `emails` directory, which contains functionality strictly limited to sending emails. Theoretically, this can be broken out to a new package eventually (but currently dependencies would not allow this). * `sendEmailsAction` component which accepts a `whatId` and `targetObjectIds` list of Contact ids and will display an action modal dialog to the user allowing them to compose an email to these recipients. This component uses `sendEmailsForm` component for UI. Supporting component `emailDeliveryReport` is used within this component to show partial success results for sent emails. * Supporting Apex classes for email functionality, including model classes for `Email` and `EmailAddress`. Service classes including `EmailService`, `EmailAddressService` and `EmailRecipientService`. And controllers for allowing LWC components to access this functionality, including `EmailController`. Currently `EmailService` only supports sending email using `Messaging.SingleEmailMessage`. * `sendVolunteersEmailAction` component, which can be invoked as a record level action for volunteer related records (it should work with anything which has a related volunteer assignment record). This component handles no UI itself, but retrieves the appropriate list of assigned volunteers and delegates to `sendEmailsAction` component. * `VolunteerAssignmentService` Apex serce, which will return a list of assigned volunteer ids for a given volunteer record, and `EmailVolunteersController` which exposes the service to LWC components. * Quick actions to invoke the "Email Volunteers" functionality for Volunteer Activity, Volunteer Position, and Volunteer Shift records. The page layouts for these objects have been updated to include the "Email Volunteers" action.
<value>Recipient Name</value> | ||
</labels> | ||
<labels> | ||
<fullName>emailRecipientsInvalid</fullName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After investigating this, I think what might have been happening in your testing is that if all recipients do not have an email address then it interprets this as the user not having FLS access to the field because the existing logic included the faulty assumption that if all emails were missing then it must have been because of a lack of access, not because the data simply didn't have those values populated.
I updated this logic with an explicit check for FLS access for the Contact.Email field, and now hopefully you would not see the issue again in your testing. Note that the warning of blank email addresses prior to sending does not show if the user does not have access to the Contact.Email field, so that is still a prerequisite, but now FLS access is more reliably determined.
Review commit 82d8c44 for details on what was incorrect before and what has been changed to fix the faulty logic.
} | ||
}); | ||
|
||
it('TODO: test case generated by CLI command, please fill in test logic', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few TODOs in here. Did you want to check those in or were you planning to complete them before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing tests is in the backlog as #67
<language>en_US</language> | ||
<protected>true</protected> | ||
<shortDescription>Email volunteers toast title</shortDescription> | ||
<value>Email Volunteers</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurs to me that we're tracking status on the assignments, so as a volunteer manager I would expect that only volunteers who are actively assigned to the position/activity/etc. would appear in the email list, or I would have a way to specify whether to include them, or I could at least see who was inactive and have a way to manually deselect inactive folks. I recognize the complexity of the ask, so we can punt that to a future issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a filter to the volunteer assignment service which will only return active volunteers. This change is in ff95892.
Adding a more sophisticated UI to allow users to control this behavior or select volunteers more interactively is in the feature backlog as #68. Note that it might be worth considering whether the email button should exist on the detail page for the position/activity/shift or if it should be a list action button instead, which could give users more nuanced control over which volunteers to include the email.
<span class="slds-form-element__label"> | ||
{label.emailRelatedToLabel} | ||
</span> | ||
<div class="slds-form-element__control"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be hard to add a tooltip here for some contextual guidance? Something like "To include information from the related record in the email body, you can include the field reference in the format {{{!record.MyCustomField__c}}}. If the field reference is invalid or the field value for this record is empty, the email will still send but will be missing this text." (but with the correct syntax and verified behavior ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have decided to omit this for now. As I thought about how to communicate to the user the right way to use the merge syntax, it felt like a tooltip is not adequate for accurately instructing the user how to do this. The right way to do this is via user documentation, and when we add the user documentation we should come back to this and add a tooltip with a reference to that user documentation.
Maybe more ideally would be to implement a merge field picker UI, which is in the backlog as #69.
key-field="id" | ||
hide-checkbox-column> | ||
</lightning-datatable> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also include a message if any messages were successfully sent? Maybe just a summary: "1 message was successfully sent."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, will some emails be sent if others fail? I didn't see the one I expected come through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I hadn't verified my email. (Is that a condition we can detect/warn about?) Note to self: Let's make sure to include a note about verifying email in documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added changes which will now include the number of successful emails sent in the email delivery report. These changes are included in 2d71e33. Review that commit (and commit message) for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added functionality which now checks the "verified" status of the user's own email before allowing them to choose it as an originator option. These changes are included in 841f87e. Review that commit (and commit message) for more details.
Since the Location__c and LocationAddress__c fields have been removed from the VolunteerPosition__c object, this change removes references to those fields in other metadata.
Since the default package directory was changed in sfdx-project.json to only refer to the `force-app/volunteers` directory, this was excluding changes made to metadata in the `force-app/emails` directory from being included when deploying the project using sfdx. This change adds the `force-app/emails` directory as an additional package directory so that it is included when deploying the project using sfdx, while the `force-app/volunteers` directory is maintained as the default package directory.
Previously, in the custom send email action, it was assumed that if no recipient returned in the recipient list had an address property or if all addresses were blank, then it was because the user did not have FLS access to the Contact.Email field. This assumption would then be used to determine the behavior of the UI for the user, such as not including informative messages which relate to the email address of the recipients (i.e. notifying the user when some of the recipients do not have an email address before sending the email, or including the 'Email' column in displays of recipients). However, this logic was flawed in that it falsly assumed that the user did not have FLS access to the Contact.Email field if they happened to be emailing a list of recipients where *none* of the recipients had a valid email address. To resolve this issue, now an explicit check for FLS access to the Contact.Email field is performed, rather than trying to infer access to this field based on the data returned. The result of the explicit FLS check is used to control whether the user is given any information about email addresses, including warning messages and whether the Email column appears in displays. Note that in either case, the user never was able to access the Email address of the Contact if they didn't have FLS access, because the data was never (and is still not) returned to the client. What was failing and is now fixed is that read access to that field is now more reliably determined which makes the UI more consistent.
Updating email delivery report to include the number of emails which were successfully sent. Since this information wasn't actually available, EmailController was updated to include explicitly the number of successful emails which were sent in the EmailResults return value. Additionally, an explicit boolean was added to EmailResults to indicate whether errors exist, which makes it easier to check whether any errors exist rather than awkwardly trying to count the number of keys in the errors map which is returned. Now, instead of taking as a parameter the list of errors, the email delivery report accepts the email result object (which contains the list of errors and now the number of successes). Logic which detects whether there were any errors when sending emails or show the number of emails successfully sent have been updated to use the two new properties returned as part of the EmailResults.
Users are not allowed to send emails unless the email address they are using to send the email is "verified" (which usually means the the user has clicked a link sent to that address to verify the email address belongs to them). We don't want to allow users to attempt to send emails using a non-verified address (which won't work), so this change updates the "From" address options in the send emails components to only allow the user to select from "verified" email addresses. We warn the user if there are no "verified" email addresses available that they may need to verify their email address. Note that this does mean the user does not need to have a verified email address themselves in the case where there are verified org-wide email addresses available to choose from. Additionally, this change introduces support for the Sender Name and Sender Email options (which more closely matches how the native send email action works). This is a feature which allows a user to override the name and email address used when sending emails to be different from the normal name and email values on their user record. It is important to support these features both because it is more accurate to display the actual values used when sending the email, but also because there are different rules for verified email addresses when using Sender Email-- specifically that a user's own email address does not need to be verified if they are using a Sender Email address, but that Sender Email address must be verified before it is allowed to be used. Many code changes were made to support the above changes, including the fact that the EmailAddress class now indicates the verification status of the email address represented (where this is relevant), the list of "from" address options sent to the sendEmailsAction component now filtering on this verified status, and changes to sendEmailsAction and sendEmailsForm components to remove assumptions around the "from" options always containing at least one valid value (which was the user's email address) and now these components are set up to better handle variations in the "from" options list returned from Apex.
Now, when selecting volunteers assigned to a record for the send emails action, only active volunteers are included. Note that this means the user requires access to the "Active__c" field on the relevant object in order to use the send emails functionality.
This includes current progress for ad-hoc email functionality, including:
emails
directory, which contains functionality strictly limited to sending emails. Theoretically, this can be broken out to a new package eventually (but currently dependencies would not allow this).sendEmailsAction
component which accepts awhatId
andtargetObjectIds
list of Contact ids and will display an action modal dialog to the user allowing them to compose an email to these recipients. This component usessendEmailsForm
component for UI. Supporting componentemailDeliveryReport
is used within this component to show partial success results for sent emails.Supporting Apex classes for email functionality, including model classes for
Email
andEmailAddress
. Service classes includingEmailService
,EmailAddressService
andEmailRecipientService
. And controllers for allowing LWC components to access this functionality, includingEmailController
.Currently
EmailService
only supports sending email usingMessaging.SingleEmailMessage
.sendVolunteersEmailAction
component, which can be invoked as a record level action for volunteer related records (it should work with anything which has a related volunteer assignment record). This component handles no UI itself, but retrieves the appropriate list of assigned volunteers and delegates tosendEmailsAction
component.VolunteerAssignmentService
Apex serce, which will return a list of assigned volunteer ids for a given volunteer record, andEmailVolunteersController
which exposes the service to LWC components.Quick actions to invoke the "Email Volunteers" functionality for Volunteer Activity, Volunteer Position, and Volunteer Shift records. The page layouts for these objects have been updated to include the "Email Volunteers" action.
To review changes included in this PR that were not in the old WIP PR #36 (i.e. to view what has changed in this PR from when the old PR was last approved), you can compare these branches.