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

Remove permission api call #2215

Open
wants to merge 2 commits into
base: py3
Choose a base branch
from

Conversation

rllola
Copy link
Contributor

@rllola rllola commented Oct 2, 2019

This PR is a re-write of #1494

  • It adds a wrapperPermissionRemove call. Calling wrapperPermissionRemove 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 if Array 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/

…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
Copy link
Contributor

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.

Copy link
Contributor Author

@rllola rllola Oct 3, 2019

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.

Copy link
Contributor

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)

Copy link
Contributor Author

@rllola rllola Oct 4, 2019

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 ?

Copy link
Contributor

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.

Copy link
Contributor Author

@rllola rllola Oct 5, 2019

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.

src/Ui/media/Wrapper.coffee Outdated Show resolved Hide resolved
@purplesyringa
Copy link
Contributor

purplesyringa commented Oct 2, 2019

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.

@HelloZeroNet
Copy link
Owner

HelloZeroNet commented Oct 3, 2019

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.

@purplesyringa
Copy link
Contributor

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.

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.

3 participants