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

SAK-43711 (V2): SWITCH: Mailarchive: Use permissions widget #8256

Merged
merged 1 commit into from Jun 30, 2020

Conversation

frasese
Copy link
Contributor

@frasese frasese commented May 29, 2020

  • Moved properties files from tool to api.
  • Merged permissions.properties files into mailarchive.properties.
  • Adapted to permissions webcomponent.
  • Update PermissionsEntitiyProvider to filter by SEARCH + "."

@frasese
Copy link
Contributor Author

frasese commented May 29, 2020

This tool has a special problem related with permissions names. They are called mail.XXX ("mail.read", "mail.new" and "mail.delete.any"). The problem is, when the webcomponent retrieves the permissions data from the direct REST url (PermissionsEntitiyProvider), it checks for everything that starts with "mail". So, not only mail.XXX (mailarchive's permissions), but also mailtool.XXX (mailsender's permissions) are returned.

So we have two different solutions here.

1.- Set the webcomponent tool-key as "mail." (#8255)
This will retrive the correct permissions data from direct but will generate the permissions table with id="mail.-permissions-table". Then, we need to adapt the javascript selector in the savePermissions method to locate this id.

2.- Change PermissionsEntitiyProvider to look for SEARCH + "." (THIS ONE).
I think this is a more elegant solution as it return the "expected result". But ,I don't know if it will affect other tools that trust the old behaviour.

Both PRs are exclusive, choose one or other.

@adrianfish
Copy link
Contributor

Have you searched for other tools that use the direct endpoint? I suspect that there aren't many, but you could get an idea. I'd rather go with this solution and fix the other tools. I don't like the idea of hacking the selector in the component when we can just fix the endpoint.

@frasese
Copy link
Contributor Author

frasese commented Jun 2, 2020

I also prefer to fix the endpoint. And no, there are no more uses of that endpoint.

@frasese
Copy link
Contributor Author

frasese commented Jun 3, 2020

Finally, in "messages" PR (#8265) we have needed to add the hacking selector option, because the tool id includes a dot (msg.permissions).
So, it's up to you to use this option or the first one (#8255) Caution with conflicts, because both PRs apply the same change.
Anyway, I consider this option stills being good.

Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

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

I'm not sure which of these versions we settled on. I'll leave it up to your judgement :)

This looks okay though.

@frasese frasese force-pushed the SAK-43711-2 branch 2 times, most recently from b419fdb to ee6ca51 Compare June 30, 2020 08:32
* Moved properties files from tool to api.
* Merged permissions.properties files into mailarchive.properties.
* Adapted to permissions webcomponent.
* Update PermissionsEntitiyProvider to filter by SEARCH + "."

webcomponents/tool/src/main/java/org/sakaiproject/webcomponents/permissions/PermissionsEntityProvider.java
@frasese
Copy link
Contributor Author

frasese commented Jun 30, 2020

This PR is the good one. The other one was closed in favour of this.

@adrianfish
Copy link
Contributor

Go for it, then :)

@ern ern merged commit 0968308 into sakaiproject:master Jun 30, 2020
@frasese frasese deleted the SAK-43711-2 branch June 30, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants