-
-
Notifications
You must be signed in to change notification settings - Fork 71
Follow / unfollow thread actions should bump its last activity date & time + minor general fixes #1931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…d, or last activity)
…red it accounts for the last_activity_at column
…e same to 'thread' to match
…aram behavior due to rename from 'dirty'
…ad#last_activity test
…ty test accordingly
Codecov Report❌ Patch coverage is
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Would it be easy to also, while we're here, make unfollow leave the in-page thread open, same as follow does? This difference was discussed in chat (pre-existing issue), but I don't know if making them consistent is easy or too much scope creep (and we should file a separate issue for it). |
cellio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested (with the env var per the description) and it's working as expected. The code overall looks good to me; I have one question and would like someone else to review that part.
7346b03 to
74eaa4c
Compare
trichoplax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having switched on caching in development as suggested (rails dev:cache from bash inside the container, followed by adding PERFORM_CACHING=true to docker/env, followed by restarting the container), I can reproduce the problem on the develop branch (both the inline thread view and the full thread page can be in a state where clicking "unfollow" appears to have no effect and even a hard refresh doesn't change it to show "follow" (or the other way around)).
On this new branch, the caching problem is fixed so if a page is not matching the server state, refreshing the page fixes it. I see that as fixing the identified problem, so I have approved this pull request, with one minor optional comment.
During testing I noticed a couple of pre-existing problems, so to avoid scope creep I've raised them as separate issues #1932 (which I see you already fixed while I was typing up the other one) and #1933.
cellio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Thanks for adding that route so the in-page behavior is the same for follow and unfollow.
cellio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup job LGTM by inspection. Re-approving pending some testing that @trichoplax is going to do. (Test code looks good; I haven't set up the bug conditions for manual testing.)
closes #1930
closes #1932
closes #1933
This PR makes sure following / unfollowing a comment thread bumps the thread's last activity date & time and thus prevents stale content from being served afterwards.
It also fixes confusing inversion of the
User#anonymizemethod'spersist_changesparameter (turns out it wasn't the best idea to renamedirtytopersist_changeswhile tired - thankfully, the logic was sound, this change is just a matter of making it not confusing).Additionally, it ensures attempting to make the same action multiple times does not create duplicate entries (as well as automatically cleans up any duplicate entry on unfollow).
Finally, it also makes sure we don't need to reload the whole page upon successfully unfollowing a thread (and gets us closer to removing
thread_unrestrictentirely).Note for reviewers: to be able to test this change, you need to enable caching in development (should be on by default, but it doesn't hurt to check) and set the
PERFORM_CACHINGenv variable totrue.