rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
462 stars 490 forks source link

Partner dashboard distributions timing refinement #2905

Closed cielf closed 1 year ago

cielf commented 2 years ago

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

cielf commented 2 years ago

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

dorner commented 2 years ago

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

seanmarcia commented 2 years ago

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

cielf commented 2 years ago

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

dorner commented 2 years ago

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 :)

seanmarcia commented 2 years ago

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.

cielf commented 2 years ago

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

cielf commented 2 years ago

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

dorner commented 2 years ago

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

cielf commented 2 years ago

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 commented 2 years ago

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.

dorner commented 2 years ago

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

cielf commented 2 years ago

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.

edwinthinks commented 2 years ago

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

dorner commented 2 years ago

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

cielf commented 2 years ago

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!

github-actions[bot] commented 2 years ago

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] commented 2 years ago

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.

cielf commented 2 years ago

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.