Feature/kak/show tours#1109

[flibbertigibbet]

Overview

• Add tours to API results and homepage tab
• Remove optional times from tour destinations, and add order and optional times to event destinations
• Add detail page for tours
• Modify detail page for events to list multiple destinations and optional times
• Fix gulp watch
• Upgrade to Django 2 (only needed to change string model methods)
• Add tests for tours and event destinations

Demo

Homepage tour tab:

Tour detail page lists locations in cards:

Multi-day event:

Single day event:

Notes

For now, directions to tours or events goes to the first location. Also, filtering by event or tour categories on the map page is now broken (shows no results), but works on the homepage. These can both be addressed in #1111 or #1121.

There's no validation to check event destination times, if set, fall within range of the event times, or that they are ordered by time.

Testing Instructions

• Destroy and rebuild VM if not on Django 2 already, or else run migrations
• In app VM: cd /opt/app/src/ && npm run gulp should build frontend and rebuild on JS or style file change
• Page refresh after gulp watch rebuild should show change (there's no live reload)
• Default event should still exist and have one destination set
• Any other events that existed before migrations should still have their destinations set
• Editing destinations on events and tours in the admin interface should work as expected
• Create a tour and a multi-destination event
• Up to two events, then up to two tours, should show at the top of the homepage results
• Homepage after toggling to a filter tab then back to 'All' should look the same (JS vs Django template rendering)
• Tour detail page should list destinations as cards in order
• Multi-destination event page should list destinations, with times, if set
• If there are more than four destinations for an event, there should be a 'See all' button with the right total count
• Clicking the 'See all' button should hide itself and show the full destination list

Checklist

• No gulp lint warnings
• No python lint warnings
• Python tests pass

Closes #1109
Fixes #1036
Closes #1110
Closes #1138

[KlaasH]

I've explored this some and gotten partway through the code. It looks good. I like the code sharing between tours and events--seems clear and efficient.

Per the comment below, the change to Event.destinations seemed wrong to me, but what I was going to suggest--making it a M2M to Destination with through=EventDestination--isn't actually very useful since it's apparently not possible to sort that type of relation based on properties of the through model.

Also, seeing that made me realize that the same structure was already in place on Tour, which I didn't catch when I reviewed #1133. So I think the same argument applies there.

I'm going to pause on further review for now because I'm not sure how best to proceed and how extensive the code changes would be.

[KlaasH]

This should show end date as well. This is a multi-day event that looks a bit weird with only the start date (since the time range is backwards):

[flibbertigibbet]

I don't understand what you mean by "the time range is backwards." If the end date/time precedes the start date/time, the admin interface will prevent saving the event destination with an error. What are the full start and end dates and times for this event destination? (If the end date is a different year, it would present this way in the interface.)

[KlaasH]

That event did span the new year, but for a realistic example, take an event running from the evening of one day to the morning a few days later (like a music festival might). It looks like this in the admin:

And comes out like this in the front-end:

Showing only the start date for multi-day events makes it look like it's a one-day event that runs backwards in time. I think it should either show the range for both or not show the time range. Though the former, or some variation, seems better, since otherwise there's no indication that it's multi-day.

[flibbertigibbet]

Oh, I see. I've added the missing date range for event destinations in the template. Thanks for the explanation.

[KlaasH]

I think this should be

destinations = models.ManyToManyField(Destination, through=EventDestination, blank=True)

The EventDestination model adds properties to the Event<->Destination relation, but that's still the relation we care about--i.e. event.destinations.first() should return a Destination, not an EventDestination.
And there's already an event.event_destinations related manager created by the ForeignKey declaration on EventDestination.

Though upon further investigation, event.destinations.all() is apparently ordered by the ordering of the target model, not the connecting model. Which probably makes sense for some purposes, but it seems like a bug here.
I think that might mean this destinations property should just be removed. We don't need a M2M relation of Event to EventDestination, and having an Event.destinations related manager that refuses to sort properly just seems like a recipe for error.

[flibbertigibbet]

I've removed the unused destinations fields from the Event and Tour models (all that was needed was the migration). I'd added them also thinking they'd be necessary to set the through relationship, but it doesn't work that way.

[flibbertigibbet]

This should be ready for another look. I've addressed everything but the date range templating; it's not clear to me what's going on there.

[KlaasH]

The handling of EventDestination and TourDestination looks good now. Still a bummer that we can't set them as through tables and have the ordering just work, but so it goes. This setup is still consistent and easy to follow.

A couple optimization comments below, but it all looks good and seems to be working well. The one thing I think needs to change somehow is the display of multi-day destinations within multi-day events, as discussed above.

[KlaasH]

Like the one on Event, this could avoid the count() query:

    @property
    def first_destination(self):
        try:
            return self.tour_destinations.first().destination
        except AttributeError:
            return None

[KlaasH]

Could this be written to use exists()? I.e.

return not self.tour_destinations.filter(destination__accessible=False).exists()

The double negative is a little odd, but it seems like it would produce the same answer without having to load any destination instances into memory.

If that works for this, it should also work for watershed_alliance below.

[flibbertigibbet]

Okay, this should be ready for another look.

[KlaasH]

There are a number of possible combinations of dates and times:

• Single-day event (for which the destinations obviously should be single-day as well)
• Multi-day destination on a multi-day event
• Single-day destination on a multi-day event

Here's an example of "everything on one day":

Nothing there is wrong or confusing, but the dates on the destinations are superflous--it could just show times.

Here's a multi-day event with one single-day and one multi-day destination:

This, too, is not incorrect but might look better if the single-day destination used the format that's being used for single-day events, i.e.

Thu Sep 12
6pm - 8pm

If the current setup is what's desired, that's fine--each of the labels is correct and complete. Some of them are just potentially a little less clear than they could be without repeated bits.

(Also, this "only show dates for multi-day events" comment doesn't quite match what's happening around it.)

[flibbertigibbet]

The single_day Django helper method was returning inconsistent results because it wasn't timezone aware (it depended on the time range whether it seemed to work or not). It was misbehaving similarly for the overall event date/time range as well.

[flibbertigibbet]

I've fixed the timezone issue causing the template single-day check to be off sometimes. Before:

After:

[KlaasH]

👍 Looks good!