AO3-6439 Set User.current_user in around_action hook and fix tests using it outside of requests #5111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bilka2
force-pushed
the
AO3-6439-set-user-current-user-in-an-around-action-hook
branch
from
16d7a78 to
26c8583
Compare
brianjaustin
approved these changes
Mar 26, 2025
Bilka2
deleted the
AO3-6439-set-user-current-user-in-an-around-action-hook
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
https://otwarchive.atlassian.net/browse/AO3-6439
Purpose
Moves setting
User.current_userto anaround_actionhook. This means it's consistently cleared outside of requests. This relevant for tests, where can be code running outside of request which would erroneously have the previous value set forUser.current_user.Because
User.current_useris no longer set outside requests, all test steps that relied on it being set needed to modified. This was various steps and the crucially also thepaths.rbfile. Unfortunately, the only way to remove references to the currently logged in user inpaths.rbis to directly reference a user by name, which means modifying all the path names. This lead to a lot of cascading changes around the tests. My apologies to whoever reviews this, but it wasn't possible to split it into multiple commits.After this fix, various test steps no longer need to force
User.current_userto be nil before using FactoryBot, so I've removed those lines.This should hopefully fix the flaky tests we've seen recently. At least some of them were definitely related to
User.current_userunexpectedly being nil outside requests. Now that it's always nil and the tests have been adjusted, this should no longer be an issue.Testing Instructions
For manual testing, refer to Jira.
For automated testing, I reran the automated tests 3 times and they were not flaky: https://github.com/Bilka2/otwarchive/actions/runs/14035346539
Credit
Bilka