search

solidusio-contrib / solidus_social

Building block for social authentication in your Solidus store.
BSD 3-Clause "New" or "Revised" License
27 stars 52 forks source link

Fix gem for rails 7 #102

Closed piyushswain closed 2 years ago

piyushswain commented 2 years ago

This PR aims to fix this gem for Rails 7 and latest Solidus.

waiting-for-dev commented 2 years ago

Ref: https://github.com/solidusio-contrib/solidus_social/pull/102#discussion_r938726743

I'd remove both omniauth-amazon & omniauth-twitter from solidus_social, as they are the only integration that doesn't look like maintained anymore. Thoughts @aldesantis, @kennyadsl?

kennyadsl commented 2 years ago

@waiting-for-dev that makes sense, especially if they are blocking us from upgrading this extension.

waiting-for-dev commented 2 years ago

@waiting-for-dev that makes sense, especially if they are blocking us from upgrading this extension.

Yeah, we just need to make sure we release a new major. @piyushswain, we can do it as part of this PR, if you like.

piyushswain commented 2 years ago

@waiting-for-dev that makes sense, especially if they are blocking us from upgrading this extension.

Yeah, we just need to make sure we release a new major. @piyushswain, we can do it as part of this PR, if you like.

Okay, so I will remove omniauth-twitter and omniauth-amazon from the gem so that we can avoid the multi-json error.

waiting-for-dev commented 2 years ago

@piyushswain, it looks like tests are failing

piyushswain commented 2 years ago

@waiting-for-dev The tests pass on my development environment but a few tests fail on the CI with the following error

Webdrivers::BrowserNotFound:
            Failed to find Chrome binary.

All the failing tests have the same error.

I'm not sure if this is an issue in the CI or something to do with the gem itself.

waiting-for-dev commented 2 years ago

@waiting-for-dev The tests pass on my development environment but a few tests fail on the CI with the following error

Webdrivers::BrowserNotFound:
            Failed to find Chrome binary.

All the failing tests have the same error.

I'm not sure if this is an issue in the CI or something to do with the gem itself.

@piyushswain, CircleCI images changed and now you need to require the browser tools manually. I think you only need to do the same that this commit:

https://github.com/solidusio/solidus_auth_devise/commit/e2503aa5b83c2448d686b3d7e8e7a87e1628877a

You can use latest v1.4, though.

waiting-for-dev commented 2 years ago

Cool! We've fixed the CircleCI issue. There're still some failures. Which version of Rails did you use locally for testing? If it was minor than 7, probably there's more to fix to make it compatible with Rails 7.

piyushswain commented 2 years ago

Cool! We've fixed the CircleCI issue. There're still some failures. Which version of Rails did you use locally for testing? If it was minor than 7, probably there's more to fix to make it compatible with Rails 7.

Tests run on the Solidus master branch on my local environment, so Rails 7 I am currently trying to fix the issues which are showing up on the CI

piyushswain commented 2 years ago

Cool! We've fixed the CircleCI issue. There're still some failures. Which version of Rails did you use locally for testing? If it was minor than 7, probably there's more to fix to make it compatible with Rails 7.

Hey @waiting-for-dev, I just noticed, the sign_in_specs related to facebook have been failing since today morning even on the master branch for solidus which were previously passing on local as well. Investigating on the issue now.

waiting-for-dev commented 2 years ago

Thanks for your continuous work on it, @piyushswain. We're almost there! I left a suggestion, as we can update the browser tools versions.

Given that omniauth-facebook is forcing us to downgrade the support for omniauth, I'd also remove that provider. As we already support adding any provider manually, users can still easily do it by themselves. In fact, I think we should probably avoid adding any integration OOTB, as we're adding dependencies that some users won't need. But that's out of the scope of this PR.

Finally, we also need to update the README with the changes.

piyushswain commented 2 years ago

Thanks for your continuous work on it, @piyushswain. We're almost there! I left a suggestion, as we can update the browser tools versions.

Given that omniauth-facebook is forcing us to downgrade the support for omniauth, I'd also remove that provider. As we already support adding any provider manually, users can still easily do it by themselves. In fact, I think we should probably avoid adding any integration OOTB, as we're adding dependencies that some users won't need. But that's out of the scope of this PR.

Finally, we also need to update the README with the changes.

It's not that omniauth-facebook is forcing us to downgrade the omniauth version. The omniauth version was 1.9.1 before this PR as well. omniauth-facebook isn't allowing us to update the omniauth version though.

But yes, we could remove it in another PR and users can add it later by themselves.

waiting-for-dev commented 2 years ago

It's not that omniauth-facebook is forcing us to downgrade the omniauth version. The omniauth version was 1.9.1 before this PR as well. omniauth-facebook isn't allowing us to update the omniauth version though.

But yes, we could remove it in another PR and users can add it later by themselves.

Well, yeah, but I mean that it was open in the gemspec, and now we're requiring it to be lesser than v2. I'll definitely remove facebook instead of forcing users to use old versions of omniauth. It's not that they need to create a PR here, they can add facebook only in their apps if they need it. See https://github.com/solidusio-contrib/solidus_social#other-oauth-providers

DanielePalombo commented 2 years ago

It's not that omniauth-facebook is forcing us to downgrade the omniauth version. The omniauth version was 1.9.1 before this PR as well. omniauth-facebook isn't allowing us to update the omniauth version though. But yes, we could remove it in another PR and users can add it later by themselves.

Well, yeah, but I mean that it was open in the gemspec, and now we're requiring it to be lesser than v2. I'll definitely remove facebook instead of forcing users to use old versions of omniauth. It's not that they need to create a PR here, they can add facebook only in their apps if they need it. See https://github.com/solidusio-contrib/solidus_social#other-oauth-providers

@waiting-for-dev I have opened an issue to remove omniauth-facebook and the commit that locks the version. Is there something else to do with this PR? Otherwise, can you review it again so we can merge it? 🙏

Thank you for your support.

waiting-for-dev commented 2 years ago

Hey @DanielePalombo. No, we can merge it. But it'd be nice to remove omniauth-facebook before releasing the new version of it.