Include Article ID in Bottom Content Cache Key

[mstruve]

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

• Refactor
• Feature
• Bug Fix
• Documentation Update

Description

If the additional bottom content on the Article show page is being stored under a cache key that is created only using an article's tags, then one could end up seeing a suggested article on the actual article page itself if the tags were the same as a previous article that was viewed. This does mean the cache will be reused less since it is article specific which will affect performance. I am not sure how often this cache is hit across different articles, but that is something to consider.

I also added the article ID to the list of :not_ids for grabbing the @classic_article

I did read the contributing guidelines about adding specs around bug fixes, but since none of the cacheing I saw was tested in the specs and because I could not come up with a good clean test I choose to forgo it. If you determine tests are needed let me know and I can close the PR and reopen after they are added.

Related Tickets & Documents

#1697

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

N/A

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

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

Something something, first one is always the worst no matter how many times you have contributed to other repos....

[CLAassistant]

All committers have signed the CLA.

[Zhao-Andy]

Awesome, looks great! I think we'll want to figure out the best way to test cache keys first, so not having a test included is alright. Thanks for the PR!

Resolves #1697

[maestromac]

Thank you for this PR @mstruve !
Since this would change the cache to be article specific, it would increase our cache by a lot. I don't have enough understanding on the implication this might have our the app. @benhalpern what do you think?

[benhalpern]

Hey @mstruve, sorry I didn’t weigh in sooner.

Basically this PR is spot on. The existing implementation is a bug. Currently it leads to slightly better performance since articles share tags but not IDs, but the behavior inside the cache doesn’t match up well with the stuff outside.

So let’s actually remove the tag sample part of the key and just go with the ID. That will lead to the most cache hits.

Also I think the article’s ID is already implicitely included as a not ID, but I could be wrong there. I’m not in a good place to check right now (on my phone 🙃)

Yeah, @maestromac this will lead to some more cache missed temporarily but it will right itself soon enough. Overall it will yield fewer cache hits but it’s a necessary trade off for actual correct behavior.

This can be merged with the changes I’ve mentioned.