Do not show thank donors interstitial when asking for school info #36360
Conversation
|
@dju90 FYI |
| @@ -8,6 +8,8 @@ def self.show?(user, request) | |||
| return false if user.age.nil? # Age dialog | |||
| return false if user && user.teacher? && !user.accepted_latest_terms? # Terms of Service dialog | |||
| return false if GdprDialogHelper.show?(user, request) | |||
| return false if SchoolInfoInterstitialHelper.show?(user) | |||
| return false if SchoolInfoInterstitialHelper.show_confirmation_dialog?(user) | |||
There was a problem hiding this comment.
This approach looks good but these method have side effects 😭
So we'd either have to refactor those (which tbh we probably should, I'm pretty anti these methods using the ? syntax and having side effects, but that might not be necessary for this PR) or we could move this logic to index.html.haml. I wonder if there's a way to enforce that we only show one dialog in that file
There was a problem hiding this comment.
Ahh, I obviously did not look closely at that, thank you for catching!
I think I was sort of envisioning we move towards a world where every dialog has a show? helper (Elijah suggested these should actually be "policy objects", but I think that's the same idea) -- I refactored the GDPR logic in the donor dialog work to have a helper, and obviously added one for whether to show the donor dialog. Then we have some central place that prefs which to show (maybe index.html.haml, or another helper)?
There was a problem hiding this comment.
Could I move the user update to index.html.haml (instead of in the show? method)?
There was a problem hiding this comment.
Gave it a shot, LMK what you think!
There was a problem hiding this comment.
The new update looks good to me! I'd be surprised (and disappointed) if this doesn't end up breaking tests, so those might need to be modified
There was a problem hiding this comment.
I did check the helper test file and didn't see anything that looked like it would fail :|
There was a problem hiding this comment.
Yeah, this passes Drone. That's disappointing :(
There was a problem hiding this comment.
It looks like we once had a test for this but I can't figure out when it was removed...
@bethanyaconnor saw this one coming -- if a teacher signs up and a) did not provide school information, b) logged in once and c) never logs out, they can see both the school info and the thank donors interstitials at the same time:
My change is to pref the school info dialog over thanking donors.
Testing story
Added new unit tests to cover this change. Also created a new account and confirmed I see the dialog without error, and also logged in with an older account confirm I don't see the dialog.