Fix reading list counter

[takmar]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

I found that a wrong reading list count was caused by caching. The cache of cached_reading_list_article_ids is changed only when public_reactions_count is updated, but we must also recache it when user archives and unarchives an article.

To achieve this, I added the last_changed_archive_at column to the users table and cache keys.
I could not come up with an idea to fix this without adding an extra column. Will you let me know If you have better thoughts, I'll give it a try.

Related Tickets & Documents

Closes #9770

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, as well
as any relevant images for UI changes.

Added tests?

• yes
• no, because they aren't needed
• no, because I need help

Added to documentation?

• docs.forem.com
• readme
• no documentation needed

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

[mstruve]

Thanks for tackling this bug. Code looks pretty good, I left one suggestion, let me know what you think!

[mstruve]

I am not a huge fan of adding another "cache" focused column to user but I cant think about a better way to do this.

What do you think of making this a more general column so we could use it for other caches as well? I was thinking last_reacted_at which would fit nicely with our pattern of last_followed_at, last_article_at etc.

[takmar]

Thank you for your comments.

I am not a huge fan of adding another "cache" focused column to user

Neither am I. There are already a bunch of cache keys.

What do you think of making this a more general column so we could use it for other caches as well? I was thinking last_reacted_at which would fit nicely with our pattern of last_followed_at, last_article_at etc.

That’s a new idea for me. At least, I think that it is better than mine as we do not need to add a column whenever something related to a user needs to be cached.

[mstruve]

Small suggestion for the spec but otherwise this LGTM!!!!

[mstruve]

Rather than relying on ActiveRecord to local memoize this here, I think a cleaner approach would be.

original_reacted_at = user.last_reacted_at
put "/reading_list_items/#{reaction.id}"
expect(reaction.reload.status).to eq("archived")
expect(original_reacted_at).not_to eq(user.reload.last_reacted_at)

Same with the example below.

[citizen428]

We could also use an actual change assertion here:

expect do 
  put "/reading_list_items/#{reaction.id}"
end.to change { user.reload.last_reacted_at }
expect(reaction.reload.status).to eq("archived")

[takmar]

I have never thought of being able to use change by giving put "..." to expect as a block. Thank you!

[atsmith813]

Looks like there's a small conflict that needs to be resolved. Otherwise, LGTM! Thank you so much, I'm looking forward to this fix 🎉 .