Remove internal user lookup #1874
Conversation
Also allow the user to provide a User object
- make the `get_user_identifier` method public. - remove the no longer needed `_get_user_id`
This commit allows users to pass a User object to the `add_watcher` and `remove_watcher` methods
|
@adehad this is the PR for what I mentioned on #1855. I did some manual tests for both server and cloud on the instances I have access to, but I didn't quite manage to set up my local environment for the automated tests, I need to spend some more time on that as it seems harder than the JIRA library itself haha |
There was a problem hiding this comment.
In general I like the idea of the merge request and it is the direction I want the library to go moving forward, I don't want to be passing strings or integers around, I much rather they be objects.
I think what we are missing in the orignal implementation was checking if the entered str/int was a valid user. In a sense could we use JIRA.user(id_or_accountId) to first check if the user provided a valid complete ID, if so then use that, otherwise fallback to the search and maybe add a warning to use a more specific search
|
Sorry about the mess with the docs, for some reason my local working copy was showing the document entirely borked, hence why I did the silly thing with the indentations. b0b552e should fix that. |
dimitarOnGithub
had a problem deploying
to
cloud
GitHub Actions
Failure
There was a problem hiding this comment.
just something I noticed that would be pretty critical to his working is removing this decorator that is trying to be helpful but removes the type safety unfortunately.
I thought we had a code coverage action but it isn't seeing the stats from this PR, so I couldn't exactly confirm it wasn't reaching the new code branch
| except Exception as e: | ||
| raise JIRAError(str(e)) | ||
| return self._get_user_identifier(user_obj) | ||
|
|
||
| # non-resource | ||
| @translate_resource_args |
There was a problem hiding this comment.
| @translate_resource_args |
It will be critical for this approach to work to remove this decorator which would have automagically converted the object to a string
There was a problem hiding this comment.
I don't think the decorator touches on the User objects, it seems to only manipulate the Issue in this case
| @@ -2719,36 +2684,38 @@ def watchers(self, issue: str | int) -> Watchers: | |||
| return self._find_for_resource(Watchers, issue) | |||
|
|
|||
| @translate_resource_args | |||
There was a problem hiding this comment.
| @translate_resource_args |
| return self._session.post(url, data=json.dumps(watcher_id)) | ||
| if isinstance(watcher, User): | ||
| watcher = self.get_user_identifier(watcher) | ||
| return self._session.post(url, data=json.dumps(watcher)) | ||
|
|
||
| @translate_resource_args |
There was a problem hiding this comment.
| @translate_resource_args |
dimitarOnGithub
had a problem deploying
to
ack
GitHub Actions
Failure
|
Label error. Requires exactly 1 of: bug, enhancement, major, minor, patch, skip-changelog. Found: |
dimitarOnGithub
had a problem deploying
to
check
GitHub Actions
Failure
- Introduce a new flag at init called 'with_lookup', by default set to True - If set to True, it will emit a warning to the end user at initialization - If set to True, automatic user lookup will be performed for the 'add_watcher', 'remove_watcher' and 'assign_issue' methods
dimitarOnGithub
had a problem deploying
to
ack
GitHub Actions
Failure
for more information, see https://pre-commit.ci
|
@adehad sorry for the delay with this, here's the updated functionality with having a Not sure about the linter errors in the automatic checks, the value provided is a |
This PR removes the internal user lookup functionality performed by the
assign_issue,add_watcherandremove_watchermethods.By doing so, issues reported in #1855 and #1284 should be resolved and #1734 might also be fixed, though further testing is required there. Additionally, this fixes an actual bug that the internal lookup logic introduced, namely the fact that while
_get_user_idmethod would work for 100% exact usernames, it'd still return the first result if it fails to do so, as seen below:Cloud
Server
This is caused by the fallback assignment that returns the first user from the search results if it fails to find an exact match.
It also makes the
_get_user_identifiermethod public and provides the ability to pass aUserresource object to the methods.Going forward, users will have to provide the exact username (for hosted) or accountId (for cloud) in order to perform any of those operations.