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

Fix name mismatch between omniauth strategy and callback. #123

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

coezbek
Copy link

@coezbek coezbek commented Oct 17, 2024

Omniauth google didn't work out of the box, because callback method was misnamed.

@coolprobn
Copy link
Collaborator

coolprobn commented Oct 20, 2024

Hi @coezbek, thanks for contributing.

I haven't used this gem and generator myself but going through the installation setup here, it seems the controller action should be google_oauth2 instead of what you have changed to, which could possibly be a typo google_auth2 , can you take a look?

image

Also apart from the code, can you address other things as per the Contribution Guidelines? I will highlight the important ones here:

[ ] Update tests to ensure generator is working correctly after the change
[ ] Manual test in the actual Rails app to ensure your changes are working as intended
[ ] Update Changelog and add an entry for this change

Note: don't forget to add yourself to contributor list at the bottom of the doc first before referencing your username in the Changelog

Here is an example PR you can take reference from https://github.com/abhaynikam/boring_generators/pull/113/files

Thanks for your time and please don't hesitate to reach out to me here in the reply if you need help with anything to get this PR to the finish line.

@coezbek
Copy link
Author

coezbek commented Oct 20, 2024

@coolprobn Hey thanks for the review! I fixed this again in the branch and updated the changelog.

I don't know where to begin to test the omniauth flow. The existing tests are pretty much static (https://github.com/abhaynikam/boring_generators/blob/main/test/generators/oauth/google_install_generator_test.rb)

@coolprobn
Copy link
Collaborator

coolprobn commented Oct 21, 2024

Hi,
I took a closer look and tested this generator in the actual app today and found some issues we need to handle and improvements we can make.

Would you mind taking a look at these so this PR can be taken to the finish line?

  def google_oauth2
      # You need to implement the method below in your model (e.g. app/models/user.rb)
      @user = User.from_omniauth(request.env['omniauth.auth'])

      if @user.persisted?
        flash[:notice] = I18n.t 'devise.omniauth_callbacks.success', kind: 'Google'
        sign_in_and_redirect @user, event: :authentication
      else
        session['devise.google_data'] = request.env['omniauth.auth'].except('extra') # Removing extra as it can overflow some session stores
        redirect_to new_user_registration_url, alert: @user.errors.full_messages.join("\n")
      end
  end
  • Remove failure action from the controller since it's not adding anything extra to what's already provided by the google omniauth gem

  • Link @coezbek to the GitHub profile at the end of the Changelog file. If you are not seeing anything at the end then you might want to see raw version of the MD file that is editable

image

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.

2 participants