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

Update templates #786

Merged
merged 34 commits into from Feb 25, 2019
Merged

Update templates #786

merged 34 commits into from Feb 25, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 20, 2019

This will probably require version bumping to 1.8.0

test/rails_app/app/views/admins/new.html.erb Outdated Show resolved Hide resolved
test/rails_app/app/views/free_invitations/new.html.erb Outdated Show resolved Hide resolved
test/rails_app/app/views/users/invitations/new.html.erb Outdated Show resolved Hide resolved
Copy link
Owner

@scambra scambra left a comment

Choose a reason for hiding this comment

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

I'm not sure about copying devise partial here, it would have to keep sync'ed to devise file if they make changes.

I rather updating required devise version, changing devise_invitable to 1.8.0, or adding a helper which checks for devise_error_messages! method or devise version, to call helper or render devise partial

@ghost
Copy link
Author

ghost commented Feb 21, 2019

Yeah, keeping partial in sync with devise is less than ideal, but they will probably remove devise_error_messages! in the next major release (4.7.0) so I don't see any other options here 😟

Your first solution (upgrading minimum devise version, bumping devise_invitable to 1.8.0) seems more sensible to me. I think fiddling with helper is too much work and is error-prone.

@scambra
Copy link
Owner

scambra commented Feb 21, 2019

Helper could be:

def devise_invitable_error_messages
  if Devise::VERSION >= '4.6'
    render "devise/shared/error_messages", resource: resource 
  else
    devise_error_messages!
  end
end

Although if Devise releases many 4.x versions it would start failing with 4.10.
Also, it could check respond_to?(:devise_error_messages!) && Devise::VERSION =~ /^4.6./

Anyway, releasing 1.8.0 without copying partial and require devise >= 4.6 is good enough to me. If some new change is added to devise invitable, people would have to upgrade devise to latest, but it's not hard, it may be recommended for safety, and I think devise 4.6 supports same rails versions as older 4.x devise versions

@ghost
Copy link
Author

ghost commented Feb 21, 2019

If I understood you correctly, you suggest removing shared partial so that devise_invitable inherits the one that comes with devise 4.6+? That actually was my initial idea, but then I inadvertently broke 15 integration tests, so that's why I decided to include the partial.

@scambra
Copy link
Owner

scambra commented Feb 21, 2019

Was it testing against devise 4.6 when integration tests were broken?

@ghost
Copy link
Author

ghost commented Feb 21, 2019

Nope, it was devise, '~> 4.0'. With devise 4.6, it seems fine.

Copy link
Owner

@scambra scambra left a comment

Choose a reason for hiding this comment

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

Remove this partial, change Gemfile and gemspec to require devise 4.6

Gemfiles testing against older versions should be removed, so remove all gemfiles/Gemfile.devise* files, and update other gemfiles to test against devise 4.6, and travis.yml needs to be changed to remove deleted gemfiles

@ghost
Copy link
Author

ghost commented Feb 21, 2019

You can merge this branch already and I will send you new PR to delete it. If I delete it from here, the build will break.

See #785 regarding support for devise 4.6.

@scambra
Copy link
Owner

scambra commented Feb 22, 2019

I have merged #785, you may want to rebase against master

@ghost
Copy link
Author

ghost commented Feb 22, 2019

I have removed a shared partial, prepared a changelog for 2.0.0 (since this release will be backwards-incompatible), and fiddled with the Travis-CI config. Take a look, but don't merge it yet: I want to update some dependencies. Will let you know when I'm done.

@scambra, if it looks good to you, then squash all commits and merge it.

Don't forget to:

  • update the gem version to 2.0.0
  • tag it with v2.0.0
  • publish it to rubygems 🚀 🚀

Cheers!

@scambra scambra merged commit 2616684 into scambra:master Feb 25, 2019
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.

None yet

2 participants