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

Custom notification banner is not working anymore #44928

Closed
bhavyarm opened this issue Sep 5, 2019 · 9 comments · Fixed by #43610
Closed

Custom notification banner is not working anymore #44928

bhavyarm opened this issue Sep 5, 2019 · 9 comments · Fixed by #43610
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages regression Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@bhavyarm
Copy link
Contributor

bhavyarm commented Sep 5, 2019

Kibana version: 7.4.0 BC2

Elasticsearch version: 7.4.0 BC2

Server OS version: darwin_x86_64

Browser version: chrome latest/safari latest

Browser OS version: OS X

Original install method (e.g. download page, yum, from source, etc.): from staging

Describe the bug: Custom notification banners are not working anymore.

Steps to reproduce:

  1. Go to advanced settings and look for "Custom banner notification"
  2. Add some input text
  3. Save it - refresh the browser - it doesn't display. Logout and login and it doesn't display

Expected behavior:

Custom notification banner should display

@bhavyarm bhavyarm added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages labels Sep 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@bhavyarm
Copy link
Contributor Author

bhavyarm commented Sep 5, 2019

Worked fine in 7.3.1

@lukeelmers
Copy link
Member

i believe the global banners live inside ui/notify, a good chunk of which was cleaned up for 7.4 in #41663

not sure if this PR is actually what caused the regression, but it caught my attention

@kertal kertal self-assigned this Sep 9, 2019
@kertal
Copy link
Member

kertal commented Sep 9, 2019

@lukeelmers yes, #41663 caused this regression

The following file is no longer imported
https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/notify/notify.js

It contains

const config = chrome.getUiSettingsClient();

config.getUpdate$().subscribe(() => {
  applyConfig(config);
});

This applyConfig
https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/notify/notify.js#L42
was responsible to apply the user-defined banner

Reenabling doesn't work, leads to a Cannot read property 'getUiSettingsClient' of undefinedException, the import of chrome here is undefined. This needs to be migrated, do you have an idea where to? Or how?

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@joshdover
Copy link
Contributor

@kertal I have fixed this in #43610 where I move this code to the New Platform. That said, it's a bit more involved change (though not huge) that I'm not sure should be backported to a BC.

@kertal
Copy link
Member

kertal commented Sep 10, 2019

thx @joshdover, what alternatives do we have to backport, since as far as I can see, we can't just enable the old version of this banner?

@joshdover
Copy link
Contributor

@kertal I'm going to see if I can't get a smaller focused fix to work. If not, I think we'll just backport that PR.

@joshdover
Copy link
Contributor

Backport for 7.4 incoming: #45958

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages regression Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants