-
Notifications
You must be signed in to change notification settings - Fork 481
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
Thank donors on first sign in (3rd attempt) #36279
Thank donors on first sign in (3rd attempt) #36279
Conversation
Ha or Bethany, could one of y'all give the new commit in this PR a look? Kind of related to this item, and Molly is swamped right now with survey work. |
Sure, happy to look this afternoon! 🌞 |
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.
New commit looks good. There's a part of me that thinks it should be its own PR as it affects a different dialog but either works
Good point -- I was thinking the new commit and the rest of the change needed to be merged at the same time or the DTT would fail again, but maybe if I merged the two PRs at (approximately) the same time it would be ok? Not sure I understand our DTT frequency well enough yet to know what would cause a failure or not... |
It seems like the latest commit could be a standalone commit and should pass a DTT on its own. So you could create a PR with just that commit, merge it, then merge this PR. Then they'll either go in the same DTT or consecutive DTTs. |
🤦 Makes total sense. Thanks! |
This PR -> #36276 (revert) -> #36195 (new code) -> #36169 (revert) -> #35914 (original PR)
In #35914, I set the normal test user being created in all other tests to have a sign_in_count of 2, to avoid them seeing the donor interstitial (it shows only on your first login).
When a user signs in for the first time after account creation in tests, their sign_in_count gets now incremented to 3, and a row in the sign_ins table is created with sign_in_count of 3, instead of 1 like it was previously.
Without a row for the first sign in, the logic that gates showing the race interstitial fails to prevent showing it.
The fix in this PR is to make the logic gating showing the race interstitial “don’t show race interstitial if you can’t find a user’s first sign in”.
Testing story
Added new unit test to cover the behavior that caused UI tests to fail before this PR was reverted (showing of race interstitial in places where it shouldn't have appeared). Added the test -> saw it fail -> made code changes -> saw it pass.