feat: Add `Permissions.delete(*permissions)` method #339

schloerke · 2024-11-15T20:32:48Z

Fixes: #338
Related: https://github.com/rstudio/connect/issues/28826

Other changes

~~Permission.delete() deprecated for Permission.destroy()~~

github-actions · 2024-11-15T20:33:16Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
1770	1631	92%	0%	🟢

New Files

No new covered files...

Modified Files

File	Coverage	Status
src/posit/connect/permissions.py	97%	🟢
TOTAL	97%	🟢

updated for commit: 11e4730 by action🐍

integration/tests/posit/connect/test_content_item_permissions.py

…existing cookbook)

jonkeane

Thanks for this, a few comments for you.

Would you mind adding more detail (can be only in the comments of the PR) about why we need to deprecate delete and transition to destroy for this? I am not necessarily opposed, but from the code itself it doesn't seem to be required and it would be good to have a papertrail of why we did that.

jonkeane · 2024-11-22T15:35:05Z

integration/tests/posit/connect/test_users.py

+        # aron, bill, cole, and me (and existing user)
+        assert self.client.users.count() == 3 + self.existing_users


Could you say more about why 3 + self.existing_users is preferred to the hard code?

It feels a little off that this blends something from setup with a test here: if count() were to not work at all, that would make the setup always fail. And if we wanted to test that client.users.count(); client.users.create(); client.users.count() increments like we expect, that should be in a test block, right?

In the test file above (integration/tests/posit/connect/test_content_item_permissions.py), I want to use a couple users to test the permissions. So I added them there to have it be self contained. (We could move the users to an earlier location, but enforcing that the users exist for any subset of testing seemed like a big hassle.)

In slack, Aron stated that users can not be deleted from within Connect. So if another user is added in a different test, then the total number of users in this test will be larger than 4 (aron, bill, cole, and me).

It feels a little off that this blends something from setup with a test here

Agreed. But each of the other tests require the three user objects to already exist.

We could move the .create() test to another test class and retrieve the users in this class to avoid crossing setup / testing. Using a subset of tests won't work well if trying to just test this situation but that's ok.

In slack, Aron stated that users can not be deleted from within Connect. So if another user is added in a different test, then the total number of users in this test will be larger than 4 (aron, bill, cole, and me).

nods right right, that's true, but also true if we have 4 there or if we do 3 + self.existing_users, right? We would need to do 4 + self.existing_users or 5 + self.existing_users if we add more users. Or am I missing something?

In this test we only add 3x users: aron, bill, and cole. So only if we add more users within this test will we need to change 3 to a new value.

src/posit/connect/_deprecated.py

tests/posit/connect/api.py

src/posit/connect/permissions.py

Co-authored-by: Jonathan Keane <[email protected]>

jonkeane

Thanks for cleaning this up, I like where this is. A few minor comments, mostly style / docs.

src/posit/connect/permissions.py

jonkeane · 2024-11-26T22:04:06Z

src/posit/connect/permissions.py

+            if isinstance(arg, str):
+                principal_guid = arg


Do we have other facilities for checking if a string is the right shape to be a GUID? That might be nice to check here since someone might try passing client.content.get(content_guid).permissions.destroy("my group name") or the like?

No. We do not have the functionality.

I believe the thought process is to have the server return an error and we display the error.

nods that makes sense. What does the server return as the error in that case? Is it something we expect SDK authors to know how to recover from?

Doing something more than ^^^ is probably scope creep, but I am curious what the ergonomics would be

Should we instead change the approach of this method to always submit the delete API calls to the server? (rather than only the permissions we know exist.)

Then the server will always display an error. And we wouldn't need to do any validation on our end.

AAAH right on line 223 it will silently be ignore if it's anything other than a matching GUID. HMMM I don't have a strong intuition which direction would be better here, pre-checking or always sending and failing. There certainly are complications with either.

Should we instead change the approach of this method to always submit the delete API calls to the server? (rather than only the permissions we know exist.)

For posterity, did you end up implementing this or does it still (silently) ignore non-GUIDs (or really anything that doesn't match a GUID already present if it's a string)?

It still silently ignores non existent permissions

src/posit/connect/permissions.py

tests/posit/connect/test_permissions.py

Co-authored-by: Jonathan Keane <[email protected]>

* main: feat: Add `Permissions.delete(*permissions)` method (#339) ci: Add in recent docker integration version (#347)

schloerke added 7 commits November 15, 2024 10:41

add Permissions.delete() method

f737bc6

add unit tests for permission

e498165

Rename Permission.delete to Permission.destroy

32e5024

Rename Permissions.delete to Permissions.destroy

85fdc1e

Update test with destroyinng user, group, and permission

1b50382

Add check for no permissions to be removed

1119e31

Create test_content_item_permissions.py

a9fbb36

schloerke added enhancement New feature or request sdk Used for automation labels Nov 15, 2024

schloerke self-assigned this Nov 15, 2024

schloerke commented Nov 15, 2024

View reviewed changes

integration/tests/posit/connect/test_content_item_permissions.py Show resolved Hide resolved

schloerke added 3 commits November 15, 2024 15:45

Update docs

9eaf38d

Have tests play nicely with each other

8269629

Do not remove Permission.destroy(), instead deprecate it (usage in …

ecdf800

…existing cookbook)

schloerke marked this pull request as ready for review November 15, 2024 21:04

schloerke requested a review from tdstein as a code owner November 15, 2024 21:04

schloerke added 2 commits November 15, 2024 16:06

Remove impossible commented code

b2cd9d2

Correct outdated name

ba02668

schloerke mentioned this pull request Nov 19, 2024

feat: Add Permission.create(principal:User | Group) support #343

Merged

3 tasks

jonkeane requested changes Nov 22, 2024

View reviewed changes

nealrichardson reviewed Nov 22, 2024

View reviewed changes

src/posit/connect/permissions.py Outdated Show resolved Hide resolved

schloerke and others added 2 commits November 22, 2024 13:06

Apply suggestions from code review

c3261ed

Co-authored-by: Jonathan Keane <[email protected]>

simplify docs. Add note about silently ignoring perms not found

2ce66b3

schloerke mentioned this pull request Nov 22, 2024

How should we deprecate methods? #344

Closed

schloerke added 2 commits November 26, 2024 16:12

Remove deprecated code

debb45a

user clearer name

19f25dd

schloerke requested a review from jonkeane November 26, 2024 21:15

jonkeane approved these changes Nov 26, 2024

View reviewed changes

Apply suggestions from code review

11e4730

Co-authored-by: Jonathan Keane <[email protected]>

schloerke merged commit fed8325 into main Dec 3, 2024
34 checks passed

schloerke deleted the schloerke/338-revoke-content-item-permissions branch December 3, 2024 16:05

schloerke added a commit that referenced this pull request Dec 3, 2024

Merge branch 'main' into schloerke/345-tags

7c1dfce

* main: feat: Add `Permissions.delete(*permissions)` method (#339) ci: Add in recent docker integration version (#347)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `Permissions.delete(*permissions)` method #339

feat: Add `Permissions.delete(*permissions)` method #339

schloerke commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

jonkeane left a comment

jonkeane Nov 22, 2024

schloerke Nov 22, 2024

jonkeane Nov 22, 2024

schloerke Nov 26, 2024

jonkeane left a comment

jonkeane Nov 26, 2024

schloerke Nov 26, 2024

jonkeane Nov 27, 2024

schloerke Nov 27, 2024

jonkeane Nov 27, 2024

jonkeane Dec 3, 2024

schloerke Dec 3, 2024

		# aron, bill, cole, and me (and existing user)
		assert self.client.users.count() == 3 + self.existing_users

feat: Add Permissions.delete(*permissions) method #339

feat: Add Permissions.delete(*permissions) method #339

Conversation

schloerke commented Nov 15, 2024 • edited Loading

Other changes

github-actions bot commented Nov 15, 2024 • edited Loading

☂️ Python Coverage

Overall Coverage

New Files

Modified Files

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Add `Permissions.delete(*permissions)` method #339

feat: Add `Permissions.delete(*permissions)` method #339

schloerke commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading