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

display english and spanish event media links correctly #295

Merged
merged 6 commits into from
May 8, 2018

Conversation

hancush
Copy link
Collaborator

@hancush hancush commented May 1, 2018

this pr:

  • pins la-metro to the latest version of django councilmatic to take advantage of multiple related EventMedia
  • updates template logic here and here to iterate over and display event.media_urls rather than only one event.media_url
  • refactors extra_context here to grab both english and spanish live media links (and make logic more obvious: a link to live media appears in legistar when a meeting is ongoing)
  • update all listen link text to "watch in english" and "ver en español"

addresses #263

@hancush
Copy link
Collaborator Author

hancush commented May 3, 2018

to generate both live media links for bilingual events, we need the guids of both the english and spanish events. to do that, we'll add the spanish event as an additional source to the unified event in the upstream scraper.

@hancush
Copy link
Collaborator Author

hancush commented May 3, 2018

blurgh, we don't import all of the sources into councilmatic, so an additional source would require changes there, too.

@hancush
Copy link
Collaborator Author

hancush commented May 3, 2018

we also only add the web detail link if it exists, otherwise we just add a link to the calendar. these links don't seem to exist, until after the meeting has passed.

screen shot 2018-05-03 at 5 09 29 pm

@fgregg
Copy link
Collaborator

fgregg commented May 4, 2018

How would we get the GUID if the meeting detail url didn't exist? Is that knowable some other way?

@hancush
Copy link
Collaborator Author

hancush commented May 4, 2018

@fgregg it's on the api event; we save it in the extras, and import it in django-councilmatic.

we could save the spanish guid in the extras, as well, but we'd still need to make a change in django-councilmatic to facilitate > 1 event guid.

@fgregg
Copy link
Collaborator

fgregg commented May 4, 2018

@fgregg it's on the api event; we save it in the extras, and import it in django-councilmatic.

we could save the spanish guid in the extras, as well, but we'd still need to make a change in django-councilmatic to facilitate > 1 event guid.

This seems sensible to me.

It also seems to me that we should add the spanish api event url and, when available, the spanish legistar meeting url to the OCD data when available, even if we don't import that data into councilmatic.

@hancush hancush requested a review from fgregg May 8, 2018 14:52
@hancush hancush changed the title [wip] display english and spanish event media links correctly display english and spanish event media links correctly May 8, 2018
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.

looks pretty good. I think one of your methods is doing too much, but otherwise looks good.

guid = self.extras['guid']
english_url = self.BASE_MEDIA_URL + 'event_id={guid}'.format(guid=guid)

return self._valid_or_generic(english_url,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For me this would be clearer if you _valid_or_gener was just _valid, and then you handled the fallback case here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point, updated in eb57335

Also add untracked meta migration
@hancush hancush merged commit 3f8e969 into master May 8, 2018
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