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.

Sign up for GitHub

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

Jump to bottom

Remove permission api call #2215

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

Remove permission api call #2215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d65c945
Remove permission api call; prompt notification to confirm removing p…
rllola Oct 2, 2019
a3c54e4
typo
rllola Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/Ui/media/Wrapper.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ class Wrapper
@actionOpenWindow(message.params)
else if cmd == "wrapperPermissionAdd"
@actionPermissionAdd(message)
else if cmd == "wrapperPermissionRemove"
@actionPermissionRemove(message)
else if cmd == "wrapperRequestFullscreen"
@actionRequestFullscreen()
else if cmd == "wrapperWebNotification"
Expand Down Expand Up @@ -280,14 +282,37 @@ class Wrapper

actionPermissionAdd: (message) ->
permission = message.params
if Array.isArray(permission) && permission.length > 1
@sendInner {"cmd": "response", "to": message.id, "result": {"error": "Can only grant one permission at the time"}}
return false
if Array.isArray(permission) && permission.length == 1
permission = permission[0]
$.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 •
edited

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 •
edited

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 •
edited

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.

return false
@ws.cmd "permissionDetails", permission, (permission_details) =>
@displayConfirm "This site requests permission:" + " <b>#{@toHtmlSafe(permission)}</b>" + "<br><small style='color: #4F4F4F'>#{permission_details}</small>", "Grant", =>
@ws.cmd "permissionAdd", permission, (res) =>
@sendInner {"cmd": "response", "to": message.id, "result": res}

actionPermissionRemove: (message) ->
permission = message.params
if Array.isArray(permission) && permission.length > 1
@sendInner {"cmd": "response", "to": message.id, "result": {"error": "Can only remove one permission at the time"}}
return false
if Array.isArray(permission) && permission.length == 1
permission = permission[0]
$.when(@event_site_info).done =>
if permission not in @site_info.settings.permissions
@notifications.add "notification-#{message.id}", "info", "Permission already removed.", 4000
return false
@ws.cmd "permissionDetails", permission, (permission_details) =>
@displayConfirm "This site want to remove permission:" + " <b>#{@toHtmlSafe(permission)}</b>" + "<br><small style='color: #4F4F4F'>#{permission_details}</small>", "Remove", =>
rllola marked this conversation as resolved.
Show resolved Hide resolved
@ws.cmd "permissionRemove", permission, (res) =>
@sendInner {"cmd": "response", "to": message.id, "result": res}


actionNotification: (message) ->
message.params = @toHtmlSafe(message.params) # Escape html
body = $("<span class='message'>"+message.params[1]+"</span>")
Expand Down Expand Up @@ -702,4 +727,3 @@ else
ws_url = proto.ws + ":" + origin.replace(proto.http+":", "") + "/ZeroNet-Internal/Websocket?wrapper_key=" + window.wrapper_key

window.wrapper = new Wrapper(ws_url)

83 changes: 68 additions & 15 deletions src/Ui/media/all.js
View file
Open in desktop

Large diffs are not rendered by default.