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

Get Rbac permissions #554

Merged
merged 19 commits into from
Jan 28, 2020
Merged

Get Rbac permissions #554

merged 19 commits into from
Jan 28, 2020

Conversation

tahmidefaz
Copy link
Member

Jira: Here

Let me know what needs changing here! Also, not sure if I did the tests right! 😄

@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

Merging #554 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #554   +/-   ##
=======================================
  Coverage   60.42%   60.42%           
=======================================
  Files          45       45           
  Lines         801      801           
  Branches      147      147           
=======================================
  Hits          484      484           
  Misses        260      260           
  Partials       57       57

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

When you are using next links you are essentially doing sequential requests, that can resolve into long promises. When you are going to use Promise.all each request is fired at one point and it waits for everyone to finish. There is no catch clause, so you should update the code with that.

config/setupTests.js Outdated Show resolved Hide resolved
src/js/rbac/userPermissions.js Outdated Show resolved Hide resolved
src/js/rbac/userPermissions.js Outdated Show resolved Hide resolved
@tahmidefaz
Copy link
Member Author

tahmidefaz commented Jan 20, 2020

@karelhala I wasn't aware of the rbac api client. Thank you for pointing it out! I just tweaked your suggestion a little bit, so that it fetches the correct count for permissions.

karelhala
karelhala previously approved these changes Jan 21, 2020
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Looking good! I checked it locally and it returns correct number of permissions and such. Really nice!

@tahmidefaz
Copy link
Member Author

tahmidefaz commented Jan 21, 2020

Looking good! I checked it locally and it returns correct number of permissions and such. Really nice!

Thanks for checking it locally! Appreciate it!

Copy link
Member

@ryelo ryelo left a comment

Choose a reason for hiding this comment

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

I think we should clear this localStorage variable on user logout.

@karelhala thoughts?

@karelhala
Copy link
Contributor

@PanSpagetka was working on some self clearing localstorage enhancements.

ryelo
ryelo previously approved these changes Jan 21, 2020
@tahmidefaz tahmidefaz dismissed stale reviews from ryelo and karelhala via ac91fde January 21, 2020 21:41
@tahmidefaz
Copy link
Member Author

I tried quite a few things regarding the async tests without any luck. I will give it another try tomorrow. Let me know if you have any suggestions @karelhala @ryelo

@tahmidefaz tahmidefaz requested a review from ryelo January 22, 2020 19:21
@tahmidefaz
Copy link
Member Author

Just added the test. Let me know if it needs to be improved.

@karelhala karelhala merged commit 16cc7c6 into master Jan 28, 2020
@Hyperkid123 Hyperkid123 deleted the get-rbac-permissions branch January 25, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants