AO3-5054 Update kudos when kudos-giver changes username

[irrationalpie7]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-5054

Purpose

When a user updates their username, updates the user's username in the kudos listing of every work they've kudosed.

Notes

Testing Instructions

2 cucumber tests added.

Jira "steps to reproduce" should be sufficient for any manual testing.

Credit

irrationalpie they/them

[irrationalpie7]

Tried a different approach involving touch_all on kudos.commentable, since I was having trouble testing fragment expiration in kudos sweeper. The other approach is basically:
Kudo.where(user: self).each(&:save) instead of Work.kudosed_by_user(self).touch_all in user.rb, and then you add the following lines to app/sweepers/kudos_sweeper.rb:

  def after_save(kudo)
    # expire the cache for the kudos section in the view
    ActionController::Base.new.expire_fragment("#{kudo.commentable.cache_key}/kudos-v4")
  end

I tried out both ways and the feature test I added passes either way, I just wasn't sure how to write spec tests for the latter. Also maybe the second approach loads more into memory and/or performs more calls since it can't do the update with just one call to the database? But then again I don't know what knock-on effects touching all these works would have either. I'm not sure how to verify what's better here.

The main problem is that touch_all doesn't do callbacks, so kudos.touch_all or update_all in user.rb can't fire the critical cache-invalidation method in the kudos sweeper. I'm not really sure of the trade-offs, but if the other approach seems better I can bring it back.

[redsummernight]

Is there a better way of triggering the sweeper to notice that kudos are being changed? Either in the loop itself or choice of method (touch & (after_)save)? I have no idea how performant this approach is.

I moved cache expiration into a model callback (#4293) so we don't need to trigger the sweeper separately anymore.

Once I know that, I can add the same mechanism to the remove_user_from_kudos method in models/user.rb--I'm guessing I'd test it almost the same, just delete the user and then make sure their username isn't visible on the work page anywhere anymore?

Yeah, I think your "Changing username updates kudos fragment" test should cover it.

Should there be any spec tests for this? And if yes, are there any existing tests that I should model this functionality after?

We've had tests for cache invalidation like:

otwarchive/spec/models/pseud_spec.rb

Lines 52 to 64 in a7c6b4e

	describe "touch_comments" do
	let(:pseud) { create(:pseud) }
	let!(:comment) { create(:comment, pseud: pseud) }
	it "modifies the updated_at of associated comments" do
	# Without this, the in-memory pseud has 0 comments and the test fails.
	pseud.reload
	travel(1.day)
	expect do
	pseud.update(name: "New Name")
	end.to change { comment.reload.updated_at }
	end
	end

The main problem is that touch_all doesn't do callbacks, so kudos.touch_all or update_all in user.rb can't fire the critical cache-invalidation method in the kudos sweeper. I'm not really sure of the trade-offs, but if the other approach seems better I can bring it back.

That's really unfortunate. The downside of using touch on each individual work or kudo is that you may end up loading them one by one, and users can have a lot of either, slowing down the process. We'll need to think of something, but I'm not sure yet.

[zz9pzza]

I know this can take a long time, I believe this should be moved to an asynchronous task sorry.

Looking at https://apidock.com/rails/ActiveRecord/Relation/touch_all it looks like touch_all changes the record in the database, I don't believe we want to do that we just want to delete the fragments. There are a silly number of Kudo and reseting the username of someone who had kudosed lots of work could create issues. ( If we did have to change the works then it would have to be in a transaction in batches of say 1000 ).

[irrationalpie7]

Does asynchronous task just mean "a thing that executes asynchronously," or does it have a specific technical meaning in this situation? I was trying to write up a response and then realized that I wasn't sure.

I agree that touch_all seems like not quite the right solution, but I'm struggling to figure out how to trigger mass fragment deletion. The key point is that we need to run ActionController::Base.new.expire_fragment("#{commentable.cache_key}/kudos-v3") for each work the user has kudosed, right? So is the idea that we somehow trigger an asynchronous task that iterates through a user's kudos (with commentable id) in batches of 1000? Would that be something like a new sweeper that observes users and reacts when they change usernames, or something else?

I notice that app/sweepers/kudos_sweeper.rb was removed in #4293 and its functionality moved to app/models/kudo.rb; that'll be some fun merging for me, but after looking the change over I don't think it fundamentally alters the core problem/high-level solution here.

Anyway, thanks for the feedback, and sorry it took me a bit to get back to you about it!

[irrationalpie7]

The key thing here is that to expire the cache, which looks like this:

    ActionController::Base.new.expire_fragment("#{kudo.commentable.cache_key}/kudos-v3")

we need to expire the fragment for every work a user has kudo'ed (Work.kudosed_by_user(self)), either by directly expiring it, or by causing the work's cache_key to change (which touch would do).

I tried looking through the code base to try to find where it would make sense to put an asynchronous task to expire the fragments, and I'm not sure--is it more like indexing, which seems to add things to a queue and then batch that every so often? Or would it be more like one of the rake tasks that performs periodic cleanup? I'm not clear on how to make this work and could use an example.

[irrationalpie7]

Change summary:

• Remove work.cache_key from the cache key because it's not really changes to the work that map to when the cache should expire
• Move some stuff I added to work.rb back to kudo.rb
• Try a new testing approach and make sure it passes (since I can no longer rely on the cache key changing)

Notes/Questions:

• I didn't really address the async part of the previous comments here because I don't understand what would make most sense for this situation. Looking in the repo, there seems to be a few different ways async work happens, and I'm unclear on the pros/cons of using any of these mechanisms:
  • shared infrastructure around async work with regards to indexing
  • kudo and subscription mailers that have a slightly different system
  • rake tasks
• Complicating this issue is the fact that when deleting a user, we remove their id from kudos, so we wouldn't be able to query kudos by user id to expire caches if we don't do it right away (although maybe we could look for kudos with nil user_ids that have no ip address and have been recently updated??)

[irrationalpie7]

Based on some discussion on slack, I've switched back to commentable.cache_key from commentable.id as it allows for more flexibility re: what you allow to be kudosed, e.g. admin posts. I mentioned in my previous comment that I'm not sure how to address making the cache expiration happen in an async manner; one idea that got brought up on slack that might eliminate that requirement entirely is switching to a time-based cache expiration, although I'm not sure how to make that call or how you'd decide on a reasonable amount of time.

I updated the cache key version from 3 to 4, since I figured we want to expire the current state of the cache in order to reset existing cases where a user might have changed their username since leaving kudos, but if we don't need that I don't think it adds anything.

[redsummernight]

We discussed this in chat. @irrationalpie7 suggested the approaches to cache expiration:

(a) touch the object (in this case, the commentable) so the cache_key changes when there's a situation where we need the cache to expire

(b) add or replace it with an object whose changes map more directly to when we want to invalidate the cache so that invalidation happens automatically (the kudos example is a good example for this in that kudos changing has nothing to do with the commentable changing, so there's a mismatch, but a bad example in that there's nothing obvious to replace it with--a more direct example might be changing cache(bookmark) to cache([bookmark, bookmark.pseud,bookmark.pseud.user]) to sync with bookmarker pseud or user name changes)

(c) directly expire the fragment when we need it to expire

@tickinginstant:

One disadvantage of (a) is that it generally locks the object(s) being touched for the rest of the transaction, which can be bad in certain cases. (e.g. If you could prevent someone from editing their own work by leaving kudos and then changing your username a bunch, that'd be bad — obviously the limits on renames prevent that, so that's not a real example, but you get the idea.)

(Also, the cache key for (b) for kudos is probably the collection cache key on the kudos being displayed, or possibly the collection cache key on the users who left those kudos. The disadvantage of that is that it requires the kudos to be loaded queried even when cached, but now that we're limited to loading 50 kudos instead of all 60,000+, it might actually be possible to do the database query for the kudos load on every page without crashing the whole website. Still kind of iffy, though.)

@zz9pzza:

Just having a simple time based expiry is not to be forgotten. It is simple. I have vague memories of caching the cache key which works when computing the cache key is expensive

Instead of clearing the kudo cache on some database change, let's set expires_in for it to some configurable value (maybe 1h to start with). In your feature tests you can use the time travel helpers to expire the caches (e.g. #4034).

Like #4034, you should also set race_condition_ttl to 10 seconds as well, keeping the stale cache around a bit longer in case of high traffic.

[irrationalpie7]

(not quite ready for review yet, I'm still trying to get docker back working on my machine so I can actually run the tests)

[irrationalpie7]

I used #4524 as an example for some of the tests to write, not sure why they're failing at the moment but I'll continue to investigate

[Bilka2]

Looks good!

There are 2-3 minor style changes that would be nice to have if you have the time. But I don't think that they are mandatory to merge this.

[Bilka2]

Suggested change

	Scenario: Deleting user updates kudos fragment
	Scenario: Deleting user updates kudos fragment cache

would be a bit clearer for what is getting tested. Same for the other test.

[sarken]

I think these tests are misleading, tbh. The cache fragment isn't actually getting expired by the user deletion or rename; it just gets expired periodically.

We'd rather the tests go in kudos.feature and be rephrased to something like "Kudos cache expires periodically to ensure deleted users are removed and renamed users are updated." Both the rename and the deletion can go in the same scenario.

[irrationalpie7]

Moved the test as @sarken suggested!