-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[APM] License prompt for service map #52668
Conversation
setLicense(nextLicense); | ||
}); | ||
return () => subscription.unsubscribe(); | ||
}, [license$]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the subscription happen inside the context and not the other way around? If several components call useLicense
we'll have several subscriptions. If we add it to the context we'll only have one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b7e5f18.
@@ -148,7 +148,9 @@ export class ServiceIntegrations extends React.Component<Props, State> { | |||
panels={[ | |||
{ | |||
id: 0, | |||
items: this.getPanelItems(license.features.ml?.is_available) | |||
items: this.getPanelItems( | |||
license.getFeature('ml').isAvailable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will getFeature
always return an object regardless if the plugin is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. getFeature
always returns a LicenseFeature
:
kibana/x-pack/plugins/licensing/common/types.ts
Lines 30 to 33 in 88ae8d0
export interface LicenseFeature { | |
isAvailable: boolean; | |
isEnabled: boolean; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using ILicense | undefined
now so a ?
has been added to license
, but the above still holds.
import React from 'react'; | ||
import { FETCH_STATUS, useFetcher } from '../../hooks/useFetcher'; | ||
import { loadLicense, LicenseApiResponse } from '../../services/rest/xpack'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also remove the loadLicense
method (and the entire rest/xpack.ts
file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file is deleted.
921e593
to
25cb6f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small suggestions
const isValidPlatinumLicense = | ||
license !== undefined && | ||
license.status === 'active' && | ||
license.type === 'platinum'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isValidPlatinumLicense = license?.isActive && license?.type === 'platinum'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 44d9568.
[http] | ||
); | ||
const hasValidLicense = data.license.is_active; | ||
const hasInValidLicense = license !== undefined && !license.isActive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const hasInValidLicense = license !== undefined && !license.isActive; | |
const hasInValidLicense = !license?.isActive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 44d9568.
import { i18n } from '@kbn/i18n'; | ||
import { useKibanaUrl } from '../../../hooks/useKibanaUrl'; | ||
|
||
export function LicensePrompt() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps call it "PlatinumLicensePrompt" Since we already have another prompt that ensures the user has a license at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 44d9568.
|
||
// if license is invalid show an error message | ||
if (status === FETCH_STATUS.SUCCESS && !hasValidLicense) { | ||
if (hasInValidLicense) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (hasInValidLicense) { | |
if (hasInvalidLicense) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 44d9568.
|
||
export function useKibanaUrl( | ||
/** The path to the plugin */ path: string, | ||
/** The hash path */ hash: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting looks odd. Shouldn't the comments either be after the argument:
path: string // The path to the plugin
Or on the line above:
// The path to the plugin
path: string
Either way I don't think they are terribly helpful. So perhaps just remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits
@smith can you add a "Closes #issue" to tie the PR and issue together? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana_react
export LGTM
Display a link to platinum license upgrade on the service map. Also add `useKibanaUrl` and `useLicense` hooks. Make the `LicenseContext` (which is used in a couple class components and on page load) use the license observable from the NP plugin. Add missing export of `useObservable` to kibana_react.
Removing review request for @elastic/kibana-security since this doesn't touch anything we own. Let me know if you'd still like a review! |
@elasticmachine test this please |
@elasticmachine test this please |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Display a link to platinum license upgrade on the service map. Also add `useKibanaUrl` and `useLicense` hooks. Make the `LicenseContext` (which is used in a couple class components and on page load) use the license observable from the NP plugin. Add missing export of `useObservable` to kibana_react.
Display a link to platinum license upgrade on the service map. Also add `useKibanaUrl` and `useLicense` hooks. Make the `LicenseContext` (which is used in a couple class components and on page load) use the license observable from the NP plugin. Add missing export of `useObservable` to kibana_react.
Display a link to platinum license upgrade on the service map. Also add `useKibanaUrl` and `useLicense` hooks. Make the `LicenseContext` (which is used in a couple class components and on page load) use the license observable from the NP plugin. Add missing export of `useObservable` to kibana_react.
Display a link to platinum license upgrade on the service map. Also add `useKibanaUrl` and `useLicense` hooks. Make the `LicenseContext` (which is used in a couple class components and on page load) use the license observable from the NP plugin. Add missing export of `useObservable` to kibana_react.
Tested in Chrome, FF, IE11, Safari. |
Display a link to platinum license upgrade on the service map.
Also add
useKibanaUrl
anduseLicense
hooks.Make the
LicenseContext
(which is used in a couple class components and on page load) use the license observable from the NP plugin.Add missing export of
useObservable
to kibana_react.Closes #52375.