Skip to content

Clone role button going to edit page#70768

Merged
thomheymann merged 3 commits intomasterfrom
security/bugfix-clone-role-button
Jul 9, 2020
Merged

Clone role button going to edit page#70768
thomheymann merged 3 commits intomasterfrom
security/bugfix-clone-role-button

Conversation

@thomheymann
Copy link
Copy Markdown
Contributor

@thomheymann thomheymann commented Jul 6, 2020 •
edited
Loading

Summary

Fixes #70219

Checklist

@thomheymann thomheymann force-pushed the security/bugfix-clone-role-button branch from e6647c6 to 892b724 Compare July 6, 2020 10:54
@thomheymann thomheymann force-pushed the security/bugfix-clone-role-button branch from 6113a50 to 5277c50 Compare July 7, 2020 15:33
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Jul 7, 2020

💚 Build Succeeded

Build metrics

Saved Objects .kibana field count

id value diff baseline
action 5 +5 -
action_task_params 3 +3 -
alert 22 +22 -
apm-indices 8 +8 -
apm-telemetry 238 +238 -
application_usage_transactional 2 +2 -
canvas-element 8 +8 -
canvas-workpad 5 +5 -
canvas-workpad-template 8 +8 -
cases 32 +32 -
cases-comments 17 +17 -
cases-configure 14 +14 -
cases-user-actions 10 +10 -
config 2 +2 -
dashboard 17 +17 -
endpoint:user-artifact 10 +10 -
endpoint:user-artifact-manifest 3 +3 -
epm-packages 8 +8 -
exception-list 35 +35 -
exception-list-agnostic 35 +35 -
file-upload-telemetry 2 +2 -
fleet-agent-actions 6 +6 -
fleet-agent-events 11 +11 -
fleet-agents 23 +23 -
fleet-enrollment-api-keys 10 +10 -
graph-workspace 9 +9 -
index-pattern 10 +10 -
infrastructure-ui-source 21 +21 -
ingest_manager_settings 5 +5 -
ingest-agent-configs 11 +11 -
ingest-outputs 10 +10 -
ingest-package-configs 35 +35 -
inventory-view 40 +40 -
kql-telemetry 3 +3 -
lens 7 +7 -
lens-ui-telemetry 5 +5 -
map 7 +7 -
metrics-explorer-view 22 +22 -
ml-telemetry 3 +3 -
namespace 1 +1 -
namespaces 1 +1 -
query 6 +6 -
references 4 +4 -
sample-data-telemetry 3 +3 -
search 9 +9 -
siem-detection-engine-rule-actions 8 +8 -
siem-detection-engine-rule-status 12 +12 -
siem-ui-timeline 86 +86 -
siem-ui-timeline-note 8 +8 -
siem-ui-timeline-pinned-event 7 +7 -
space 9 +9 -
telemetry 9 +9 -
timelion-sheet 13 +13 -
tsvb-validation-telemetry 2 +2 -
type 1 +1 -
ui-metric 2 +2 -
updated_at 1 +1 -
upgrade-assistant-reindex-operation 15 +15 -
upgrade-assistant-telemetry 13 +13 -
uptime-dynamic-settings 4 +4 -
url 6 +6 -
visualization 9 +9 -
total - +951 -

History

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

@thomheymann thomheymann marked this pull request as ready for review July 7, 2020 17:41
@thomheymann thomheymann requested review from a team as code owners July 7, 2020 17:41
@thomheymann thomheymann added the Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// label Jul 7, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 7, 2020

Pinging @elastic/kibana-security (Team:Security)

pgayvallet
pgayvallet reviewed Jul 8, 2020
View reviewed changes
x-pack/plugins/security/public/management/roles/roles_grid/roles_grid_page.test.tsx
Comment on lines +48 to +50
history = (scopedHistoryMock.create({
createHref: jest.fn((location) => location.pathname!),
}) as unknown) as ScopedHistory;
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet Jul 8, 2020

Choose a reason for hiding this comment

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

We don't usually add API to override core mock properties, as the same goal can be achieved by using the existing mocked methods.

const history: ScopedHistoryMock = scopedHistoryMock.create()
history.createHref.mockImplementation((location) => location.pathname!));

However I see that our ScopedHistoryMock is not compatible with ScopedHistory, which complicates the process a little, see:

const h: ScopedHistory =  scopedHistoryMock.create()

Type 'Mocked<Pick<ScopedHistory<unknown>, "replace" | "length" | "push" | "block" | "location" | "action" | "goBack" | "go" | "goForward" | "listen" | "createHref" | "createSubHistory">>' is missing the following properties from type 'ScopedHistory<unknown>': isActive, listeners, locationKeys, currentLocationKeyIndex, and 7 more.

@joshdover wdyt? Is this change fine with you, or should we 'fix' the mock instead? I guess that would force us to create a IScopedHistory, but OTOH the mock cannot currently be properly used for testing (appart from using it for jest.mock('src/core/public'))

Copy link
Copy Markdown
Contributor

@joshdover joshdover Jul 8, 2020

Choose a reason for hiding this comment

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

I agree we should probably fix the mock and/or create an interface that doesn't require this private properties. Can be done in a follow up PR though if @thomheymann wants to keep this PR focused or have someone Platform do it.

Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet Jul 8, 2020

Choose a reason for hiding this comment

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

Ok. I'll create an issue for the mock fix, and revert this change then.

Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet Jul 8, 2020

Choose a reason for hiding this comment

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

created #71166

legrego
legrego approved these changes Jul 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM pending decision on platform's ScopedHistoryMock. Thanks for the fix!

pgayvallet
pgayvallet approved these changes Jul 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

@pgayvallet pgayvallet mentioned this pull request Jul 8, 2020
Closed
@thomheymann thomheymann merged commit a285ede into master Jul 9, 2020
@thomheymann thomheymann deleted the security/bugfix-clone-role-button branch July 9, 2020 09:15
thomheymann added a commit to thomheymann/kibana that referenced this pull request Jul 9, 2020
@thomheymann
Clone role button going to edit page (elastic#70768)
8c6c15a 
* Clone role button going to edit page

* Added unit tests

* Fixed types
@thomheymann thomheymann mentioned this pull request Jul 9, 2020
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Jul 13, 2020

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jul 13, 2020
thomheymann added a commit that referenced this pull request Jul 13, 2020
@thomheymann @elasticmachine
Clone role button going to edit page (#70768) (#71218)
a33504b 
* Clone role button going to edit page

* Added unit tests

* Fixed types

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@thomheymann
Copy link
Copy Markdown
Contributor Author

thomheymann commented Jul 13, 2020

7.x #71218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@legrego legrego legrego approved these changes

+1 more reviewer

@joshdover joshdover joshdover left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:fix Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.9.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clone role button going to edit page

6 participants

@thomheymann @kibanamachine @elasticmachine @pgayvallet @joshdover @legrego