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

Fix newsfeed unread notifications always on when reloading Kibana #100357

Merged
merged 11 commits into from
Jun 2, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented May 19, 2021

Summary

Fix #99191

Fix the newsfeed unread notification being reset every time Kibana is reloaded

  • switch from session to local storage
  • use the same persistence key per news feed
  • update the read status in the storage when the user opens the newsfeed flyout.
  • cleanup, split for proper isolation of concerns, add tests

Checklist

@pgayvallet pgayvallet added v7.14.0 release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 20, 2021
@pgayvallet pgayvallet marked this pull request as ready for review May 20, 2021 07:55
@pgayvallet pgayvallet requested a review from a team as a code owner May 20, 2021 07:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

afharo
afharo previously requested changes May 24, 2021
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM! Code review only. I'm requesting changes because I believe that we can remove NewsfeedApiFetchResult, and maybe we should consider making the test with the date in the future a bit more dynamic.

src/plugins/newsfeed/public/lib/driver.test.ts Outdated Show resolved Hide resolved
src/plugins/newsfeed/public/lib/storage.ts Outdated Show resolved Hide resolved
return true;
};

export const convertItem = (rawItem: ApiItem, userLanguage: string): NewsfeedItem => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: seems like convert is a pretty vague term here, maybe this function and file should be localize_items instead?

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 makes sense to rename convertItem to localizeItem, however convertItems does more than that, as it also validates the content/dates of the news, so not sure the whole file should be renamed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me 👍

src/plugins/newsfeed/public/lib/driver.ts Outdated Show resolved Hide resolved
@pgayvallet pgayvallet requested review from afharo and joshdover and removed request for afharo May 31, 2021 08:11
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
newsfeed 16 14 -2

Page load bundle

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

id before after diff
newsfeed 19.0KB 18.1KB -959.0B

History

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

@pgayvallet pgayvallet enabled auto-merge (squash) June 2, 2021 14:01
@pgayvallet pgayvallet requested review from afharo and removed request for afharo June 2, 2021 14:01
@pgayvallet pgayvallet dismissed afharo’s stale review June 2, 2021 14:07

He's in PTO for a long time

@pgayvallet pgayvallet merged commit ccc1485 into elastic:master Jun 2, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jun 2, 2021
…astic#100357)

* fix the implementation

* add unit tests

* add API unit tests

* fix public interface

* address review comments

* name convertItem to localizeItem

* use fetch instead of core.http and add tests
pgayvallet added a commit that referenced this pull request Jun 2, 2021
…00357) (#101163)

* fix the implementation

* add unit tests

* add API unit tests

* fix public interface

* address review comments

* name convertItem to localizeItem

* use fetch instead of core.http and add tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 2, 2021
* master: (68 commits)
  Unskip advanced settings a11y test (elastic#100558)
  [App Search] Crawler Landing Page (elastic#100822)
  [DOCS] Clarify when to use kbn clean (elastic#101155)
  change label behavior (elastic#100991)
  skip flaky suite (elastic#101126)
  Fix cases plugin ownership (elastic#101073)
  [Home] Adding file upload to add data page (elastic#100863)
  [ML] Functional tests - reenable categorization tests (elastic#101137)
  [DOCS] Adds server.uuid to settings docs (elastic#101121)
  Fix newsfeed unread notifications always on when reloading Kibana (elastic#100357)
  [Lens] Time shift metrics (elastic#98781)
  [Deprecations service] make `correctiveActions.manualSteps` required (elastic#100997)
  Add "Risk Matrix" section to the PR template (elastic#100649)
  [Maps] spatially filter by all geo fields (elastic#100735)
  [Security Solution] Add Ransomware canary advanced policy option (elastic#101068)
  [Exploratory view] Core web vitals (elastic#100320)
  [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034)
  [Lens] Use a setter function for the dimension panel (elastic#101123)
  [Index Patterns] Fix return saved index pattern object (elastic#101051)
  [CI] For PRs, build TS refs before public api docs check (elastic#100791)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newsfeed unread badge is always shown after refresh
5 participants