Update README to include note on reloading guest_user

[joshmenden]

In the scenario where a User record has a dependent: :destroy association, such as

class User < ApplicationRecord
  devise :database_authenticatable # etc

  has_many :tasks, dependent: :destroy
end

When you implement transfer_guest_to_user like this

class ApplicationController < ActionController::Base
  private

  def transfer_guest_to_user
    guest_user.tasks.update!(user: current_user)
  end
end

You run into the problem where the dependent tasks are still destroyed when the guest_user is destroyed.

TRANSACTION (0.8ms)  BEGIN
Task Destroy (0.8ms)  DELETE FROM "tasks" WHERE "tasks"."id" = $1  [["id", "1d1b92dc-f951-4ec0-8637-b5b96a3a2087"]]
# guest_user id
User Destroy (0.7ms)  DELETE FROM "users" WHERE "users"."id" = $1  [["id", "0902f0d5-3f47-4eb7-8a7b-5539833b7ccd"]]
TRANSACTION (1.0ms)  COMMIT

Because the guest_user in memory that is being destroyed still references the task. This can be solved with a simple reload before destroying the guest_user to make sure that the in-memory record is up to date.

I confirmed that this fix works in my Rails app, but I noticed we use double for mocking in the specs and it wouldn't feel great to just mock all the functionality to test this use case. If you'd like me to set up a factory and persist some records to the Database to write a test for this I'd be happy to, just let me know.

[pacso]

Wouldn't a better solution be for those who need to reload the guest record to do so within their transfer_guest_to_user implementation? Rather than forcing all users of the gem to have a reload which may be unnecessary?

[joshmenden]

@pacso I totally understand where you're coming from. Wouldn't you say that it's pretty common for a User to have some sort of dependent: association, though? And it's a bit surprising to have data destroyed after implementing the gem following the provided steps.

OK so maybe there are 3 paths forward:

1. I can use this PR to update the README to make it clearer that a reload may be necessary
2. We use some sort of reflection on the class and only reload it if there is a dependent association. Something like

run_callbacks :logging_in_#{mapping} do
  if guest_#{mapping}.class.reflect_on_all_associations.any? { |assoc| assoc.options[:dependent].present? }
      guest_#{mapping}.reload
  end
  guest_#{mapping}.destroy unless send(:"skip_destroy_guest_#{mapping}")
  session[:guest_#{mapping}_id] = nil
end

3. Do nothing and close the PR and I can just reload in the transfer_guest method in my own app 😄

[pacso]

In my implementation within my own app, the last thing it does in that function is reload the guest record.

However, I still think this is an implementation detail and we shouldn't assume everybody needs to do it. Just because there's a dependent association doesn't mean there's a problem with all of those records getting deleted.

I'd be in favour of updating the instructions in the readme and leaving it for people to decide whether they need it or not.

[joshmenden]

@pacso Yep I totally get that. Sounds like a good plan. I've just pushed up a commit with an extra note in the README. Let me know if you'd prefer the note to live in a different place in the README and I can push that up as well.

Thanks for your timely responses and for maintaining such excellent open source software!

[joshmenden]

Hi @pacso just wondering if you have any feedback or comments about the current suggested change.

[joshmenden]

@pacso Great change, just committed!