mhfs/devise-async

Support for Devise 4.x

Closed this issue · 19 comments

arturoherrero commented

I'm upgrading Devise to 4.x version in one of my Rails app and I was surprised that devise-async downgrade the version.

-    devise (3.5.10)
+    devise (4.1.1)
       bcrypt (~> 3.0)
       orm_adapter (~> 0.1)
-      railties (>= 3.2.6, < 5)
+      railties (>= 4.1.0, < 5.1)
       responders
-      thread_safe (~> 0.1)
       warden (~> 1.2.3)
-    devise-async (0.10.1)
-      devise (~> 3.2)
-    devise-token_authenticatable (0.4.6)
-      devise (>= 3.5.2, < 4.0.0)
+    devise-async (0.7.0)
+      devise (>= 2.2)
+    devise-token_authenticatable (0.5.0)
+      devise (>= 4.0.0, < 4.2.0)

I see a branch with some work related with Devise 4.x. I don't know if this branch needs too much work or it's ready to be merged. Let me know if I can help.

baschtl commented

Hey @arturoherrero, in this branch I started with the work for the support of devise 4.x. As noted in the Changelog the current version of devise-async will only support devise 3.

As of now, there is one failing spec when using devise 4 and I did not have the time to deep dive into it. Feel free to have a look into it. Help is much appreciated!

arturoherrero commented

I've done a quick test and it seems it's not calling to send_devise_notification.

tmaier commented

Just a thought: Could this be drastically simplified by relying on ActiveJob?
https://github.com/plataformatec/devise#activejob-integration

  • 👍9
baschtl commented

This issue - the failing spec - was introduced by a change in devise (first bullet point). I looked into it and with the current test setup - the test running within a transaction - it is not possible to solve this problem as far as I can tell. Here are my thoughts:

  • send_devise_notification is not called as devise now uses an after_commit instead of after_create,
  • the test runs within a transaction which means that the confirmation email is not being sent as after_commit is not called within an open transaction,
  • even if you remove the test being wrapped within a transaction, changed? is false as the record has already been saved when after_commit gets triggered,
  • you could use previous_changes.any? which is available within after_commit, however, this leads to a lot of other tests failing.

I do not have the time to dive deeper and find a solution for this. If you need a similar means to send notifications delayed in devise 4+ I strongly advise to switch to ActiveJob for Rails 4.2+. If you need the functionality of this gem for using devise 4 in Rails < 4.2 you probably need to help me out on this.

  • 👍2
alagos commented

@tmaier awesome.
That worked great, and with no need of an aditional gem.

LocutusOfBorg commented

Can anybody please share the patch?

LocutusOfBorg commented

I would like to test a possible patch, this issue is preventing the new gitlab from going in Debian and Ubuntu next releases

giedriusr commented

Any news when support for Devise 4 will be available?

  • 👍1
LocutusOfBorg commented

gitlab is now devise-async free, so there is no hurry anymore

baschtl commented

@giedriusr Please have a look at a past comment of mine: #94 (comment).

giedriusr commented

@baschtl thanks, will follow these suggestions.

swapnilchincholkar commented

@baschtl Will code changes & test cases under a303ee9 suffice OR we should consider few more scenario's/changed within it?

Nowaker commented

Howdy @baschtl, any news about support for Devise 4 in devise-async? Thanks.

baschtl commented

Hey @Nowaker, I commented on #98 but haven't got any feedback. Feel free to have a look at it. I guess I have to make the changes myself at some point.

giedriusr commented

No need of this gem after following steps at: #94 (comment)

Nowaker commented

Hey @baschtl and @giedriusr, thanks for your response. Here's my thoughts what could be done with this gem.

No need of this gem

As an ordinary Devise user, it makes more sense to me to simply depend on devise-async gem that gets the job done. The gem knows what method to override, what to do with each parameter, etc.

When I have my own def send_devise_notification(notification, *args) within models, I have to follow Devise implementation details to match my implementation with theirs.

Here's the solution I would suggest:

If you need a similar means to send notifications delayed in devise 4+ I strongly advise to switch to ActiveJob for Rails 4.2+ within devise-async and drop support for Rails <4.2

The gem would become almost a one-liner gem that does only this. This way, when a developer migrates to Rails 5, all they do is bump devise-async along the way.

We would release a new version, 0.10.3, which would only restrict Rails to < 5. And then another version 1.0.0 that requires Rails >= 5.

Your thoughts?

baschtl commented

@Nowaker That sounds like a great idea! I will try to set something up.

baschtl commented

Please try out https://github.com/mhfs/devise-async/releases/tag/v1.0.0beta of this gem and give me feedback on how it works in connection with devise 4.x.

This release implements the suggestion of @Nowaker. Which means you need to set up ActiveJob in your Rails app.

baschtl commented

Closing this as the latest release depends on ActiveJob and implements the suggestion discussed in the last comments.

  • 👍1