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.

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

[Newsfeed] UI plugin for Kibana #49579

Merged
merged 88 commits into from
Nov 13, 2019
Merged

[Newsfeed] UI plugin for Kibana #49579

merged 88 commits into from
Nov 13, 2019

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Oct 28, 2019

Summary

Newsfeed system for the notification center #49539

Screen Shot 2019-10-31 at 11 52 13 AM

2019-10-31 11 51 40

Release note:

Added a Newsfeed feature to the main navigation bar of Kibana. Now you can stay up-to-date on Elastic news, such as upcoming features and webinars to sign up for, right in Kibana.

Checklist

  • Make sure that fetch doesn't happen on the Login page.

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services (Team:Stack Services)

@YulNaumenko YulNaumenko changed the title [DRAFT] Added base folder structure for Newsfeed plugin [DRAFT] Newsfeed plugin for Kibana Oct 28, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@YulNaumenko YulNaumenko requested a review from a team as a code owner October 29, 2019 19:11
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

@YulNaumenko great progress!

I started this locally and noticed that the old/current styles were being applied. Upon further testing, it doesn't appear the new _header_alert.scss file is being properly imported. Instead, we are seeing styles from the big EUI CSS blob that Kibana imports).

Related, I was are able to import the EuiHeaderAlert component, but I see that it gives a warning since it does not currently support a badge prop. We're working on updating this component on the EUI side to include these new styles and a badge prop, but it will take while to work its way through the system.

For the short term, once the new SCSS is imported, we'll need to override those old styles. We could apply an extra class on the Flyout body, for example, to get more specificity on these overriding styles. Something like...

<EuiFlyoutBody className=“kbnNews__flyoutAlerts”>

...and then add that class to the top level selector like...

.kbnNews__flyoutAlerts .euiHeaderAlert {...

@YulNaumenko
Copy link
Contributor Author

@YulNaumenko great progress!

I started this locally and noticed that the old/current styles were being applied. Upon further testing, it doesn't appear the new _header_alert.scss file is being properly imported. Instead, we are seeing styles from the big EUI CSS blob that Kibana imports).

Related, I was are able to import the EuiHeaderAlert component, but I see that it gives a warning since it does not currently support a badge prop. We're working on updating this component on the EUI side to include these new styles and a badge prop, but it will take while to work its way through the system.

For the short term, once the new SCSS is imported, we'll need to override those old styles. We could apply an extra class on the Flyout body, for example, to get more specificity on these overriding styles. Something like...

<EuiFlyoutBody className=“kbnNews__flyoutAlerts”>

...and then add that class to the top level selector like...

.kbnNews__flyoutAlerts .euiHeaderAlert {...

The styles is not imported because it is a New Platform plugin, and adding stylesheets doesn't supported by this new sort of plugins. I will push the fix for this in the next commit. I didn't ping you yes because the styles in progress now. I will introduce it under the legacy plugin with the same id.

…der for legacy plugin 'newsfeed' with the same id to support this
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@gmmorris gmmorris self-requested a review October 30, 2019 14:32
@tsullivan
Copy link
Member

Our new functional tests are passing, in CI Group 6:

         └-: Newsfeed icon button
           └-> "before all" hook
           └-> "before all" hook
           └-> has red icon which is a sign of not checked news
             └-> "before each" hook: global before each
             └- ✓ pass  (181ms) "homepage app Newsfeed icon button has red icon which is a sign of not checked news"
           └-> clicking on newsfeed icon should open you newsfeed
             └-> "before each" hook: global before each
             └- ✓ pass  (127ms) "homepage app Newsfeed icon button clicking on newsfeed icon should open you newsfeed"
           └-> no red icon, because all news is checked
             └-> "before each" hook: global before each
             └- ✓ pass  (2.6s) "homepage app Newsfeed icon button no red icon, because all news is checked"
           └-> shows all news from newsfeed
             └-> "before each" hook: global before each
             └- ✓ pass  (61ms) "homepage app Newsfeed icon button shows all news from newsfeed"
           └-> clicking on newsfeed icon should close opened newsfeed
             └-> "before each" hook: global before each
             └- ✓ pass  (2.6s) "homepage app Newsfeed icon button clicking on newsfeed icon should close opened newsfeed"

Not sure where to find the error handling tests yet

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

No blockers so LGTM!
There are a few small things I'd like revisited, but nothing that I feel is worth blocking over. :)

test/functional/apps/home/_newsfeed.ts Outdated Show resolved Hide resolved
src/plugins/newsfeed/public/plugin.tsx Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@willemdh
Copy link

@YulNaumenko Hopefully this news feed can be disabled on Space Level or global level? Advertising new features could be useful for some users, but is also distracting and can result in users trying things they should not, time wasting questions, etc..

@tsullivan
Copy link
Member

@willemdh yes, it can be disabled from a Kibana instance with a setting in kibana.yml:
https://github.com/elastic/kibana/pull/49579/files#diff-2499d53955d2a55313d347b85522d7f7

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -29,6 +29,7 @@ export default function ({ getService, loadTestFile }) {

loadTestFile(require.resolve('./_navigation'));
loadTestFile(require.resolve('./_home'));
loadTestFile(require.resolve('./_newsfeed'));
Copy link
Member

Choose a reason for hiding this comment

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

This way tests will be executed under default config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is supposed to be execute this way, because in this file is only success newsfeed tests

@tsullivan
Copy link
Member

The "error handling" test case is successfully running in CI Group 6!

       └-: Newsfeed icon button handle errors
         └-> "before all" hook
         └-> "before all" hook
           │ proc [kibana]  error  [22:26:56.427]  Error: Internal server error
           │ proc [kibana]     at handler (/dev/shm/workspace/kibana/test/common/fixtures/plugins/newsfeed/newsfeed_simulation.ts:71:13)
           │ proc [kibana]     at module.exports.internals.Manager.execute (/dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-6/node_modules/hapi/lib/toolkit.js:35:106)
           │ proc [kibana]     at Object.internals.handler (/dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-6/node_modules/hapi/lib/handler.js:50:48)
           │ proc [kibana]     at exports.execute (/dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-6/node_modules/hapi/lib/handler.js:35:36)
           │ proc [kibana]     at Request._lifecycle (/dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-6/node_modules/hapi/lib/request.js:263:62)
           │ proc [kibana]     at process._tickCallback (internal/process/next_tick.js:68:7)
         └-> clicking on newsfeed icon should open you empty newsfeed
           └-> "before each" hook: global before each
             │ERROR browser[SEVERE] http://localhost:6161/api/_newsfeed-FTS-external-service-simulators/kibana/crash.json - Failed to load resource: the server responded with a status of 500 (Internal Server Error)
             │ERROR browser[SEVERE] http://localhost:6161/bundles/plugin/newsfeed.bundle.js 0:20414 Error: Internal Server Error
             │          at _callee6$ (http://localhost:6161/bundles/commons.bundle.js:1:1195605)
             │          at tryCatch (http://localhost:6161/built_assets/dlls/vendors.bundle.dll.js:344:133885)
             │          at Generator.invoke [as _invoke] (http://localhost:6161/built_assets/dlls/vendors.bundle.dll.js:344:137734)
             │          at Generator.forEach.prototype.<computed> [as next] (http://localhost:6161/built_assets/dlls/vendors.bundle.dll.js:344:135009)
             │          at asyncGeneratorStep (http://localhost:6161/bundles/commons.bundle.js:1:1186979)
             │          at _next (http://localhost:6161/bundles/commons.bundle.js:1:1187290)
           └- ✓ pass  (235ms) "Newsfeed icon button handle errors clicking on newsfeed icon should open you empty newsfeed"
         └-> no red icon
           └-> "before each" hook: global before each
           └- ✓ pass  (2.5s) "Newsfeed icon button handle errors no red icon"
         └-> shows empty panel due to error response
           └-> "before each" hook: global before each
           └- ✓ pass  (10.1s) "Newsfeed icon button handle errors shows empty panel due to error response"
         └-> clicking on newsfeed icon should close opened newsfeed
           └-> "before each" hook: global before each
           └- ✓ pass  (2.6s) "Newsfeed icon button handle errors clicking on newsfeed icon should close opened newsfeed"
         └-> "after all" hook
     │
     │4 passing (19.1s)

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.

React code LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan merged commit a50dbef into master Nov 13, 2019
@tsullivan tsullivan deleted the newsfeed_system_ui branch November 13, 2019 15:48
tsullivan pushed a commit to tsullivan/kibana that referenced this pull request Nov 13, 2019
* Added base folder structure for Newsfeed plugin

* Added base folders for lib and component

* Added newsfeed button to navigation controls on the right side

* add getApi() to return api data observable (elastic#49581)

* Added flyout base body and provided EuiHeaderAlert component inside the newsfeed plugin

* Moved newsfeed plugin to OSS and added for the styles purpose new folder for legacy plugin 'newsfeed' with the same id to support this

* Added subscribe on fetch newsfeed change

* Add NewsfeedApiDriver class (elastic#49710)

* add NewsfeedApiDriver class

* fix xpack prefix

* add corner case handling

* Added data binding to the ui

* added EuiHeaderAlert style overrides (elastic#49739)

* Fixed due to comments on PR

* add missing fields to NewsfeedItem and FetchResult

* fix templating of service url

* gracefully handle temporary request failure

* Mapped missing fields for data and badge

* Fixed typos issues

* integrate i18n.getLocale()

* allow service url root to be changed in dev mode

* replace a lot of consts with config

* fix flyout height (elastic#49809)

* Add "error" field to FetchResult: Error | null

* simplify fetch error handling

* Do not store hash for items that are filtered out

* add expireOn in case it is useful to UI

* always use staging url for dev config

* unit test for newsfeed api driver

* simplify modelItems

* Fixed eslint errors

* Fixed label translations

* Add unit test for concatenating the stored hashes with the new

* add newsfeed to i18n.json

* Fixed expression error

* --wip-- [skip ci]

* fix parse error

* fix test

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* add tests for getApi()

* add tests for getApi

* Added no news page

* fix fetch not happening after page refresh with sessionStorage primed

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* Added loading screen

* Small fixes due to comments

* Fixed issue with stop fetching news on error catch

* test(newsfeed): Configure FTS to point newsfeed to the simulated newsfeed endpoit

* Fixed browser error message: Invariant Violation: [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry.

* Fixed typo issue in label name

* polish the code changes

* Add simple jest/enzyme tests for the components

* honor utc format

* Filter pre-published items

* Fall back to en

* retry tests

* comment clarfication

* Setup newsfeed service fixture from test/common/config

* Added base functional tests for newsfeed functionality

* valid urlroot is for prod

* add documentation for the supported enabled setting

* more urlRoot

* --wip-- [skip ci]

* add the before for fn

* add ui_capabilties test

* update jest snapshot

* Fixed failing test

* finish newsfeed error functional test

* include ui_capability config

* error case testing in ci group 6

* refactor(newsfeed): moved newsfeed api call so that it is done before its use

* code polish

* enabled newsfeed_err test in CI
tsullivan pushed a commit to tsullivan/kibana that referenced this pull request Nov 13, 2019
* Added base folder structure for Newsfeed plugin

* Added base folders for lib and component

* Added newsfeed button to navigation controls on the right side

* add getApi() to return api data observable (elastic#49581)

* Added flyout base body and provided EuiHeaderAlert component inside the newsfeed plugin

* Moved newsfeed plugin to OSS and added for the styles purpose new folder for legacy plugin 'newsfeed' with the same id to support this

* Added subscribe on fetch newsfeed change

* Add NewsfeedApiDriver class (elastic#49710)

* add NewsfeedApiDriver class

* fix xpack prefix

* add corner case handling

* Added data binding to the ui

* added EuiHeaderAlert style overrides (elastic#49739)

* Fixed due to comments on PR

* add missing fields to NewsfeedItem and FetchResult

* fix templating of service url

* gracefully handle temporary request failure

* Mapped missing fields for data and badge

* Fixed typos issues

* integrate i18n.getLocale()

* allow service url root to be changed in dev mode

* replace a lot of consts with config

* fix flyout height (elastic#49809)

* Add "error" field to FetchResult: Error | null

* simplify fetch error handling

* Do not store hash for items that are filtered out

* add expireOn in case it is useful to UI

* always use staging url for dev config

* unit test for newsfeed api driver

* simplify modelItems

* Fixed eslint errors

* Fixed label translations

* Add unit test for concatenating the stored hashes with the new

* add newsfeed to i18n.json

* Fixed expression error

* --wip-- [skip ci]

* fix parse error

* fix test

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* add tests for getApi()

* add tests for getApi

* Added no news page

* fix fetch not happening after page refresh with sessionStorage primed

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* Added loading screen

* Small fixes due to comments

* Fixed issue with stop fetching news on error catch

* test(newsfeed): Configure FTS to point newsfeed to the simulated newsfeed endpoit

* Fixed browser error message: Invariant Violation: [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry.

* Fixed typo issue in label name

* polish the code changes

* Add simple jest/enzyme tests for the components

* honor utc format

* Filter pre-published items

* Fall back to en

* retry tests

* comment clarfication

* Setup newsfeed service fixture from test/common/config

* Added base functional tests for newsfeed functionality

* valid urlroot is for prod

* add documentation for the supported enabled setting

* more urlRoot

* --wip-- [skip ci]

* add the before for fn

* add ui_capabilties test

* update jest snapshot

* Fixed failing test

* finish newsfeed error functional test

* include ui_capability config

* error case testing in ci group 6

* refactor(newsfeed): moved newsfeed api call so that it is done before its use

* code polish

* enabled newsfeed_err test in CI
@tsullivan
Copy link
Member

7.x/7.6: #50503
7.5.0: #50504

tsullivan added a commit that referenced this pull request Nov 13, 2019
* [Newsfeed] UI plugin for Kibana (#49579)

* Added base folder structure for Newsfeed plugin

* Added base folders for lib and component

* Added newsfeed button to navigation controls on the right side

* add getApi() to return api data observable (#49581)

* Added flyout base body and provided EuiHeaderAlert component inside the newsfeed plugin

* Moved newsfeed plugin to OSS and added for the styles purpose new folder for legacy plugin 'newsfeed' with the same id to support this

* Added subscribe on fetch newsfeed change

* Add NewsfeedApiDriver class (#49710)

* add NewsfeedApiDriver class

* fix xpack prefix

* add corner case handling

* Added data binding to the ui

* added EuiHeaderAlert style overrides (#49739)

* Fixed due to comments on PR

* add missing fields to NewsfeedItem and FetchResult

* fix templating of service url

* gracefully handle temporary request failure

* Mapped missing fields for data and badge

* Fixed typos issues

* integrate i18n.getLocale()

* allow service url root to be changed in dev mode

* replace a lot of consts with config

* fix flyout height (#49809)

* Add "error" field to FetchResult: Error | null

* simplify fetch error handling

* Do not store hash for items that are filtered out

* add expireOn in case it is useful to UI

* always use staging url for dev config

* unit test for newsfeed api driver

* simplify modelItems

* Fixed eslint errors

* Fixed label translations

* Add unit test for concatenating the stored hashes with the new

* add newsfeed to i18n.json

* Fixed expression error

* --wip-- [skip ci]

* fix parse error

* fix test

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* add tests for getApi()

* add tests for getApi

* Added no news page

* fix fetch not happening after page refresh with sessionStorage primed

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* Added loading screen

* Small fixes due to comments

* Fixed issue with stop fetching news on error catch

* test(newsfeed): Configure FTS to point newsfeed to the simulated newsfeed endpoit

* Fixed browser error message: Invariant Violation: [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry.

* Fixed typo issue in label name

* polish the code changes

* Add simple jest/enzyme tests for the components

* honor utc format

* Filter pre-published items

* Fall back to en

* retry tests

* comment clarfication

* Setup newsfeed service fixture from test/common/config

* Added base functional tests for newsfeed functionality

* valid urlroot is for prod

* add documentation for the supported enabled setting

* more urlRoot

* --wip-- [skip ci]

* add the before for fn

* add ui_capabilties test

* update jest snapshot

* Fixed failing test

* finish newsfeed error functional test

* include ui_capability config

* error case testing in ci group 6

* refactor(newsfeed): moved newsfeed api call so that it is done before its use

* code polish

* enabled newsfeed_err test in CI

* correct bad merge resolve

* allow default export for ftr file

* fix type check error
tsullivan added a commit that referenced this pull request Nov 13, 2019
* [Newsfeed] UI plugin for Kibana (#49579)

* Added base folder structure for Newsfeed plugin

* Added base folders for lib and component

* Added newsfeed button to navigation controls on the right side

* add getApi() to return api data observable (#49581)

* Added flyout base body and provided EuiHeaderAlert component inside the newsfeed plugin

* Moved newsfeed plugin to OSS and added for the styles purpose new folder for legacy plugin 'newsfeed' with the same id to support this

* Added subscribe on fetch newsfeed change

* Add NewsfeedApiDriver class (#49710)

* add NewsfeedApiDriver class

* fix xpack prefix

* add corner case handling

* Added data binding to the ui

* added EuiHeaderAlert style overrides (#49739)

* Fixed due to comments on PR

* add missing fields to NewsfeedItem and FetchResult

* fix templating of service url

* gracefully handle temporary request failure

* Mapped missing fields for data and badge

* Fixed typos issues

* integrate i18n.getLocale()

* allow service url root to be changed in dev mode

* replace a lot of consts with config

* fix flyout height (#49809)

* Add "error" field to FetchResult: Error | null

* simplify fetch error handling

* Do not store hash for items that are filtered out

* add expireOn in case it is useful to UI

* always use staging url for dev config

* unit test for newsfeed api driver

* simplify modelItems

* Fixed eslint errors

* Fixed label translations

* Add unit test for concatenating the stored hashes with the new

* add newsfeed to i18n.json

* Fixed expression error

* --wip-- [skip ci]

* fix parse error

* fix test

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* add tests for getApi()

* add tests for getApi

* Added no news page

* fix fetch not happening after page refresh with sessionStorage primed

* test(newsfeed): Added testing endpoint which simulates the Elastic Newsfeed for consumption in functional tests

* Added loading screen

* Small fixes due to comments

* Fixed issue with stop fetching news on error catch

* test(newsfeed): Configure FTS to point newsfeed to the simulated newsfeed endpoit

* Fixed browser error message: Invariant Violation: [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry.

* Fixed typo issue in label name

* polish the code changes

* Add simple jest/enzyme tests for the components

* honor utc format

* Filter pre-published items

* Fall back to en

* retry tests

* comment clarfication

* Setup newsfeed service fixture from test/common/config

* Added base functional tests for newsfeed functionality

* valid urlroot is for prod

* add documentation for the supported enabled setting

* more urlRoot

* --wip-- [skip ci]

* add the before for fn

* add ui_capabilties test

* update jest snapshot

* Fixed failing test

* finish newsfeed error functional test

* include ui_capability config

* error case testing in ci group 6

* refactor(newsfeed): moved newsfeed api call so that it is done before its use

* code polish

* enabled newsfeed_err test in CI

* allow default export for ftr file

* [Newsfeed/Lint] fix chained fn lint
@alexfrancoeur alexfrancoeur mentioned this pull request Nov 15, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants