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

Feature/kak/multiple event destinations#1107 #1128

Conversation

flibbertigibbet
Copy link
Contributor

Overview

Allow an event to be associated with multiple destinations, in order to support the upcoming "tours" feature.

Demo

The field for associated destinations in the admin site UI for editing an event has changed from a drop-down to a multi-select field:
image

Notes

Originally I was planning on doing #1125 as well as part of this task, but after discussion with @lederer decided to break that out into a separate task, and also added #1126 and #1127. Instead, this PR only handles the data migration and minimal logic changes to prevent errors. There are a couple of places where an event is still assumed to have a single destination associated with it (if any), where for now it simply uses the first destination for the event: in routing, and in setting properties such as flags.

There is a backwards migration, but for it to work, the old destination field must be added back alongside the new destinations field in the model before running the backward migration.

I also fixed a small issue with two of the images for the default development event not being wide enough; they were set by the migration, but editing the event later would fail validation on the image.

Testing Instructions

  • Run migrations (provisioning will also do this)
  • Go to http://localhost:8024/admin/destinations/event and go to default event
  • The previous destination (Independence Seaport Museum) should be set on the new field
  • Edit multi-select widget to add other destinations and save
  • Home page should still load as expected
  • Routing should still work for event (it should go to one of the selected destinations)
  • Details page for event should still load as expected
  • Search results at http://localhost:8024/api/destinations/search should be as expected

Closes #1107

Also add flag to destinations to note if it is only for use with an asssociated event.
Remove development images for default event that are too narrow
to be the wide image. Fixes not being able to save event without
changing image(s).
Not yet implementing event-only destinations.
Copy link
Contributor

@KlaasH KlaasH left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I ran into some trouble with editing the Crazy Philadelphia Eddie event (missing CrazyPhiladelphiaEddie_3.jpg in the media directory) which I ended up solving by manually renaming a file. But I don't know if that would affect other people or was an artifact of the particular chain of branches and migrations in my database (plus the fact that I'm reluctant to destroy my database altogether).

obj['destinations'] = [set_destination_properties(x) for x in event.destinations.all()]

# if the first related destination belongs to Watershed Alliance, so does this event
obj['watershed_alliance'] = event.destinations.first().watershed_alliance if event.destinations.count() else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first one special, or should any Watershed Alliance destination cause the event to get the label? This could also be something like event.destinations.filter(watershed_alliance=True).exists(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interim fix that should be revisited with #1127, when we have answers on how these properties should be assigned (does it apply if any destination has the property? if all do?).

@flibbertigibbet
Copy link
Contributor Author

The missing image you encountered was likely due to the fix here removing test images that were too small to pass validation, and shouldn't be an issue for newly created VMs.

@flibbertigibbet flibbertigibbet merged commit 58fa786 into azavea:tours Aug 21, 2019
@flibbertigibbet flibbertigibbet deleted the feature/kak/multiple-event-destinations#1107 branch August 21, 2019 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants