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

[ML] Overview: ensure proper permissions check for empty prompt 'Create job' buttons #49067

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Oct 23, 2019

Summary

Fixes #49018

Disables Create job button for empty prompts in Overview page if permissions are not met.
Uses same permissions check for corresponding job types (anomaly detection Create job check for the anomaly detection panel and analytics Create job check for analytics panel).

Checklist

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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Disabling the Create buttons in the EuiEmptyPrompts looks good, but we should probably also hide the link to Creating a new job in the text on the left of the page if they don't have canCreateJob or ml nodes available.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87
Copy link
Contributor Author

Thanks for taking a look @peteharverson 🙌
Good catch on disabling the 'create a job' link in the sidebar text if permissions are lacking. Change made here: 7238204
Checks moved to the main Overview page parent component so we only do them when the page is loaded and not on every render.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Add two suggestions, one about a possible simplification where to do checks.

Comment on lines +26 to +35
<FormattedMessage
id="xpack.ml.overview.gettingStartedSectionCreateJob"
defaultMessage="creating a new job"
/>
) : (
<EuiLink href={createJobLink} target="blank">
<FormattedMessage
id="xpack.ml.overview.gettingStartedSectionCreateJob"
defaultMessage="creating a new job"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Suggest to move both FormattedMessage to a const so we don't have the same i18n id twice.

Comment on lines +9 to 20
import { checkPermission } from '../privilege/check_privilege';
import { mlNodesAvailable } from '../ml_nodes_check/check_ml_nodes';
import { NavigationMenu } from '../components/navigation_menu';
import { OverviewSideBar } from './components/sidebar';
import { OverviewContent } from './components/content';

export const OverviewPage: FC = () => {
const disableCreateAnomalyDetectionJob = !checkPermission('canCreateJob') || !mlNodesAvailable();
const disableCreateAnalyticsButton =
!checkPermission('canCreateDataFrameAnalytics') ||
!checkPermission('canStartStopDataFrameAnalytics');
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of moving the imports and check consts directly into the panel components? Then there would be no need to pass them down 2 levels via props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping them in the parent component makes it easier to share the permissions check since it has to be called from the panels and the sidebar component. I thought it might be preferable to calling it in 3 different places. Happy to make the change if it seems cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed that the same check is used in Panel + Sidebar, happy to leave as is!

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 merged commit f4cf28f into elastic:master Oct 24, 2019
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Oct 24, 2019
…te job' buttons (elastic#49067)

* disabled overview empty prompt create job button if no permissions

* permission check from overviewPage main component. add mlNodes check to resolver

* remove period from overview empty prompt title
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Oct 24, 2019
…te job' buttons (elastic#49067)

* disabled overview empty prompt create job button if no permissions

* permission check from overviewPage main component. add mlNodes check to resolver

* remove period from overview empty prompt title
@alvarezmelissa87 alvarezmelissa87 deleted the ml-overview-permissions branch October 24, 2019 16:33
alvarezmelissa87 added a commit that referenced this pull request Oct 24, 2019
…te job' buttons (#49067) (#49215)

* disabled overview empty prompt create job button if no permissions

* permission check from overviewPage main component. add mlNodes check to resolver

* remove period from overview empty prompt title
alvarezmelissa87 added a commit that referenced this pull request Oct 25, 2019
…te job' buttons (#49067) (#49214)

* disabled overview empty prompt create job button if no permissions

* permission check from overviewPage main component. add mlNodes check to resolver

* remove period from overview empty prompt title
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.

[ML] Create job elements on Overview page enabled for machine_learning_user
5 participants