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

First Pass at a Bulk Reassign #24

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BrettBSigma
Copy link
Contributor

No description provided.

@BrettBSigma BrettBSigma requested a review from ocawthorne October 2, 2024 12:53
Copy link

@ocawthorne ocawthorne left a comment

Choose a reason for hiding this comment

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

Just a few suggestions for improvement - I'll hold off on testing just in case you want to take on some of the more major suggestions detailed in the comments

bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
try:
response = client.patch(
f"v2/files/{file_id}",
json={"ownerId": user_id}

Choose a reason for hiding this comment

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

Without a parentId, what happens with workbooks and unsaved explorations? Would these all be dumped into someone's "My documents" section? (I assume if the workbooks are in a workspace, then they won't be moved)

Could it be better to allow the end user to optionally designate a folder into which these reassigned files will go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added optional arg for this

Copy link

@ocawthorne ocawthorne left a comment

Choose a reason for hiding this comment

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

LGTM - in summary, there's just a few minor camel-to-snake cases and validating that the destination folder is owned by the intended owner, but other than that, all clear (spare for testing) - will get ready to test soon.

bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved

"""
updateFileBody={"ownerId": user_id}
if folderID:

Choose a reason for hiding this comment

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

We should probably also verify that the owner of the destination folder matches the new_owner_id

Choose a reason for hiding this comment

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

We should probably throw an error and abort entirely if the owner of the folder doesn't match the new owner's ID actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after some back and forth on this, i think that we shouldnt need the folder owner to be match the person (what if i put it in a team folder for example). That said, I did validate that the ID is type folder
image

bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
bulk_reassign_docs.py Outdated Show resolved Hide resolved
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.

2 participants