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

Improve setUser functionality #151

Merged
merged 3 commits into from
Aug 17, 2022

Conversation

izsi-salmon
Copy link
Contributor

@izsi-salmon izsi-salmon commented Aug 16, 2022

This PR makes some improvements to the SetUser method. We've had a couple users confused as to why their users aren't being set properly and this is likely due to it not being clear in our documentation that setUser expects the user parametre to be a string, otherwise the user will be set to anonymous. I've updated the provider to accept numbers for this parametre now.

Changes:

  • The user parameter now expects both strings and number data types – the is_string and is_numeric checks just make sure we're not getting any unexpected data types

robbieaverill
robbieaverill previously approved these changes Aug 16, 2022
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@izsi-salmon
Copy link
Contributor Author

UPDATED: I've reverted the changes to user details only being set when the customer isn't anonymous. It's been brought to my attention that the other providers also allow you to set user details on an anonymous customers, so for consistency's sake I will keep this the same.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@izsi-salmon izsi-salmon merged commit 6408b96 into master Aug 17, 2022
@izsi-salmon izsi-salmon deleted the is/APL-327/improve-setuser-functionality branch August 17, 2022 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants