-
Notifications
You must be signed in to change notification settings - Fork 47
Add assign batch action SDK #765
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
Conversation
Nathanjp91
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Shouldn't be any issues.
| dataset_ids: int | list[int], | ||
| assignee_id: int, | ||
| workflow_id: str, | ||
| filters: dict[str, UnknownType], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a datatype for this, JSONDict, which is basically a wrapper for dict[str, Any]. Nothing huge but good to keep consistent. It's in future data_objects I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copy-pasted from the set_stage, but sure, I'll change it here.
owencjones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
| """ | ||
| Assign a user to all items matched by filters. | ||
| Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this parses with the doc generation tool.
|
|
||
| def assign(self, assignee_id: int, workflow_id: str | None = None) -> None: | ||
| if not assignee_id: | ||
| raise ValueError("Must specify assignee to assign items to") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't need this to be a ValueError, this could be an assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urgh, I just merged this before I got email about your comments :(
But this code is copied from other similar functions, so that keeps it consistent with them anyway.
This PR adds "assign" batch action. It's largely copy-pasted "set_state" as it works the same way, just with users, not stages. It doesn't support "User" object as input, as I'm pretty sure we don't have one yet.
Example test: