feat: Customizable email subject name #26327
Conversation
|
Run: superset db downgrade
Run: superset db upgrade
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -177,8 +177,7 @@ def _get_content(self) -> EmailContent: | |||
|
|
|||
| def _get_subject(self) -> str: | |||
| return __( | |||
| "%(prefix)s %(title)s", | |||
| prefix=app.config["EMAIL_REPORTS_SUBJECT_PREFIX"], | |||
There was a problem hiding this comment.
Feels like this could still be useful even with per-schedule customisation, as you might want to prefix all your emails with a standard prefix and then choose individual subject lines after the prefix. I'd continue supporting it, though we could probably improve the behaviour where prefix is None or empty by removing any leading space etc.
There was a problem hiding this comment.
I think if the user wants to have prefix then they can always add it themselves and we should allow user 100% control over their email subject.
For the default subject line it still have prefix I just modify code to this
if self._report_schedule.chart:
name = (
f"{prefix} "
f"{self._report_schedule.name}: "
f"{self._report_schedule.chart.slice_name}"
)
else:
name = (
f"{prefix} "
f"{self._report_schedule.name}: "
f"{self._report_schedule.dashboard.dashboard_title}"
)
There was a problem hiding this comment.
I agree with @giftig on the premise that the app.config is set by the application admin, whereas the title will be set by individual users. If an application admin set an app.config prefix, it's likely something important that they don't want removed from the emails, such as "Company Confidential", etc.
There was a problem hiding this comment.
Ok, I got it. I already modify to be as previous.
|
Thanks for contributing @puridach-w . I've added some minor suggestions and kicked off the build for you. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26327 +/- ##
==========================================
- Coverage 69.81% 69.81% -0.01%
==========================================
Files 1910 1910
Lines 74827 74856 +29
Branches 8346 8357 +11
==========================================
+ Hits 52243 52263 +20
- Misses 20532 20538 +6
- Partials 2052 2055 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What should I do with this? Can you suggest please. |
|
ping @eschutho and @lilykuang who are working on SIP-110. |
|
@giftig @john-bodley Could you please help me review this PR again? |
|
@puridach-w the code all looks ok to me but I'm not very familiar with the subject of reports. I think it'd be a good idea to get a review from @eschutho or @lilykuang since they're working on the related SIP John mentioned above; would be good to confirm what's done here aligns ok with that work. |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@eschutho Ephemeral environment spinning up at http://34.222.243.175:8080. Credentials are |
...t/migrations/versions/2024-01-03_16-57_61304d31c995_add_subject_column_to_report_schedule.py
Outdated
Show resolved
Hide resolved
superset/commands/report/execute.py
Outdated
| f"{self._report_schedule.name}: " | ||
| f"{self._report_schedule.dashboard.dashboard_title}" | ||
| ) | ||
| name = self._report_schedule.email_subject |
There was a problem hiding this comment.
This name variable is really just for internal logs. I'm not sure if the subject line would help here?
There was a problem hiding this comment.
If i didn't misunderstand the code that use for send alert is smtp
send_email_smtp( to, subject, content.body, app.config, files=[], data=content.data, images=content.images, bcc="", mime_subtype="related", dryrun=False, header_data=content.header_data, )
and subject it come from this
def _get_subject(self) -> str: return __( "%(title)s", title=self._content.name, )
There was a problem hiding this comment.
Thanks for pointing that out. You're correct. Maybe just a small nit for readability, I would switch the logic to
if self._report_schedule.email_subject:
name = self._report_schedule.email_subject
else:
...
```
I also left a separate comment about persisting the app.config prefix.
|
|
||
| if (name === 'name') { | ||
| updateEmailSubject(); | ||
| } |
There was a problem hiding this comment.
instead of using onInputChange here, you can just create a new event handler like this for example:
const onSubjectChange = (value: string) => {
updateEmailSubject(value);
};
There was a problem hiding this comment.
But it also needs to update the 'report name' value in the state before updating the email subject. That is the reason why I added this code. It is used to show the default subject line immediately when changing the report name. If a separate function is used, code must also be added to set the new value into the state, which will cause redundant code.
if (name === 'name') {
updateEmailSubject();
}
There was a problem hiding this comment.
Right, the pattern that the other fields are using, just to be consistent, is something like this:
const onSubjectChange = (value: string) => {
updateAlertState('email_subject', value);
};
That will add the subject key/val into currentAlert. I don't think you need to have both emailSubject in local and currentAlert.email_subject in the parent component state.
There was a problem hiding this comment.
First video:
https://github.com/apache/superset/assets/76474644/47df84ce-e7c1-4db9-b008-3b43b6c39592
From the first video, it can be observed that the Report or Dashboard name changes whenever the _default value that's shown as a placeholder changes. The need for two variables arises from the requirement to display the default Subject name, and another one to store the customized name.
Second Video:
https://github.com/apache/superset/assets/76474644/bb024b5e-96e6-4323-ada7-cef981752341
As shown in the second video, initially when I customize the "Email subject name", it changes again when I edit the report name. This is caused by using only the currentAlert.subject_name as the placeholder and value for the "Email subject name" field. Therefore, when the report name is changed, the "Email subject name" field gets updated.
Do you think it's important to display the default email subject line?
There was a problem hiding this comment.
@eschutho would you kindly provide your opinion on this matter?
There was a problem hiding this comment.
Thanks for the videos. I'll defer to a product or design person on the functionality. @yousoph do you think the default subject line should appear in the email subject field?
There was a problem hiding this comment.
Sorry for the delayed response here, but the rest is looking good to me. Thanks for all the contributions @puridach-w!
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
Outdated
Show resolved
Hide resolved
|
Ping @eschutho I have already modified the code based on your comment. |
|
Ping @eschutho Could you please help me review this PR again? |
| setEmailSubject( | ||
| `${currentAlert?.name}: ${currentAlert?.dashboard?.label || ''}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
since the default is handled in execute.py (server side) I don't think you need to also define it here in the client. It can just be left blank.
There was a problem hiding this comment.
As you can see in the image the default value will be shown if user doesn't input any text and this code is a part of that function.
There was a problem hiding this comment.
Perhaps the placeholder could show the default if empty? This would also allow for users to revert back to default if they've saved a report with a name, but want to return to the Superset default subject line.
There was a problem hiding this comment.
Ping @eschutho @yousoph, What are your thoughts on reverting to displaying the default value in the placeholder? This way, users can easily revert to the default if they accidentally delete the email_subject. For example, if we make the "email_subject" field a required field and the user deletes it, it could be harder for them to restore the default value.
|
Ping @eschutho Could you please help me review this PR again? |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@geido Ephemeral environment spinning up at http://52.34.171.63:8080. Credentials are |
|
@puridach-w not sure if this is related to your PR or not but I cannot add a report. It thinks recipients are not entered even though I did. Can you verify that, please? |
Sure let me check |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26327 +/- ##
==========================================
+ Coverage 60.48% 70.17% +9.68%
==========================================
Files 1931 1944 +13
Lines 76236 77327 +1091
Branches 8568 8668 +100
==========================================
+ Hits 46114 54264 +8150
+ Misses 28017 20938 -7079
- Partials 2105 2125 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi @geido , as I checked, I found that the issue come from the updateNotificationSetting function in the AlertReportModal file. The problem arises because the function does not update the state with the new setting value, resulting in the recipient variable doesn't have a value. I've fixed this by adding an else statement to update the setting value. |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@geido Ephemeral environment spinning up at http://52.39.176.197:8080. Credentials are |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@geido Ephemeral environment spinning up at http://34.213.161.78:8080. Credentials are |
|
Hey @puridach-w this is looking much better now. However, I noticed that you can add an empty space as a valid value for the subject. Should we probably strip it or allow for a minimum amount of chars? Wdyt? |
Hi @geido. I agree and have added the condition to trap that case. Could you please review it again? There's a failed job under 'Build & Publish Docker Images / docker-build', but I noticed there is an open PR aimed at resolving the issue. |
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@geido Ephemeral environment spinning up at http://54.218.89.202:8080. Credentials are |
geido
dismissed
eschutho’s stale review
Several iterations have been made on the PR and it looks ready now
|
Ephemeral environment shutdown and build artifacts deleted. |
Co-authored-by: Puridach Wutthihathaithamrong <>
SUMMARY
This feature allows users to customize the email subject when creating a report or an alert within Superset. This is driven by a need to include specific parameters within the email subject line, enhancing the specificity and resonance of alert or report notifications.
Changes:
This feature has been tested with Superset master branch.
This proposed feature has already been discussed in the Superset Improvement Proposal (see #25945)
BEFORE SCREENSHOTS
AFTER SCREENSHOTS
TESTING INSTRUCTIONS
To verify the changes made, please follow these steps: (Assume that there TAG for alert and report and open)
ADDITIONAL INFORMATION