[ResponseOps][Reporting] Scheduled report flyout UI#222135
[ResponseOps][Reporting] Scheduled report flyout UI#222135umbopepato merged 30 commits intoelastic:scheduled-reports-uifrom
Conversation
4dd68c2 to
0d3db10
Compare
f11a696 to
a296181
Compare
a296181 to
51e811c
Compare
d5b5d9a to
8abc15d
Compare
51e811c to
4d63748
Compare
|
Pinging @elastic/response-ops (Team:ResponseOps) |
ymao1
left a comment
There was a problem hiding this comment.
Comments from verification:
License
- running with a
basiclicense, I can see theSchedule Reportsbutton in Discover. scheduled reporting is not available inbasiceven though single CSV exports is. We should hide the button - same comment about
basiclicense andSchedule Reportbutton in Lens. We allow CSV exports through lens but should not allow scheduling reports - export button for Dashboard correctly hidden for
basiclicense 👍
Email notifications
- email option correctly disabled when notifications are not properly configured 👍
- email only allows setting
tooption. do we want to allow settingccandbccaddresses? - if I click
Schedulewithout filling in an email address, I get a validation error in thetofield and an error toast is shown. The schedule button is disabled and shows a loading spinner. Filling out thetofield does not reenable the schedule button.
Security or API keys turned off
- when ES security is false or API keys are disabled, we show the
Schedule exportbutton but open a flyout that just shows an error. TheSchedulebutton on the flyout is still enabled and an error toast shows up if you click it. Do we want to just show a disabledSchedule exportbutton in the dropdown and skip opening the flyout altogether?
Scheduling
- seems to me that the
nowdate is set when the flyout is opened, not when the request is made, so let's say you open the flyout at 8:00, mess around with the options and don't click the schedule button until 8:03. The schedule will be set to recur (daily/weekly/monthly) at 8:00. - could we show the HH:MM time when scheduling? right now it says
Repeats every Wednesday. Could we have it sayRepeats every Wednesday at 08:00?
|
Thanks for taking a look Ying! I still haven't had time to update the description and add comments but itm about these points: License
I thought the Email notifications
I know the
My bad! I removed an intermediate component level last minute to remove a hack to adapt to the new share API and missed a Security or API keys turned off
Scheduling
This is something we discussed in a meeting last Friday with @tiamliu. If I remember correctly the point was not confusing the user with a preview that doesn't correspond to the actual time used for scheduling (I have to pass a start date right away to the recurrence form, and all suggestions will be based on that date, but then the real schedule would be based on a different time - when executing the request - that the user doesn't have visibility on). I know it's not ideal and I'm open to all possibilities.
|
4b7a882 to
623c801
Compare
8abc15d to
4805aee
Compare
| customFrequency?: RecurrenceFrequency; | ||
| byweekday?: Record<string, boolean>; | ||
| bymonth?: string; | ||
| bymonthweekday?: string; |
There was a problem hiding this comment.
These are needed to be able to view previously created scheduled reports since they (still) don't have a start date, which was used to compute all this information
| * A collapsible version of EuiDescribedFormGroup. Use the `narrow` prop | ||
| * to obtain a vertical layout suitable for smaller forms | ||
| */ | ||
| export const ResponsiveFormGroup = ({ |
There was a problem hiding this comment.
EuiDescribedFormGroup seemed the perfect component for the groups with titles, but it doesn't have a column version, so I added one here. If we ever need to expand this form in a standalone page, we could use narrow = false and get an expanded UX
There was a problem hiding this comment.
FWIW I did it this way
, which should be responsive by defaultThere was a problem hiding this comment.
Ah that's nice! We could propose Eui to integrate your solution in the original one 🙂
| onClose, | ||
| }: ScheduledReportFlyoutProps) => { | ||
| return ( | ||
| <EuiFlyout size="m" maxWidth={500} paddingSize="l" ownFocus={true} onClose={onClose}> |
There was a problem hiding this comment.
We need this layer because the export menu controls the rendering of the outer flyout layer, but we have to open it too in the Reports page
| iconType="error" | ||
| color="danger" | ||
| > | ||
| <p>{i18n.UNMET_REPORTING_PREREQUISITES_MESSAGE}</p> |
There was a problem hiding this comment.
could we add more info for easier debug?
There was a problem hiding this comment.
This was a temporary text, but I moved the prerequisites check before registering the schedule export button altogether so you shouldn't be able to open it and see this error at all. I kept it just for extra safety, I'll ask docs to have a look and suggest a better message
| onClose(); | ||
| } | ||
| } catch (e) { | ||
| // Keep the flyout open in case of schedule error |
There was a problem hiding this comment.
should we also console.error(e) to make it easier to debug?
| [JOB_STATUS.WARNINGS, JOB_STATUS.FAILED].some((status) => job.status === status) | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I see no tests for these new functions, but they seem complex enough to have some
| import { getInvalidEmailAddress, getNotAllowedEmailAddress } from '../translations'; | ||
| import type { ScheduledReport } from '../../types'; | ||
|
|
||
| export const getEmailsValidator = |
There was a problem hiding this comment.
we also might need to test this
| ); | ||
| } | ||
|
|
||
| import('./management/integrations/scheduled_report_share_integration').then( |
There was a problem hiding this comment.
what's the reason for this being imported dynamically? just curious
There was a problem hiding this comment.
When imported statically it broke the page load bundle size limit for reporting, see
https://buildkite.com/elastic/kibana-pull-request/builds/309752/summary/annotations?jid=01978228-643a-4d7d-810e-4cd9e2596513
Yes, it was a decision made in the first design review to keep it simple in the first version. |
| if (!license) { | ||
| return false; | ||
| } | ||
| return license.type !== 'basic'; |
There was a problem hiding this comment.
We added an allowlist for licenses here: f5f9d9d Not sure if you can reuse.
There was a problem hiding this comment.
I couldn't import the list from there since it's marked as server code so I moved it to the common folder, hope it's fine 🙂
I have a PR for this here (the CSV part at least) |
Co-authored-by: Eyo O. Eyo <7893459+eokoneyo@users.noreply.github.com>
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
|









Summary
Important
This PR is targeting the
scheduled-reports-uifeature branch, where the backend changes from thescheduled-reportsbranch are temporarily integrated while waiting for #221028 to be merged (see the squashed[TMP] ...commit message).Implements the flyout UI for the creation and read-only viewing of scheduled reports (exports).
Known issues
Console error due to

compressedattribute wrongly forwarded by form-hook-lib to DOM element (this is likely a form lib issue):Email validation errors accumulate instead of replacing the previous one (again looks like a fom lib issue):
https://github.com/user-attachments/assets/f2dc7a46-a3a9-465d-b8a1-3187b200f9b9
Screenshots
Health API error:

Health API loading state:

Health API success with some missing prerequisites:

Form validation:

Success toast:

Failure toast:

Print format toggle:

Missing notifications email connector callout:

References
Closes #216321
Stacked on #220535