Determine the first date to show feedback question banner more randomly #18315
Conversation
sgiehl
force-pushed
the
feedbackquestion
branch
from
eb5789b
to
b65a175
Compare
There was a problem hiding this comment.
@sgiehl it looks random. However, if I see this correct there is one problem:
Date::factory($userCreatedDate)->addMonth(6)->getTimestamp() > $now
This would apply to a lot of users after updating? Would potentially most users get to see this after updating if their users was created more than 6 months ago? I think I mention something like we may want to check something like if the user was created between 6 and 6.5 months ago.
For this to work we might actually need to do a modulo as a user might have been created 1 year ago. Would we need to find out number of days since signing up, and then do a modulo (like $numDaysSinceRegister modulo 6*30)?
|
@tsteur updated the code. I had actually used the wrong line of code. It should now do what I had described in the PR. |
|
@sgiehl I'm not sure if it would still cause every user that was created like a year ago to see this at the same time? Like assuming creation date was 1.5 years ago. That means It then adds 6 months to the creation date in The same would happen for users signed up 1 year ago and 2 years ago etc. Basically, as soon as they upgrade to the next version, they would all get to see the feedback screen? Or maybe I'm missing something? |
|
@tsteur it actually should only go into the if block if the creation date is within the past 6 months. If it is older |
|
Looks right to me, if the creation date plus six months is less than now it will skip the if block |
|
I see, makes sense. Sorry about that. So in that case we then just pick something randomly. I thought initially to make it then continuous so we show it after 1 year, 1.5 years, 2 years but we make it instead randomly. That's why I read it wrong. That should be all fine though and works too 👍 Ideally there were tests for it @sgiehl as working with date etc there's always quickly a bug introduced. But I'm happy to skip it to get it into the RC. |
|
Meaning feel free to merge if tests pass. |
|
@tsteur it should only be the first date that is picked randomly in that case. Once it was shown and clicked away it will be shown again after 6 months. |
|
Yep 👍 all good. I meant the first one we show after 1 year, or 1.5 year etc. But that's not important actually just something I had in my mind somehow. Picking the first date randomly is probably even better 👍 so all good |
sgiehl
added
the
not-in-changelog
Description:
This should randomize the first time the feedback banner is shown
@tsteur is this random enough?
follow-up to #18308
Review