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

[APM] Delay rendering invalid license notification #53924

Merged
merged 2 commits into from
Jan 10, 2020
Merged

[APM] Delay rendering invalid license notification #53924

merged 2 commits into from
Jan 10, 2020

Conversation

dgieselaar
Copy link
Member

Don't render an invalid license notification if the license information has not been loaded. (Don't render any UI either).

This only happens for a very, usually unnoticeable, amount of time, but it's still visible from time to time AFAICT.

@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 3, 2020
@dgieselaar dgieselaar requested a review from a team as a code owner January 3, 2020 09:37
sorenlouv
sorenlouv reviewed Jan 3, 2020
View reviewed changes
x-pack/legacy/plugins/apm/public/context/LicenseContext/index.tsx Outdated
@@ -19,6 +19,10 @@ export function LicenseProvider({ children }: { children: React.ReactChild }) {
const license = useObservable(license$);
const hasInvalidLicense = !license?.isActive;

if (!license) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

This adds an additional (third) case. I don't think that is necessary. The two states that's needed imo:

  1. If the license has loaded AND is invalid, show <InvalidLicenseNotification />
  2. If license is not loaded OR loaded and valid, show children: <LicenseContext.Provider value={license} children={children} />

sorenlouv
sorenlouv previously requested changes Jan 3, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

In most cases the license will be valid eventually, so the user should not have to wait for the license check until they can see the app.

@dgieselaar
Copy link
Member Author

dgieselaar commented Jan 3, 2020 via email

@sorenlouv
Copy link
Member

sorenlouv commented Jan 3, 2020 •
edited
Loading

That was my initial thought. But then you'd still have a flash of a
different UI (if your license is not valid). Or is that acceptable because
we expect our users to have an appropriate license?

I think by far the majority pagehits (like, 90% +) will have a valid license so we should optimize for that case. I'm therefore good with a flash from app to license message if it's invalid.

@dgieselaar
[APM] Delay rendering invalid license notification
8d6fb09 
Don't render an invalid license notification if the license information has not been loaded. (Don't render any UI either).
@dgieselaar
Show UI if license has not loaded
9505d3f
@dgieselaar dgieselaar force-pushed the delay-license-notif branch from 6f0528d to 9505d3f Compare January 9, 2020 21:29
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #17717 succeeded 6f0528df5424c41a73071113c9c95709e6afa96b

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

@smith smith dismissed sorenlouv’s stale review January 10, 2020 17:23

The PR has been updated as @sqren is on vacation.

smith
smith approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

tenor-69672635

@dgieselaar dgieselaar merged commit c87ba85 into elastic:master Jan 10, 2020
@dgieselaar dgieselaar deleted the delay-license-notif branch January 10, 2020 21:07
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 13, 2020
@gmmorris
Merge branch 'master' into task-manager/schema-config
4173614 
* master: (69 commits)
  [Graph] Fix various a11y issues (elastic#54097)
  Add ApplicationService app status management (elastic#50223)
  logs in one time (elastic#54447)
  Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392)
  [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457)
  Security - Role Mappings UI (elastic#53620)
  [SIEM] [Detection engine] Permission II (elastic#54292)
  Allow User to Cleanup Repository from UI  (elastic#53047)
  [Detection engine] Some UX for rule creation (elastic#54471)
  share specific instances of some ui packages (elastic#54079)
  [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792)
  [APM] Delay rendering invalid license notification (elastic#53924)
  [Graph] Improve error message on graph requests (elastic#54230)
  [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719)
  Unit Tests for common/lib (elastic#53736)
  [Graph] Only show explorable fields (elastic#54101)
  remove linting rule exception for markdown (elastic#54232)
  [Monitoring] Fetch shard data more efficiently (elastic#54028)
  [Maps] Add hiddenLayers option to embeddable map input (elastic#54355)
  Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 13, 2020
@gmmorris
Merge branch 'master' into alerting/zombie-redux
0ce6ed5 
* master: (69 commits)
  [Graph] Fix various a11y issues (elastic#54097)
  Add ApplicationService app status management (elastic#50223)
  logs in one time (elastic#54447)
  Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392)
  [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457)
  Security - Role Mappings UI (elastic#53620)
  [SIEM] [Detection engine] Permission II (elastic#54292)
  Allow User to Cleanup Repository from UI  (elastic#53047)
  [Detection engine] Some UX for rule creation (elastic#54471)
  share specific instances of some ui packages (elastic#54079)
  [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792)
  [APM] Delay rendering invalid license notification (elastic#53924)
  [Graph] Improve error message on graph requests (elastic#54230)
  [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719)
  Unit Tests for common/lib (elastic#53736)
  [Graph] Only show explorable fields (elastic#54101)
  remove linting rule exception for markdown (elastic#54232)
  [Monitoring] Fetch shard data more efficiently (elastic#54028)
  [Maps] Add hiddenLayers option to embeddable map input (elastic#54355)
  Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391)
  ...
@dgieselaar dgieselaar mentioned this pull request Jan 13, 2020
Merged
dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Jan 13, 2020
@dgieselaar
[APM] Delay rendering invalid license notification (elastic#53924)
8d85de3 
* [APM] Delay rendering invalid license notification

Don't render an invalid license notification if the license information has not been loaded. (Don't render any UI either).

* Show UI if license has not loaded
@dgieselaar dgieselaar mentioned this pull request Jan 14, 2020
Merged
@ogupte ogupte self-assigned this Jan 16, 2020
@ogupte ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smith smith smith approved these changes

@sorenlouv sorenlouv sorenlouv left review comments

Assignees

@ogupte ogupte

Labels
apm:test-plan-done Pull request that was successfully tested during the test plan release_note:skip Skip the PR/issue when compiling release notes v7.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dgieselaar @sorenlouv @kibanamachine @smith @ogupte