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

Jira Cloud user support #1109

Merged
merged 5 commits into from
Oct 24, 2021
Merged

Jira Cloud user support #1109

merged 5 commits into from
Oct 24, 2021

Conversation

adehad
Copy link
Contributor

@adehad adehad commented Jul 29, 2021

  1. Using allow testing of Jira Cloud #1107 I have been able to test a fix for supporting accountId in the User class. Two tests were updated to assert that the Jira Cloud users were returned as User objects:
    • The test related to .user() method of the JIRA class - i.e. testing obtaining a known user
    • The test related to .search_allowed_users_for_issue() method of the JIRA class - i.e. testing a paginated endpoint
    • The test related to .search_users() method of the JIRA class - i.e. adding tests to a commonly used function

Fixes #1060 #971

Replaces #1106

NOTE: This is pointing at #1107 temporarily to make the review small, will eventually directly target the main branch

@adehad adehad mentioned this pull request Jul 29, 2021
1 task
@adehad adehad changed the title jira cloud user support Jira Cloud user support Jul 29, 2021
@adehad adehad changed the base branch from master to feature/test_jira_cloud July 29, 2021 21:24
@adehad adehad marked this pull request as draft July 29, 2021 21:34
@adehad adehad requested a review from studioj July 29, 2021 21:35
@adehad adehad force-pushed the feature/jira_cloud_user_support branch from 962849c to dc4c987 Compare July 29, 2021 21:43
studioj
studioj previously approved these changes Aug 28, 2021
Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

everything seems understandable and correct...

what worries me architecturally is that maybe we should split up the cloud vs the server part into two classes ...
this seems like a patch for a much larger problem... (i'm guessing more problems will pop up)

I'll check out your code to play around a little bit (y)

I'd be willing to help contribute to this refactoring... (but we should plan it a bit)

@adehad
Copy link
Contributor Author

adehad commented Aug 28, 2021

everything seems understandable and correct...

what worries me architecturally is that maybe we should split up the cloud vs the server part into two classes ...
this seems like a patch for a much larger problem... (i'm guessing more problems will pop up)

I'll check out your code to play around a little bit (y)

I'd be willing to help contribute to this refactoring... (but we should plan it a bit)

Agreed, for a 4.0 or other major version release I think it makes sense to revisit how we do this and implement a more scalable solution

@adehad adehad linked an issue Sep 18, 2021 that may be closed by this pull request
@adehad adehad force-pushed the feature/jira_cloud_user_support branch from dc4c987 to 2f1acbc Compare October 4, 2021 18:23
@adehad adehad removed the feature label Oct 4, 2021
@adehad adehad force-pushed the feature/jira_cloud_user_support branch from 2f1acbc to 96f4225 Compare October 24, 2021 10:15
@adehad adehad marked this pull request as ready for review October 24, 2021 10:23
Base automatically changed from feature/test_jira_cloud to main October 24, 2021 14:21
@adehad adehad force-pushed the feature/jira_cloud_user_support branch from 96f4225 to f791cc1 Compare October 24, 2021 14:22
@adehad adehad merged commit e68bf79 into main Oct 24, 2021
@adehad adehad deleted the feature/jira_cloud_user_support branch October 24, 2021 14:40
@adehad adehad linked an issue Oct 30, 2021 that may be closed by this pull request
@adehad adehad linked an issue Oct 30, 2021 that may be closed by this pull request
svermeulen pushed a commit to svermeulen/jira that referenced this pull request Oct 31, 2021
* test_user allow_on_cloud, with appropriate fixes to jira.resources

* update test_generic_resource

* update search_allowed_users_for_issue() for Jira Cloud

* add a search function to the Jira Cloud tests

* add test_search_users() to allow_on_cloud
@kot0dama
Copy link

Hi,

I think delete_user() also needs an update as it's not working with emails anymore on JIRA Cloud.

I got a workaround for now:

  • fetch the user's accountId through search_user()
  • pass it to delete_user() along with the username (something like "{username}&accountId={accountId}")

Let me know if I can help in any way!

Thank you,
Loïc

@adehad
Copy link
Contributor Author

adehad commented Nov 15, 2021

Hi @kot0dama it would be great if you could raise an issue for this - (I would use the bug template). Would also be great for you to include the workaround you have in case this is useful for anyone else in the meantime.

We are happy to review any pull requests you have too.

If you do want to take a crack at some code this kind of behaviour we currently handle with a self_is_cloud property

So perhaps we could do something like "{'accountId' if self._is_cloud else 'username'}={username}"
We can then specify in the docstring that the username argument should be the accountId in Jira Cloud.

@kot0dama
Copy link

Hi @adehad, here's the bug report including my workaround: #1241
I'll followup on your comment there !

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants