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.

Sign up for GitHub

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

Jump to bottom

[Reporting] Fix copy issue in report notifications #123607

Merged
merged 16 commits into from
Jan 25, 2022
Merged

[Reporting] Fix copy issue in report notifications #123607

merged 16 commits into from
Jan 25, 2022

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 24, 2022 •
edited
Loading

Summary

Closes #122372

  • Updated the notifier logic to display a user friendly report name or fall back to the stored value (i.e., if it is a report type that we don't know, very uncommon, but possible)
  • Restructured constants so that job types and report types are easier to work with from a TS perspective

Screenshots (after)

Screenshot 2022-01-25 at 10 15 04

Release note

We fixed a bug that resulted in an unfriendly notification title being displayed after a report has run.

Checklist

@jloleysens jloleysens added release_note:fix (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:Reporting Services v8.1.0 v8.2.0 v8.0.1 labels Jan 24, 2022
@jloleysens jloleysens requested a review from tsullivan January 24, 2022 14:53
@jloleysens jloleysens requested review from a team as code owners January 24, 2022 14:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@jloleysens jloleysens changed the title Reporting/fix copy issue in report notifications [Reporting] Fix copy issue in report notifications Jan 24, 2022
@gchaps
Copy link
Contributor

gchaps commented Jan 25, 2022 •
edited
Loading

Screens 1 & 2

For these screens, I need to better understand why there are two actions, "pick it up" and "download report". What is the reason for the user to go to Stack Management > Reporting. Can we add a leave Stack Management out? Or, alternately, add more information about the difference between downloading now and going to Stack Management?

Suggestion 1

Title: PDF created for '[Logs] Web Traffic'
Button: Download report

Suggestion 2:

Title: PDF created for '[Logs] Web Traffic'
Description: Download it now, or get it later in Stack Management > Reporting. (and maybe not a link)
Button: Download report

Screen 3

There's a lot to parse in the title. What about moving the report title to the description? Would it help to include what the characters are, or are there too many?

Title: CSV report might have formulas
Description: The report 'Last 15 minutes of data` has characters that spreadsheet applications can interpret as formulas.
Download the report now, or get it later in Stack Management > Reporting.
Button: Download report

Screen 4

I hope that "Error:blah: is just placeholder text and not a real error. Is it possible to rewrite the description with more understandable text? Or leave out the error?

Title: Cannot create PDF report for '[Logs] Web Traffic'
Description: Go to Stack Management > Reporting for details.

tsullivan
tsullivan approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jloleysens
Copy link
Contributor Author

jloleysens commented Jan 25, 2022 •
edited
Loading

Thanks for the thorough feedback @gchaps !

I've incorporated most of your feedback except I have left the link to the stack management section for now. I think this merits a bit more discussion since it has already been released in this state for some time, but at the very least we should consider that fact that not all users who are able to generate reports are able to access stack management which means showing this link is perhaps a kind of bug in itself i was wrong on this, users that can generate reports can access stack management section. I'll open an issue for this!

I've also opted for leaving the error description as is ("Error:blah!" was my bogus error 😄 ) because it does help users in reporting issues back to us - even when they don't have access to the management section.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
reporting 81 83 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
reporting 37.8KB 38.5KB +712.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit 8a8a12a into elastic:main Jan 25, 2022
@jloleysens jloleysens deleted the reporting/fix-copy-issue-in-report-notifications branch January 25, 2022 10:48
@jloleysens jloleysens removed the v8.0.1 label Jan 25, 2022
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 27, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

7 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

4 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 123607 or prevent reminders by adding the backport:skip label.

@tsullivan tsullivan added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. v8.1.0 v8.0.1 labels Feb 15, 2022
@kibanamachine kibanamachine mentioned this pull request Apr 28, 2022
Merged
3 tasks
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 29, 2022
@jloleysens
[Reporting] Fix copy issue in report notifications (elastic#123607)
ef347e0 
* refactor constants to better group report types separately

* format

* update comment

* split report type and job type constants into separate files

* added prettyReportTypeName getter to Job

* use pretty name if it exists, otherwise fallback to whatever is in jobtype

* slight restructure to existing copy to make it sound more natural

* added basic test for pretty names

* another update to existing copy, this time for the warning notification

* slight update to make test clearer

* fixed i18n strings for error notification and refactored success for consistency

* removed previous copy

* updated snapshots

* remove old i18n

* incorporoated copy feedback

* updated snapshots

(cherry picked from commit 8a8a12a)

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
@jloleysens jloleysens mentioned this pull request Apr 29, 2022
Merged
jloleysens added a commit that referenced this pull request May 4, 2022
@jloleysens @kibanamachine
[8.2] [Reporting] Fix copy issue in report notifications (#123607) (#…
d62b676 
…131222)

* [Reporting] Fix copy issue in report notifications (#123607)

* refactor constants to better group report types separately

* format

* update comment

* split report type and job type constants into separate files

* added prettyReportTypeName getter to Job

* use pretty name if it exists, otherwise fallback to whatever is in jobtype

* slight restructure to existing copy to make it sound more natural

* added basic test for pretty names

* another update to existing copy, this time for the warning notification

* slight update to make test clearer

* fixed i18n strings for error notification and refactored success for consistency

* removed previous copy

* updated snapshots

* remove old i18n

* incorporoated copy feedback

* updated snapshots

(cherry picked from commit 8a8a12a)

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json

* resolve merge conflicts

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsullivan tsullivan tsullivan approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:fix v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kibana displays V2 in pdf/png reporting notifications
6 participants
@jloleysens @elasticmachine @gchaps @kibana-ci @kibanamachine @tsullivan