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

Correct variable name and remove unneeded #261

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kentarohorie
Copy link
Contributor

@kentarohorie kentarohorie commented Nov 19, 2020

I want to add changing about two points. It's simple refectoring.

  • Change variable name in add_provider_to_user because it was wrong.
  • Make add_provider_to_user's return simple because it was unneeded.

@kentarohorie kentarohorie changed the title Change/variable name Correct variable name and remove unneeded Nov 19, 2020
Copy link
Member

@joshbuker joshbuker left a comment

Choose a reason for hiding this comment

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

Needs some housekeeping stuff, but the code change itself looks good.

Comment on lines +99 to +103
authentication = send(authentications).build(sorcery_config.provider_uid_attribute_name => uid,
sorcery_config.provider_attribute_name => provider)
user.sorcery_adapter.save(validate: false)
authentication.sorcery_adapter.save(validate: false)
else
user = false
false
Copy link
Member

Choose a reason for hiding this comment

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

Actually, looking closer at this, I think this changes the return value when the authentication is present. Previously it would return authentication, but with this change it would instead return authentication.sorcery_adapter.save(validate: false) as that's the last value called in the method.

Can you revert the second part of this, and keep it to just renaming the variable to be more accurate?

@joshbuker joshbuker added the to be implemented in v1 This issue or pull request will be resolved in the v1 rework, but has not yet been completed. label Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be implemented in v1 This issue or pull request will be resolved in the v1 rework, but has not yet been completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants