[SYNPY-1244] Clarify docstrings with delete_permissions function and recursive behavior
#1202
Conversation
…t with recursive flag and update unit tests for validation
| if recursive and not include_container_content: | ||
| raise ValueError( | ||
| "When recursive=True, include_container_content must also be True. " | ||
| "Setting recursive=True with include_container_content=False has no effect." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Just a consideration from a ux perspective:
Had it been clarified that setting recurive=True and include_container_content=False wouldn't have deleted any of the access controls (but that it still iterates through the project and displays the folders), as I user I would have appreciated having the functionality to do a dry run and see everything that would have had access controls removed before actually doing so. @BryanFauble What are your thoughts on not halting execution and rather adjusting the messaging displayed in the case of this combination of parameters? Granted it doesn't look like files themselves are displayed in the current messaging?
[WARNING] Failed to delete ACL for entity syn66520312: 403 Client Error: Cannot restore inheritance for resource which has no parent.
[syn66520312:acc_control_test]: Syncing Project from Synapse.
Syncing from Synapse: 0%| | 0.00/1.00 [00:00<?, ?B/s]
[syn66520313:temp folder]: Syncing Folder from Synapse.
Syncing from Synapse: 0%|
There was a problem hiding this comment.
[WARNING] Failed to delete ACL for entity syn66520312: 403 Client Error: Cannot restore inheritance for resource which has no parent.
This message only shows because you cannot delete the ACL for projects.
Had it been clarified that setting recurive=True and include_container_content=False wouldn't have deleted any of the access controls (but that it still iterates through the project and displays the folders)
This was not clarified, and in the code it would have just skipped over these entries.
as I user I would have appreciated having the functionality to do a dry run and see everything that would have had access controls removed before actually doing so. @BryanFauble What are your thoughts on not halting execution and rather adjusting the messaging displayed in the case of this combination of parameters?
This is a great idea! I created this follow-up ticket https://sagebionetworks.jira.com/browse/SYNPY-1604 to accomplish this ask. We can do quite a bit to make this easier for folks to understand the ACL structure of their project and implications of running the delete.
There was a problem hiding this comment.
Okay that all makes sense, thanks! @BryanFauble
Problem:
recursiveflag implied incorrect behavior of the function when used in conjunction withinclude_container_contentSolution:
Testing: