human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

Partner dashboard distributions timing refinement

Open cielf opened this issue 3 years ago • 18 comments

Summary

The partner dashboard shows upcoming distributions and prior distributions. Currently anything with a date of today or later is upcoming. This should be based on the current time instead. That is, if you had a distribution at 10am, and you are looking it at 3pm, it should be in the prior distributions rather than the upcoming distributions.

Things to Consider

Although this is a very minor issue, business-wise, it also as a side effect of making our current tests fail late in the evening. That's the impetus for the change.

Criteria for Completion

  • [ ] The partner dashboard shows distributions that are earlier in the current day in prior distributions, and those that are later in the day than 'now' in upcoming.
  • [ ] The tests around this work both during the day and late in the evening (post 9pm on your eastern time computer.)

cielf avatar May 15 '22 12:05 cielf

Apparently there is a whole can of worms around timing that I'm opening up here. Which does not change that from a business p.o.v., "now" would probably be better...

cielf avatar May 15 '22 16:05 cielf

@cielf can you elaborate on what can of worms you're referring to? :)

dorner avatar May 23 '22 17:05 dorner

I think it is this spec: rspec ./spec/system/partners/partner_dashboard_system_spec.rb:53 which fails periodically.

seanmarcia avatar May 23 '22 18:05 seanmarcia

I haven't dug completely into it -- it's on my mental list of things to grok -- but say we are entering the time for pickup of distribution. My understanding is that we are storing what the user enters (say 3pm), without rebasing it to a common time zone.

One of the effects of this is that rspec ./spec/system/partners/partner_dashboard_system_spec.rb:53 fails after 9 on machines on Eastern Time , because test.rb is running on LA time.

Without digging, I expect that "now" would be similarly affected to "today". Only we'd see any effects all the time, not just late in the evening.

To be understood more fully: The edge cases of organizations that run across time zones (Chicago area, state-wide orgs in Indiana, Tennessee, Oregon, Idaho).

cielf avatar May 24 '22 11:05 cielf

This should be handled internally by Rails in most cases. Rails automatically stores all datetimes as UTC. It converts them to local time when it pulls them out of the DB. Because of this, the "upcoming" and "previous" should "just work" ™️ . If the code is doing something weird, we should fix it so it isn't :)

dorner avatar May 24 '22 14:05 dorner

I think in the meantime we should skip this test until we figure out the cause. The periodic failures are giving bad signals on other PRs.

seanmarcia avatar May 25 '22 21:05 seanmarcia

I'm going to assign myself to this for the nonce -- for investigation. (this may be a stretch assignment)

cielf avatar May 31 '22 19:05 cielf

My current understanding is that Rails doesn't automatically know the client time zone.

cielf avatar Jun 05 '22 15:06 cielf

That shouldn't affect specs though, since specs would run in the same time zone as Rails itself.

dorner avatar Jun 07 '22 14:06 dorner

Well, the behaviour is consistent with there being a problem regarding time zones. config/environments/test.rb sets the time zone to America/Los Angeles, and the distribution test in question fails consistently after 9pm eastern. Now, I'm pretty sure there is no reason any more for the test time zone to be set to America/Los Angeles, but that just would make the current spec work. I don't know that we a spec for the case of the time for a distribution being set by someone in one time zone, but read by someone in another -- I don't know how you would set up a test for that.

cielf avatar Jun 08 '22 03:06 cielf

I'll have to work on this during the period where it fails to confirm that I'm right about the distribution test. That just hasn't been happening (I'm a lark, not a night owl). Though I suppose.... I could prove it by changing the test.rb time zone to something even further west. Maybe.. though I'm not sure off the top of my head how the international date line would impact.

cielf avatar Jun 10 '22 19:06 cielf

Can you temporarily just change your computer's date and time? :)

dorner avatar Jun 10 '22 19:06 dorner

I've had some bad experiences doing that in the past, so I avoid it.

Worked on it this evening -- removing the test.rb setting of the timezone to LA makes rspec ./spec/system/partners/partner_dashboard_system_spec.rb:53 work after 9pm. Alas, it seems to make at least 2 dozen other tests fail. This is interesting.

cielf avatar Jun 12 '22 01:06 cielf

@cielf love to jump into this with ya.

https://github.com/iansinnott/jstz could be an interesting tool to use to accommodate client's timezone to change the rendered page. Also a small plug here, maybe we can use hotwire to auto-reload periodically to re-render some distributions to be the past and others in the upcoming.

edwinthinks avatar Jun 12 '22 13:06 edwinthinks

Is auto-reload something we need here? Is the issue that users are on this screen for a long period of time?

dorner avatar Jun 12 '22 15:06 dorner

I don't know yet that we would need auto-reload. The issue is not that users are on this screen for a long period of time.

We've ended up 'in the weeds' of time stuff because the current test doesn't work between 9 and midnight, reliably.

If we take setting the time to Los Angeles off, it works, but 2 dozen others fail (this was around 10 last night). In the middle of the day (i.e. around 11 am), most of those pass, but ./spec/services/calendar_service_spec.rb:3: does not.

Definitely going to be a learning experience!

cielf avatar Jun 12 '22 16:06 cielf

This issue has been inactive for 248 hours (10.33 days) and will be automatically unassigned after 112 more hours (4.67 days).

github-actions[bot] avatar Jun 23 '22 00:06 github-actions[bot]

This issue has been inactive for 368 hours (15.33 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

github-actions[bot] avatar Jun 28 '22 00:06 github-actions[bot]

That’s where it originated from. I haven’t dug deeply myself, but if I understand correctly, we aren’t converting to, say, GMT, when we enter times, which is what is behind that issue?

Sent from my iPhone

On May 23, 2022, at 2:27 PM, Sean Marcia @.***> wrote:

 I think it is this spec: rspec ./spec/system/partners/partner_dashboard_system_spec.rb:53 which fails periodically.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

cielf avatar Oct 11 '22 08:10 cielf