@sgiehl opened this Pull Request on November 15th 2021 Member

Description:

This should randomize the first time the feedback banner is shown

@tsteur is this random enough?

follow-up to #18308

Review

@sgiehl commented on November 15th 2021 Member

@tsteur updated the code. I had actually used the wrong line of code. It should now do what I had described in the PR.
Users created within the last 6 months will see the banner 6 months after the creation. Users that are older will get a random date within the next 6 months.

@tsteur commented on November 15th 2021 Member

@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 !empty($userCreatedDate) && Date::factory($userCreatedDate)->addMonth(6)->getTimestamp() > $now === true meaning it goes into the if block.

It then adds 6 months to the creation date in $nextReminder = Date::factory($userCreatedDate)->addMonth(6)->toString('Y-m-d');. Meaning nextReminder is in this case 1 year ago. Meaning next page load we would show it?

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?

@sgiehl commented on November 15th 2021 Member

@tsteur it actually should only go into the if block if the creation date is within the past 6 months. If it is older (creationdate + 6 months) should be lower than $now. Or did I get something wrong in the if statement :thinking:

@justinvelluppillai commented on November 15th 2021 Contributor

Looks right to me, if the creation date plus six months is less than now it will skip the if block

@tsteur commented on November 15th 2021 Member

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.

@tsteur commented on November 15th 2021 Member

Meaning feel free to merge if tests pass.

@sgiehl commented on November 15th 2021 Member

@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.

@tsteur commented on November 15th 2021 Member

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

This Pull Request was closed on November 15th 2021
Powered by GitHub Issue Mirror