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

Add available product features local storage. #4756

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Oct 11, 2018

Based on this https://bugzilla.redhat.com/show_bug.cgi?id=1634053, there is a requirement to show custom buttons events on certain screens. Only super admin should be able to view these events, but the API endpoint (ManageIQ/manageiq-api#464) will always return an array.

If you are not authorized, you will receive an empty array of events and nothing should be rendered on screen. That is the same result if user is authorized, but there are no custom button events. In this case there the ui should render message that no events are present.

To prevent bugs information about the user identity (if is super admin) user product_features is required.

Using this pr ManageIQ/manageiq-api#490 we can get this information and store it to localStorage

features are checked when:

  • on login
  • on current group change

localStorage is reset when:

  • before login
  • after logout

@Hyperkid123
Copy link
Contributor Author

@miq-bot assign @himdel

@Hyperkid123
Copy link
Contributor Author

@miq-bot add-reviewer @skateman

@skateman
Copy link
Member

I don't really like that we bring down a (deprecated) term (super-admin) from the backend to the frontend. What about just caching the list of features for the given group? This is-super-admin thing is highly specific to the custom button events, but the features list can be useful elsewhere.

@Hyperkid123
Copy link
Contributor Author

@skateman ok i will store all the permissions for the current group.

@Hyperkid123 Hyperkid123 changed the title [WIP] Add isSuperAdmin flag to local storage. [WIP] Add available product features local storage. Oct 15, 2018
@Hyperkid123 Hyperkid123 changed the title [WIP] Add available product features local storage. Add available product features local storage. Oct 15, 2018
@miq-bot miq-bot removed the wip label Oct 15, 2018
@@ -696,6 +706,9 @@ function miqAjaxAuth(url) {
.then(function() {
return vanillaJsAPI.ws_init();
})
.then(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

bad indent here

(otherwise lgtm)

Copy link
Member

@skateman skateman 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 you have too many commits here 😉

@@ -70,6 +70,8 @@ API.ws_destroy = function() {
};

API.logout = function() {
// delete superadmin flag from local storage
Copy link
Member

Choose a reason for hiding this comment

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

superadmin?

@miq-bot
Copy link
Member

miq-bot commented Oct 15, 2018

Checked commit Hyperkid123@724680d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@himdel
Copy link
Contributor

himdel commented Oct 16, 2018

LGTM, would merge :) but looks like it depends on ManageIQ/manageiq-api#490, so added pending core instead

@Hyperkid123
Copy link
Contributor Author

@himdel @martinpovolny ManageIQ/manageiq-api#490 has been merged can you merge this as well?

@miq-bot remove_label pending core

@martinpovolny
Copy link
Member

I am wondering if the user can gain something by manipulating the content of the local store. But don't see what that would be.

Also I am wondering if all the groups should be stored and only the active one used as opposed to reloading the perms when you switch group in the top right drop-down.

But this way, to reload features when playing with some settings it's enough to change group where if we stored all the groups a log in/log out would be needed.

So it's probably a good thing as it is in this PR.

I am going to merge this one and if what I wrote above makes someone rethink something we can address that in a follow up PR.

@martinpovolny martinpovolny merged commit 1a8c2a3 into ManageIQ:master Nov 9, 2018
@martinpovolny martinpovolny added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 9, 2018
@martinpovolny
Copy link
Member

Hammer?

@skateman
Copy link
Member

skateman commented Nov 9, 2018

@martinpovolny can't touch this
@miq-bot add_label hammer/no

@himdel
Copy link
Contributor

himdel commented Jan 15, 2019

@Hyperkid123 is this being used anywhere? Or is there a follow up PR somewhere?

It looks like there's no code touching localStorage.userFeatures in ui-classic right now.

(noticed in https://github.com/ManageIQ/manageiq-ui-classic/pull/5164/files#r248013681)

@skateman
Copy link
Member

@himdel this was my idea for something that got cancelled/postponed (not sure which)

@Hyperkid123
Copy link
Contributor Author

@himdel its postponed right now it was done because of custom buttons for user management. I would like to return to it sometimes but i can't say when i will find the time for it (too much stuff right now)

@himdel
Copy link
Contributor

himdel commented Oct 22, 2019

This is no longer happening on login since #5164,
and I'm changing the last caller => removing the rest in #6325.

We can always resurrect it, but either we need a common js after login code, or this needs to come from ruby side, from dashboard#authenticate or dashboard#change_group.

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.

6 participants