[Reporting] Replace Job Completion Notifier as NP Plugin #47283
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsullivan
force-pushed
the
reporting/np-job-completion-notifier
branch
from
b0000e9 to
a2990a6
Compare
tsullivan
changed the title
tsullivan
force-pushed
the
reporting/np-job-completion-notifier
branch
3 times, most recently
from
39d29ea to
0894f7c
Compare
tsullivan
added
v7.5.0
v8.0.0
Feature:NP Migration
zDeprecated Feature:Reporting
Contributor
|
Pinging @elastic/kibana-stack-services (Team:Stack Services) |
tsullivan
force-pushed
the
reporting/np-job-completion-notifier
branch
from
9effb88 to
a388775
Compare
tsullivan
force-pushed
the
reporting/np-job-completion-notifier
branch
from
a388775 to
12ef096
Compare
tsullivan
force-pushed
the
reporting/np-job-completion-notifier
branch
from
12ef096 to
366408d
Compare
Contributor
💚 Build Succeeded
|
Contributor
💚 Build Succeeded |
tsullivan
commented
Oct 8, 2019
Contributor
💚 Build Succeeded |
tsullivan
commented
Oct 9, 2019
| method: 'GET', | ||
| config: getRouteConfig(), | ||
| handler: (request: Request) => { | ||
| // @ts-ignore |
Member
Author
There was a problem hiding this comment.
Unrelated cleanup of TS
Contributor
💚 Build Succeeded |
Contributor
💔 Build Failed |
bmcconaghy
approved these changes
Oct 16, 2019
Contributor
bmcconaghy
left a comment
bmcconaghy
left a comment
There was a problem hiding this comment.
Code LGTM. I would recommend getting someone from @elastic/kibana-platform to review this too.
joshdover
reviewed
Oct 16, 2019
|
|
||
| public setup(core: CoreSetup) {} | ||
|
|
||
| // FIXME: only perform these actions for authenticated routes |
Contributor
There was a problem hiding this comment.
How does this behave now? Will it throw errors when unauthenticated?
Contributor
💚 Build Succeeded |
joshdover
reviewed
Oct 17, 2019
Contributor
joshdover
left a comment
joshdover
left a comment
There was a problem hiding this comment.
Should the reporting_notifier be a separate plugin than reporting? It seems to me we'd want xpack.reporting.enabled=false config to also disable this piece. Other than that, LGTM
Contributor
💔 Build Failed |
Contributor
💚 Build Succeeded |
Contributor
💚 Build Succeeded |
tsullivan
added a commit
to tsullivan/kibana
that referenced
this pull request
Oct 18, 2019
* [Reporting] Replace Job Completion Notifier as NP Plugin * beautiful toasts * unit test * showNotifications returns observable * fix kibana.json * depends on links * no prettier ignore * Update content per feedback * Remove unnecessary wrapper * remove type annos and condense * use an observable of the stop method * rename the new platform plugin to match legacy * fix i18n config * additional plugin rename * i18n strings prefix change * try something with x-pack/.i18nrc.json * remove a few more notifier phrasing from plugin definition
tsullivan
added a commit
that referenced
this pull request
Oct 21, 2019
…8703) * [Reporting] Replace Job Completion Notifier as NP Plugin * beautiful toasts * unit test * showNotifications returns observable * fix kibana.json * depends on links * no prettier ignore * Update content per feedback * Remove unnecessary wrapper * remove type annos and condense * use an observable of the stop method * rename the new platform plugin to match legacy * fix i18n config * additional plugin rename * i18n strings prefix change * try something with x-pack/.i18nrc.json * remove a few more notifier phrasing from plugin definition
tsullivan
commented
Oct 31, 2019
|
|
||
| public setup(core: CoreSetup) {} | ||
|
|
||
| // FIXME: only perform these actions for authenticated routes |
Member
Author
There was a problem hiding this comment.
Follow up note: this can be possibly be achieved better by registering the job completion notifier as a component of something global for authenticated screens in Kibana, such as the nav header. This is what the Newsfeed Service does, and it is effectively prevented from fetching for news on the Login page due to there being no Nav Header on the Login page.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #45427
This PR replaces the Reporting Job Completion Notifier hack with New Platform plugin to run on the background of every page.
[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibility[ ] License checking[ ] Only run the poller on authenticated pages