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

[SYNPY-1302] Replace getPermission with get_acl and add new get_permissions #1037

Merged
merged 17 commits into from
Dec 29, 2023

Conversation

danlu1
Copy link
Contributor

@danlu1 danlu1 commented Dec 26, 2023

Problem:

  1. The original getPermission method uses /acl endpoint only that shows inconsistent behavior in the case of publicly downloadable content from Synapse.

Solution:

  1. create a get_permissions method that implements GET /entity/{id}/permissions API. These API factors in the entity's ACL and the user's group membership when determining the caller's permission to the entity.
  2. rename getPermission with get_acl indicating it extracts ACL information only
  3. Deprecate getPermission

Example:
Screenshot 2023-12-15 at 9 31 28 AM
Screenshot 2023-12-26 at 2 56 46 PM

Testing:
Unit tests and integration tests are included in the PR.

@danlu1 danlu1 requested a review from a team as a code owner December 26, 2023 22:57
@pep8speaks
Copy link

pep8speaks commented Dec 26, 2023

Hello @danlu1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 2:89: E501 line too long (90 > 88 characters)
Line 12:89: E501 line too long (89 > 88 characters)
Line 22:89: E501 line too long (110 > 88 characters)
Line 23:89: E501 line too long (114 > 88 characters)
Line 25:89: E501 line too long (97 > 88 characters)
Line 29:89: E501 line too long (106 > 88 characters)
Line 30:89: E501 line too long (118 > 88 characters)
Line 32:89: E501 line too long (113 > 88 characters)
Line 34:89: E501 line too long (112 > 88 characters)
Line 38:89: E501 line too long (119 > 88 characters)
Line 39:89: E501 line too long (119 > 88 characters)
Line 59:89: E501 line too long (103 > 88 characters)
Line 74:89: E501 line too long (93 > 88 characters)
Line 81:89: E501 line too long (118 > 88 characters)
Line 84:89: E501 line too long (93 > 88 characters)
Line 90:89: E501 line too long (111 > 88 characters)
Line 93:89: E501 line too long (93 > 88 characters)
Line 138:89: E501 line too long (96 > 88 characters)
Line 155:89: E501 line too long (118 > 88 characters)

Line 620:89: E501 line too long (98 > 88 characters)
Line 647:89: E501 line too long (96 > 88 characters)
Line 713:89: E501 line too long (106 > 88 characters)
Line 797:89: E501 line too long (110 > 88 characters)
Line 884:89: E501 line too long (90 > 88 characters)
Line 910:89: E501 line too long (114 > 88 characters)
Line 933:89: E501 line too long (90 > 88 characters)
Line 934:89: E501 line too long (93 > 88 characters)
Line 940:89: E501 line too long (112 > 88 characters)
Line 959:89: E501 line too long (95 > 88 characters)
Line 963:89: E501 line too long (123 > 88 characters)
Line 967:89: E501 line too long (103 > 88 characters)
Line 1005:89: E501 line too long (122 > 88 characters)
Line 1024:89: E501 line too long (99 > 88 characters)
Line 1033:89: E501 line too long (99 > 88 characters)
Line 1037:89: E501 line too long (123 > 88 characters)
Line 1042:89: E501 line too long (105 > 88 characters)
Line 1057:89: E501 line too long (92 > 88 characters)
Line 1088:89: E501 line too long (115 > 88 characters)

Comment last updated at 2023-12-29 16:30:09 UTC

@thomasyu888 thomasyu888 changed the title [SYNPY-1302]Replace getPermission with get_acl and add new get_permissions [SYNPY-1302] Replace getPermission with get_acl and add new get_permissions Dec 27, 2023
@BryanFauble
Copy link
Contributor

Excellent work! Thank you for incorporating the handful of suggested changes. There are a few more I added around documentation and some code smells that SonarCloud flagged.

docs/reference/core.md Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
::: synapseclient.models.permission
Copy link
Contributor

Choose a reason for hiding this comment

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

I committed 1 additional change here - Please review and let me know your thoughts: 315eb98

The reason I added it is because we have a docstring on each of the attributes, and we have them in the class docstring it ends up duplicating the information in the doc pages. See the screenshots below. Unfortunately because they serve 2 different purposes, See this slack thread, they cannot be combined at the moment.

In this settings I have it setup to not show if no docstring, but again, since we are including it the attributes show up twice.

Before:
image

After:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that we don't want to show duplicate information on the doc. I like your idea and found show_docstring_attributes might also be helpful. Let me test it and circle back.

Copy link
Contributor Author

@danlu1 danlu1 Dec 29, 2023

Choose a reason for hiding this comment

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

@BryanFauble Here is the output of using show_docstring_attributes as False.

  1. If we add show_docstring_attributes as false, nothing except the class description will be shown.
Screenshot 2023-12-29 at 11 12 11 AM 2. If we comment out the member section, we get the attribute and class property (access_types) as paragraph Screenshot 2023-12-29 at 11 13 43 AM Screenshot 2023-12-29 at 11 13 59 AM

I will go with the approach you committed since it will keep the attribute table.

Copy link
Contributor

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

Thanks for completing this.

Give a look over the few commits I added (I took care of some formatting issues I saw since it was an easy fix).

Copy link

sonarcloud bot commented Dec 29, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

7 New issues
0 Security Hotspots
100.0% Coverage on New Code
13.4% Duplication on New Code

See analysis details on SonarCloud

@danlu1 danlu1 merged commit 74d544f into develop Dec 29, 2023
38 checks passed
@BryanFauble BryanFauble deleted the SYNPY-1302-add_get_permissions branch May 8, 2024 18:56
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.

None yet

3 participants