Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor current meeting logic #304

Merged
merged 6 commits into from
May 18, 2018
Merged

Refactor current meeting logic #304

merged 6 commits into from
May 18, 2018

Conversation

hancush
Copy link
Collaborator

@hancush hancush commented May 17, 2018

This PR:

  • Refactors LAMetroEvent.current_meeting
  • Removes methods superseded by the refactor from lametro.utils
  • Refactors tests for current_meeting functionality

@hancush hancush changed the title [wip] Refactor current meeting logic Refactor current meeting logic May 18, 2018
@hancush hancush requested a review from fgregg May 18, 2018 14:47
Copy link
Collaborator

@fgregg fgregg left a comment

Choose a reason for hiding this comment

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

Looking pretty good. There's one troubling thing with the GUID and we need a bit more explanation of intent in some of the docstrings.

# We get back two GUIDs, but we won't know which is the English
# audio GUID stored in the 'guid' field of the extras dict. Thus,
# we iterate.
meeting = cls.objects.filter(extras__guid=guid.upper())
Copy link
Collaborator

Choose a reason for hiding this comment

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

seeing guid.upper() suggests something is pretty wrong upstream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

guids are all-uppercase in the api, but are all-lowercase in the running events endpoint. i'd prefer not to alter the source data in the scrape, so it seems appropriate to do a transformation for this instance-specific comparison. happy to hear an alternate perspective!

Copy link
Collaborator

Choose a reason for hiding this comment

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

yikes, okay. add a comment that explains why this thing is happening.

'''
from .utils import calculate_current_meetings # Avoid circular import
If there is a meeting scheduled to begin in the last six hours or in
the next five minutes, hit the running events endpoint. If there is a
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a pretty good explanation of what the code is doing, but it would be even better if it explained why.

In particular, if there is not streaming_meeting, then why do want to show the meetings that started within the last 20 minutes.

Is the idea that streaming_meetings is not completely reliable to indicate that a a meeting is happening right now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, per metro, events "regularly" start (e.g., show up as streaming) later than their scheduled start time. if we are only looking for meetings five minutes in the future, a meeting that does not start on time will not appear as current.

so, if there is not a streaming meeting, we also look for meetings scheduled to start up to 20 minutes ago (i consulted with omar on this buffer), so meetings that start late are still returned as current in the interim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fgregg i expounded on this in the docstring

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, that's the kind of thing that would be good to have in your comment. This code would be clearer to me if you used if elif else constructs instead of returns.

     if scheduled_meetings:
            streaming_meeting = cls._streaming_meeting()

            if streaming_meeting:
                current_meetings = streaming_meeting
            else:
                # explanation that sometimes a meeting stream shows up late
                twenty_minutes_ago = cls._time_ago(minutes=20)

                # '.annotate' adds a boolean field, 'is_board_meeting'. We want to
                # show board meetings first, so order in reverse, since False (0)
                # comes before True (1).
                current_meetings = scheduled_meetings.filter(start_time__gte=twenty_minutes_ago)\
                                     .annotate(is_board_meeting=RawSQL("name like %s", ('%Board Meeting%',)))\
                                     .order_by('-is_board_meeting')
      else:
           current_meetings = cls.objects.none()

      return current_meetings

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