-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove permission api call #2215
base: py3
Are you sure you want to change the base?
Conversation
…ermission; fix checking if permission is already granted;
$.when(@event_site_info).done => | ||
if permission in @site_info.settings.permissions | ||
@notifications.add "notification-#{message.id}", "info", "Permission already granted.", 4000 |
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.
Please remove this. This condition was added for the use case when you don't want to check whether you have the permission already.
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 think giving some form of feedback on what is happening is a good practice. If not a notification, we can send a message back.
Also note that the condition was always false because permission was an array. Even when the permission was already granted it would keep asking you.
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.
You could pass a single argument instead of an array of arguments but whatever. I think that you should just reply with an "already added" message. (but not an error)
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.
You could pass a single argument instead of an array of arguments but whatever.
Yeah, we need to update the documentation though because in the example it is an array (https://zeronet.io/docs/site_development/zeroframe_api_reference/#wrapperpermissionadd). I am not sure how to handle this without breaking compatibility in case people have done the same as in the documentation.
I think that you should just reply with an "already added" message. (but not an error)
I can remove the notification and replace by a response to the site. However I do have an argument in favor of keeping it. The user is used to be asked if he wants to grant or not a permission. In the case the permission would have already been granted (but the user might not be aware of it) it would not prompt the question and the site might neither inform the user of it. The lack of information and the incoherent experience could be confusing for the user.
Does it make sense to you ?
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'm afraid it still doesn't. See, many sites need Merger:whatever permission. They don't check whether they already have it, they just send wrapperPermissionAdd on page load. Currently, it would only bother the user once, but if you add notification it'll show up when the site is loaded and bother the user a lot more.
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.
You made a good point. I agree with you regarding the Merger Site permission. I will proceed to modify it. Thank you for the feedback.
I'm afraid the current implementation can give a false sense of security. A zite can draw a box similar to the real permission remove box, and the user would think that they removed the permission so they're now safe. This looks like a huge problem. |
I don't think it's a huge problem (actually I'm not sure if we need confirmation on permission remove at all), but to improve the situation we could give different look to the confirm dialogs generated by the wrapper. |
I think there's no point in the site removing its own permission. It makes more sense to make a single configuration page for all sites. |
This PR is a re-write of #1494
It adds a
wrapperPermissionRemove
call. CallingwrapperPermissionRemove
will prompt a notification message to ask user for confirmation.If more than one permission is asked at the same time an error message will be send.
If the permission is not already granted it will prompt an informative message.
Also added a small the same behavior for
wrapperPermissionAdd
; it will send an error if more than one permission being passed as an argument and prompt a notification if permission already granted.NOTE: The
wrapperPermissionAdd
verification for already granted permission was failing because we are checking ifArray is in Array
because in the documentation we passed the permission as an array (https://zeronet.io/docs/site_development/zeroframe_api_reference/#wrapperpermissionadd). In order to avoid this I added an if statement to check we passed an array.NOTE 2: A small site to test http://127.0.0.1:43111/1MMF8RiHAqU2Ac6guTX5KLBeEsat846ien/