Avoid using meta-programming for solving code duplication. #1457
Conversation
|
|
| private | ||
|
|
||
| def callback_for(provider) | ||
| raise '9999999999999999999999' |
There was a problem hiding this comment.
Looks like you left a raise.
There was a problem hiding this comment.
@abraham Yes this is intentional, tests don't work locally for me.
I wanted to know which tests were covering this line, so I created this commit and let the CI run the specs.
For my surprise nothing break, I'll create a test for covering these lines before refactoring.
The PR is not ready for review yet.
There was a problem hiding this comment.
Might I suggest a descriptive error in the future like raise 'Checking if run on CI' or similar.
This might be difficult to test with its reliance on external services.
This change make code easier to change in the future.
delbetu
changed the title
|
After extracting the method out from the |
| def callback_for(provider) | ||
| cta_variant = request.env["omniauth.params"]["state"].to_s | ||
| @user = AuthorizationService.new(request.env["omniauth.auth"], current_user, cta_variant).get_user | ||
| if @user.persisted? && @user.valid? |
There was a problem hiding this comment.
Extracting the if and elsif conditions into their own methods like if persisted_and_valid and elsif persisted_but_username_taken or something like that would clean this up and solve this CodeClimate issue.
There was a problem hiding this comment.
@benhalpern thanks for the suggestion.
pr-triage
bot
added
PR: reviewed-approved
pr-triage
bot
added
PR: merged
What type of PR is this? (check all applicable)
Description
Small refactor avoid using meta-programming for solving code duplication.
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?