refinery / refinerycms-authentication-devise

Devise based authentication extension for Refinery CMS
MIT License
17 stars 61 forks source link

Supporting Rails 5 #31

Closed sjoulbak closed 7 years ago

sjoulbak commented 7 years ago

Dear Refinery community 😄 ,

I've been busy with updating this gem, so the authentication will also work with Rails 5. This PR is currently using the feature/rails-5 branch for the dependencies refinerycms and refinerycms-i18n.

I've replaced except! with extract!, because except! will be deprecated in Rails 5.1. This function can be replaced with extract!, because we're only removing the :password and the password_confirmation in exclude_password_assignment_when_blank! from the user params.

The feature spec https://github.com/refinery/refinerycms-authentication-devise/blob/master/spec/features/refinery/authentication/devise/sessions_spec.rb#L119 will probably fail. This is caused by the new RefineryCMS UI, which is only less than half implemented. Maybe it's an idea that we'll remove this UI, because it's been 1,5 year and the UI isn't still fully implemented in Refinery? I'll create a special PR for this in the RefineryCMS gem, so you guys may make a call about it.

bricesanchez commented 7 years ago

Thanks for your contribution @sjoulbak!

I agree with you to revert new admin UI at this time in order to be able to easily ship a new version of Refinery and extensions with Rails 5 support.

After the Rails 5 support we could rework on the new admin UI.

Thoughts @parndt and @stefanspicer ?

parndt commented 7 years ago

I've merged your other PR for the UI 😄

sjoulbak commented 7 years ago

Thank you Philip for approving the PR 😄

I think that we should wait merging this PR, because we're still using the feature/rails-5 branch here in the Gemfile. I'll update the Gemfile when that PR is merged.

parndt commented 7 years ago

👍 thanks for that @sjoulbak

sjoulbak commented 7 years ago

@parndt Do you agree with: https://github.com/sjoulbak/refinerycms-authentication-devise/pull/1 ?

parndt commented 7 years ago

@sjoulbak sure, but you'll need to open the PR on this repo for me to merge it 😄

sjoulbak commented 7 years ago

Haha I only wanted your approvement 😉 It's merged into this branch

bricesanchez commented 7 years ago

Closed in favor of #34