Skip to content

Conversation

@dodumosu
Copy link
Collaborator

see comments on nditech/apollo-issues#41

@dodumosu dodumosu requested a review from takinbo May 16, 2020 05:44
@takinbo
Copy link
Collaborator

takinbo commented May 16, 2020

Thanks for the PR. Coincidentally, I started working on this and started rethinking the purpose for how we determine the current event.

So we currently have this:

now = current_timestamp()

lower_bound = datetime.combine(now, datetime.min.time())
upper_bound = datetime.combine(now, datetime.max.time())

# convert back to UTC because datetime.combine() returns
# naive objects
lower_bound = pytz.utc.localize(lower_bound)
upper_bound = pytz.utc.localize(upper_bound)

event = self.filter(
    Event.start <= upper_bound, Event.end >= lower_bound
).order_by(Event.start.desc()).first()

I forget why we need to fit the current event within the current day. Why not simply use the current timestamp and compare with that instead? I mean it is possible to have several events within the day! No?

So I rewrote it as this:

now = current_timestamp()

# find events overlapping with the current timestamp and return
# the most recent.
event = self.filter(
    Event.start <= now, Event.end >= now
).order_by(Event.start.desc()).first()

Thoughts?

@dodumosu
Copy link
Collaborator Author

dodumosu commented May 16, 2020

it's not usual, but it's happened. for example, i recall an instance when in Nigeria a training event (i believe it was across several states) was split across a single day (a certain state in the morning, with its specific event, another in the afternoon). still, that could be a workflow issue.

@takinbo
Copy link
Collaborator

takinbo commented May 16, 2020

What are the drawbacks of simplifying current event retrieval? I've done some testing with the Zambia current set of events and it appears to do what is expected. I'm trying to understand what were the considerations that led us to have a complicated system for determining what event to select as the current one.

@dodumosu
Copy link
Collaborator Author

i don't think there are any demerits to simply using the timestamp. i guess we were just trying to get a window for active events, instead of an instant.

@dodumosu
Copy link
Collaborator Author

dodumosu commented May 16, 2020

so this is what it looks like now based on the discussion above:

now = current_timestamp(use_app_timezone=True)

event = self.filter(
    Event.start <= now, Event.end >= now
).order_by(Event.start.desc(), Event.id).first()

if event:
    return event

# if there's no event, pick the closest past event
event = self.filter(Event.end <= now).order_by(
    Event.end.desc(), Event.id).first()

if event:
    return event

# if there's no event, pick the closest future event
event = self.filter(Event.start >= now).order_by(
    Event.start, Event.id).first()

if event:
    return event

return None

@takinbo
Copy link
Collaborator

takinbo commented May 16, 2020

current_timestamp currently returns the current timestamp in UTC format. Event start and end times are stored in UTC so I don't think there's a need to use the use_app_timezone option.

rather than use a day range for the active event query,
this commit uses the current instance.
it also reverts the implementation of `apollo.utils.current_timestamp`.
Copy link
Collaborator

@takinbo takinbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@takinbo takinbo merged commit dd61ef7 into nditech:develop May 16, 2020
@dodumosu dodumosu deleted the fix-event-timezone branch May 16, 2020 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants