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

[SYNPY-1186] When a username is a number, getUserProfile cannot retrieve the user #1014

Merged

Conversation

BWMac
Copy link
Contributor

@BWMac BWMac commented Nov 16, 2023

Problem:

When a user calls the getUserProfile method with a username that is a number, the appropriate UserProfile is not retrieved. This essentially boils down to the fact that we currently allow people to pass userNames and ownerIds to getUserProfile, and that people are allowed to create usernames on Synapse that are just a string of numbers.

Solution:

Split getUserProfile into two separate functions. One that accepts userName as a string only, and one that only will accept an ownerId in the form of an integer.

A complication of this solution is that getUserProfile is used in a number of places, some of which also allow userNames or ownerIds. This refactoring and the eventual deprecation of getUserProfile will need to be done in a future PR.

@BryanFauble
Copy link
Contributor

I like the approach you are taking on this - While looking for areas/dependent functions let's take the time to update the parameter section of those function(s) docstrings to be clear that typing of the data they're passing in matters and what we'll do with a string vs int

Copy link
Contributor

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 Let's hold on the deprecation warning because we don't want it popping up every time someone uses the functions that leverages getUserProfile.

We will need to create separate tickets for the deprecation and also create platform tickets for not allowing integer usernames.

synapseclient/client.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Nov 20, 2023

Hello @BWMac! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-20 21:07:56 UTC

@BWMac BWMac marked this pull request as ready for review November 20, 2023 18:53
@BWMac BWMac requested a review from a team as a code owner November 20, 2023 18:53
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

Excellent - here's some feedback.

synapseclient/client.py Outdated Show resolved Hide resolved
synapseclient/client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🚀 LGTM!

@BWMac BWMac merged commit 677d1ec into develop Nov 21, 2023
35 checks passed
@BWMac BWMac deleted the bwmac/SYNPY-1186/support_int_usernames_for_get_user_profile branch November 21, 2023 15:58
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.

None yet

4 participants