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

SDK updates #56

Merged
merged 11 commits into from
Mar 22, 2024
Merged

SDK updates #56

merged 11 commits into from
Mar 22, 2024

Conversation

passage-beachball-bot
Copy link
Collaborator

No description provided.

CHANGELOG.md Outdated Show resolved Hide resolved
user.go Show resolved Hide resolved
require.Nil(t, err)

createUserBody := passage.CreateUserBody{
Email: RandomEmail,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also be good to see a test case that captures a phone number as the identifier

Copy link
Contributor

Choose a reason for hiding this comment

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

created extra test

require.Nil(t, err)
assert.Equal(t, RandomEmail, user.Email)

userByIdentifier, err := psg.GetUserByIdentifier(RandomEmail)
Copy link
Contributor

@ctran88 ctran88 Mar 22, 2024

Choose a reason for hiding this comment

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

RandomEmail only generates lowercase letters.

email := fmt.Sprintf("%X@email.com", randomChars)
return strings.ToLower(email)

Another test case that searches for the mixed or uppercase counterpart would be good to make sure that there is no regression when you transform to lowercase inside the GetUserByIdentifier method

Copy link
Contributor

Choose a reason for hiding this comment

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

created specific all uppercase test

@tdeshong tdeshong requested a review from ctran88 March 22, 2024 19:25
user.go Outdated
var errorText string
message := "failed to get Passage User By Identifier"
limit := 1
ilikeIdentifier := "like:(?i)" + identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry for the confusion -- I meant to say that your implementation of ToLower works since we lowercase on the backend. Although this works too, if you prefer to keep the lowercase version because it is an exact matcher vs a regex matcher I would be good with that as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha. I am going to change it back to exact matcher since it is usually more performance than regex matcher. Thanks for the pro tip though! I did not know about the (?i).

user_test.go Outdated
})
require.Nil(t, err)

email := strings.ToUpper(RandomEmail)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: creating the user with an uppercase email and then searching for it with the same value wouldn't necessarily prove that the search is case insensitive. If the ToUpper happened after CreateUser and before or at GetUserByIdentifier instead, it would be more accurate since this client doesn't know that the email is lowercased in the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! They are not suppose to be the same value! Updating this

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tdeshong tdeshong merged commit c096138 into main Mar 22, 2024
2 checks passed
@tdeshong tdeshong deleted the sdk-updates-1711036009 branch March 22, 2024 20:00
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.

3 participants