Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Upgrade Assistant] Add permissions check to logs step #112420
[Upgrade Assistant] Add permissions check to logs step #112420
Changes from 10 commits
d6ee0af
8fddc7d
9bdca01
69d4457
93ce8e3
d53055b
1c28563
a4a20da
0f4b4dc
b8ae396
65bd246
3cd92e3
c0f13fd
0030b0b
602ccf9
efe2e20
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I patched this block of code to also accept privileges that might contain dots in its name. I extracted this part into its own function and wrote a few tests for it, I decided not to write tests for the HOC itself because it seemed a bit outside the scope of this PR.
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.
Awesome!!
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.
Hmm, this is implicitly dependent on a guarantee that a privilege will never contain
&
. I understand this guarantee is unlikely to ever be broken, but I think we can be completely safe by being a bit more direct. This possibly makes the code easier to understand too.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.
Very nice suggestion, thanks for pointing that out!
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.
I had to change these types to be exported directly from the types file, otherwise it doesn't tree-shake the rest of the things exported from the
authorization
file and it causes errors if you try to use it server side (certain dependencies needed by the components exported require window object to exist).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.
Do we have any tests that assert this isn't shown when the user has the required privileges?
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.
Nice call, I've just added one that checks for that also