[ResponseOps][Reporting] Add email cc, bcc, subject and message fields to scheduled reports email#242922
Conversation
3eb4780 to
a91dfac
Compare
|
Pinging @elastic/response-ops (Team:ResponseOps) |
a91dfac to
1427b87
Compare
| ? { | ||
| email: { | ||
| to: emailRecipients, | ||
| cc: emailCcRecipients, |
There was a problem hiding this comment.
The cc and bcc fields were already supported by the backend but not on the FE, added these too
| [http.basePath] | ||
| ); | ||
| const { defaultTimezone } = useDefaultTimezone(); | ||
| const hasCcBcc = |
There was a problem hiding this comment.
It seemed weird to be able to click the cc bcc toggle in editMode so I removed it and automatically showed the fields only if they have a value:
cc @tiamliu
There was a problem hiding this comment.
Edit: now that those are editable I reenabled the Cc Bcc button, but kept the auto-show logic in edit mode for ease of use
| ( | ||
| validateEmailAddresses: ActionsPublicPluginSetup['validateEmailAddresses'] | ||
| ): ValidationFunc<ScheduledReport, string, string> => | ||
| ): ValidationFunc<Partial<ScheduledReport>, string, string> => |
There was a problem hiding this comment.
Had to make this partial to avoid type errors with FormData being only a subset of ScheduledReport
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9885[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 0/50 tests passed. |
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9891[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 0/50 tests passed. |
cnasikas
left a comment
There was a problem hiding this comment.
I tested the whole end-to-end workflow, and it is working as expected! Are there any PM requirements where the email address, the text, and the subject cannot be changed for a user with full access? Same, for a user without the scheduled reporting feature privilege, but for the subject and the message.
There was a problem hiding this comment.
Could you please add a test in x-pack/platform/plugins/private/reporting/public/management/components/create_scheduled_report_form.test.tsx for the new functionality?
There was a problem hiding this comment.
I initially added some tests there, but I ended up writing some functional tests in disguise that took an absurd amount of time to run (I had to increase the timeout to 15 seconds to pass CI 🤯). After all this component is a thin wrapper on top of the form and I was testing well beyond its code, and since I did write some actual functional tests for the entire flyout experience in this PR, I switched to testing the actual code of this component and leave the entire flow to fn tests
...ck/platform/plugins/private/reporting/public/management/components/scheduled_report_form.tsx
Outdated
Show resolved
Hide resolved
...ck/platform/plugins/private/reporting/public/management/components/scheduled_report_form.tsx
Show resolved
Hide resolved
...ck/platform/plugins/private/reporting/public/management/components/scheduled_report_form.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Small nit in the scheduled form:
- The space between fields it's too big. I think
8pxis enough and we can also use16pxif needed. - There's no need for the help text in the
To fieldto be that long. Suggestion to change to: We'll also email the report to the addresses you specify here.
Example:
Subject and Message fields
-
Should we have a default text in the subject and message field? I know they are optional fields but I think would be a nice insentive to avoid empty subject and message all the time. cc: @nastasha-solomon
-
I would change the subject to required field, it's weird to allow it to send emails with empty subject and message. Might be tagged as spam right away.
joana-cps
left a comment
There was a problem hiding this comment.
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9892[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 0/200 tests passed. |
This is a great idea! Emails with empty subject lines and bodies could easily be ignored or end up in folk's spam folder. |
There was an extra
Agree, but I had to shorten variant that non-reporting-manager users see (they can only email to themselves so this field is disabled), hope it's fine!
About these last two points: email subject and message weren't editable before these couple of PRs and had a default hard-coded value, which we keep assigning as default values if the user doesn't set them. So we they do have default values, they're just not visible to the user right away from the UI. I used help texts and placeholders to clarify this and guide the user. We could pre-compile the two fields with the default values in the UI too, but we'd need to keep doing it in the backend regardless; idk how much sense does it make to do it everywhere.
|
Thanks again for finding this, I initially fixed it but... ... as you could tell from the external conversations on this last point, things have changed a bit and I now enabled editing of all email fields, so I reverted the fix 😂 |
…reports-email-body-form
…reports-email-body-form
Done! The previous fallback values set by the backend weren't i18nalized so I took the occasion and created translations for them shared by both server and client 🙂 I left a bit of hint below the message textarea because there wasn't anything hinting to markdown support |
| legend: 'Repeat on weekday', | ||
| options: WEEKDAY_OPTIONS, | ||
| isDisabled: readOnly, | ||
| ...(compressed ? { buttonSize: 'compressed' } : {}), |
There was a problem hiding this comment.
Form hook lib doesn't type these props objects so some of these compressed, isDisabled and readOnly props were passed the wrong way and ended up being passed to HTML elements, causing a lot of React error logs. All should be fixed now
|
|
||
| import { i18n } from '@kbn/i18n'; | ||
|
|
||
| export const SCHEDULED_REPORT_FORM_EMAIL_SUBJECT_DEFAULT_VALUE = i18n.translate( |
There was a problem hiding this comment.
These default subject and message texts were not translated in the backend so I added common versions for both FE and BE.
Read here to know more
| jest.mock('../apis/get_reporting_health'); | ||
|
|
||
| const mockValidateEmailAddresses = jest.fn().mockResolvedValue([]); | ||
| const mockValidateEmailAddresses = jest.fn().mockReturnValue([]); |
There was a problem hiding this comment.
The original fn is not async
| jest.mock('../apis/schedule_report'); | ||
|
|
||
| const mockValidateEmailAddresses = jest.fn().mockResolvedValue([]); | ||
| const mockValidateEmailAddresses = jest.fn().mockReturnValue([]); |
There was a problem hiding this comment.
The original fn is not async
| renderWithProviders(<ScheduledReportForm {...props} />); | ||
| describe('when in readonly mode', () => { | ||
| it('disables title', async () => { | ||
| renderWithProviders(<ScheduledReportForm {...defaultProps} readOnly />); |
There was a problem hiding this comment.
Moved the props to the JSX el itself so they're typed
| .then(({ body }) => | ||
| expect(body.message).toMatchInlineSnapshot( | ||
| `"[request body.notification.email.to]: could not parse array value from json input"` | ||
| expect(body.message).toEqual( |
There was a problem hiding this comment.
These somehow started failing even though the error message was exactly the same as the expected one. toEqual works though
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9934[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 44/50 tests passed. |
cnasikas
left a comment
There was a problem hiding this comment.
LGTM! Tested and it is working fine! Thanks for the extra effort with the update.
There was a problem hiding this comment.
nit: Let's add a small unit test to check that the deprecated button is hidden.
| { | ||
| defaultMessage: 'Your scheduled report is attached for you to download or share.', | ||
| description: | ||
| 'The curly braces are Mustache interpolations, not translation variables, hence the single quotes around them.', |
There was a problem hiding this comment.
nit: There are no interpolations in the message. Should we remove this?
There was a problem hiding this comment.
Oh right thanks for the heads up!
| title, | ||
| schedule: { rrule: rrule as Rrule }, | ||
| notification: { | ||
| email: sendByEmail |
There was a problem hiding this comment.
Yes, this is how ES works. Setting to null is the only option.
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9936[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 171/200 tests passed. |
…reports-email-body-form
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9943[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 174/200 tests passed. |
…reports-email-body-form
…reports-email-body-form
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#9947[❌] x-pack/platform/test/reporting_functional/reporting_and_security.config.ts: 73/100 tests passed. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
|
…s to scheduled reports email (elastic#242922) ## 📄 Summary - Adds new fields to the scheduled reports flyout form: - cc and bcc recipients fields, - subject and message with Mustache support - Makes the email fields editable for previously created schedules - Fixes some unit tests that were passing but were using a wrong format for scheduled reports sent to the form component - Adds unit tests and improve existing ones - Adds basic functional tests for scheduled reports flyout <details> <summary> ## 🧪 Verification steps </summary> ### ✅ Happy Path 1. Configure a working default email connector in your kibana.yaml, i.e. a local mailhog instance: ```yaml notifications.connectors.default.email: email xpack.actions.preconfigured: email: name: 'Default email connector' actionTypeId: '.email' config: service: other from: testsender@test.com host: 0.0.0.0 port: 1025 secure: false hasAuth: false ``` 2. Schedule an export (i.e. of a Dashboard or Discover session) 3. In the flyout, select "Send by email" 4. Add at least one recipient 5. Add a subject and message, with variable interpolations 6. Wait for the export to be scheduled 7. In the email service you configured, check that the reported email contains the expected subject and message ### ⚡️ Edge Cases Schedule an export with a non-existent variable in the email message. The resulting text should ignore the invalid variable. ### ❌ Failure Cases Schedule an export with one or more syntax errors in the template. The resulting email messages should include a debug message for the user to correct the template. </details> <details> <summary> ## 📷 Screenshots </summary> <img width="595" height="1066" alt="image" src="https://github.com/user-attachments/assets/2ffecd32-70f4-4b9d-81a1-647d733ce297" /> </details> ## ⏪ Backport rationale Not backporting since this is a new functionality ## 🔗 References Closes elastic/kibana-team#2084 ## Release Notes Adds new fields to the schedule export form email notification section: cc and bcc recipients, subject and message with Mustache templating support ## ☑️ Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] (📝 [Docs issue](elastic/docs-content#4027)) [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ - [ ] ~~This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ - [ ] (⌛ [running](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9934/steps/canvas)) [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…s to scheduled reports email (elastic#242922) ## 📄 Summary - Adds new fields to the scheduled reports flyout form: - cc and bcc recipients fields, - subject and message with Mustache support - Makes the email fields editable for previously created schedules - Fixes some unit tests that were passing but were using a wrong format for scheduled reports sent to the form component - Adds unit tests and improve existing ones - Adds basic functional tests for scheduled reports flyout <details> <summary> ## 🧪 Verification steps </summary> ### ✅ Happy Path 1. Configure a working default email connector in your kibana.yaml, i.e. a local mailhog instance: ```yaml notifications.connectors.default.email: email xpack.actions.preconfigured: email: name: 'Default email connector' actionTypeId: '.email' config: service: other from: testsender@test.com host: 0.0.0.0 port: 1025 secure: false hasAuth: false ``` 2. Schedule an export (i.e. of a Dashboard or Discover session) 3. In the flyout, select "Send by email" 4. Add at least one recipient 5. Add a subject and message, with variable interpolations 6. Wait for the export to be scheduled 7. In the email service you configured, check that the reported email contains the expected subject and message ### ⚡️ Edge Cases Schedule an export with a non-existent variable in the email message. The resulting text should ignore the invalid variable. ### ❌ Failure Cases Schedule an export with one or more syntax errors in the template. The resulting email messages should include a debug message for the user to correct the template. </details> <details> <summary> ## 📷 Screenshots </summary> <img width="595" height="1066" alt="image" src="https://github.com/user-attachments/assets/2ffecd32-70f4-4b9d-81a1-647d733ce297" /> </details> ## ⏪ Backport rationale Not backporting since this is a new functionality ## 🔗 References Closes elastic/kibana-team#2084 ## Release Notes Adds new fields to the schedule export form email notification section: cc and bcc recipients, subject and message with Mustache templating support ## ☑️ Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] (📝 [Docs issue](elastic/docs-content#4027)) [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ - [ ] ~~This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ - [ ] (⌛ [running](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9934/steps/canvas)) [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…s to scheduled reports email (elastic#242922) ## 📄 Summary - Adds new fields to the scheduled reports flyout form: - cc and bcc recipients fields, - subject and message with Mustache support - Makes the email fields editable for previously created schedules - Fixes some unit tests that were passing but were using a wrong format for scheduled reports sent to the form component - Adds unit tests and improve existing ones - Adds basic functional tests for scheduled reports flyout <details> <summary> ## 🧪 Verification steps </summary> ### ✅ Happy Path 1. Configure a working default email connector in your kibana.yaml, i.e. a local mailhog instance: ```yaml notifications.connectors.default.email: email xpack.actions.preconfigured: email: name: 'Default email connector' actionTypeId: '.email' config: service: other from: testsender@test.com host: 0.0.0.0 port: 1025 secure: false hasAuth: false ``` 2. Schedule an export (i.e. of a Dashboard or Discover session) 3. In the flyout, select "Send by email" 4. Add at least one recipient 5. Add a subject and message, with variable interpolations 6. Wait for the export to be scheduled 7. In the email service you configured, check that the reported email contains the expected subject and message ### ⚡️ Edge Cases Schedule an export with a non-existent variable in the email message. The resulting text should ignore the invalid variable. ### ❌ Failure Cases Schedule an export with one or more syntax errors in the template. The resulting email messages should include a debug message for the user to correct the template. </details> <details> <summary> ## 📷 Screenshots </summary> <img width="595" height="1066" alt="image" src="https://github.com/user-attachments/assets/2ffecd32-70f4-4b9d-81a1-647d733ce297" /> </details> ## ⏪ Backport rationale Not backporting since this is a new functionality ## 🔗 References Closes elastic/kibana-team#2084 ## Release Notes Adds new fields to the schedule export form email notification section: cc and bcc recipients, subject and message with Mustache templating support ## ☑️ Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] (📝 [Docs issue](elastic/docs-content#4027)) [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ - [ ] ~~This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ - [ ] (⌛ [running](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/9934/steps/canvas)) [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>




📄 Summary
🧪 Verification steps
✅ Happy Path
⚡️ Edge Cases
Schedule an export with a non-existent variable in the email message. The resulting text should ignore the invalid variable.
❌ Failure Cases
Schedule an export with one or more syntax errors in the template. The resulting email messages should include a debug message for the user to correct the template.
📷 Screenshots
⏪ Backport rationale
Not backporting since this is a new functionality
🔗 References
Closes https://github.com/elastic/kibana-team/issues/2084
Release Notes
Adds new fields to the schedule export form email notification section: cc and bcc recipients, subject and message with Mustache templating support
☑️ Checklist
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listThis was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. Therelease_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.