From 7e4cb491c1bcd126f42133013fb5d8ee6167e5aa Mon Sep 17 00:00:00 2001 From: David Cain Date: Mon, 24 Jun 2024 11:16:39 -0600 Subject: [PATCH] Fix lingering weirdness of "upcoming trips" Stop linking to a permanent redirect straight from the homepage! We *used* to have different trips pages -- one just for upcoming trips, and one for all past trips. We did away with `/trips/all` when we had *thousands* of trips to display. We left some weird artifacts around -- a pointless class hierarchy, a misleadingly-named route, and bad URLs. Let's fix all those. --- ws/feeds.py | 22 ++++++++++++---------- ws/models.py | 5 +++++ ws/templates/base.html | 4 ++-- ws/templates/help/leaders/trip_admin.html | 5 ++--- ws/templates/home.html | 7 ++++++- ws/templates/preferences/lottery/edit.html | 4 ++-- ws/templates/profile/membership.html | 2 +- ws/templates/trips/__create_or_edit.html | 2 +- ws/templates/trips/all/view.html | 6 +++--- ws/templates/trips/signup.html | 2 +- ws/tests/test_auth.py | 2 +- ws/tests/views/test_trips.py | 2 +- ws/urls.py | 4 ++-- ws/views/participant.py | 15 +++++++++------ ws/views/signup.py | 2 +- ws/views/trips.py | 21 ++++++--------------- 16 files changed, 55 insertions(+), 50 deletions(-) diff --git a/ws/feeds.py b/ws/feeds.py index d9c65ab1..f1c2b1ff 100644 --- a/ws/feeds.py +++ b/ws/feeds.py @@ -1,4 +1,7 @@ +from datetime import datetime + from django.contrib.syndication.views import Feed +from django.db.models import QuerySet from django.urls import reverse, reverse_lazy from django.utils import timezone @@ -10,27 +13,26 @@ class UpcomingTripsFeed(Feed): title = "MITOC Trips" - link = reverse_lazy("upcoming_trips") + link = reverse_lazy("trips") description = "Upcoming trips by the MIT Outing Club" - def items(self): - upcoming_trips = Trip.objects.filter(trip_date__gte=local_date()) - return upcoming_trips.order_by("-trip_date") + def items(self) -> QuerySet[Trip]: + return Trip.objects.filter(trip_date__gte=local_date()).order_by("-trip_date") - def item_title(self, item): + def item_title(self, item: Trip) -> str: return item.name - def item_description(self, item): + def item_description(self, item: Trip) -> str: return item.description - def item_link(self, item): + def item_link(self, item: Trip) -> str: return reverse("view_trip", args=[item.pk]) - def item_pubdate(self, item): + def item_pubdate(self, item: Trip) -> datetime: return item.time_created.astimezone(DEFAULT_TIMEZONE) - def item_author_name(self, item): + def item_author_name(self, item: Trip) -> str: return item.creator.name - def item_updateddate(self, item): + def item_updateddate(self, item: Trip) -> datetime: return item.last_edited.astimezone(DEFAULT_TIMEZONE) diff --git a/ws/models.py b/ws/models.py index 9c223475..4b4d3e4e 100644 --- a/ws/models.py +++ b/ws/models.py @@ -1040,6 +1040,11 @@ def __str__(self) -> str: class Trip(models.Model): + # Constant to control the default "page size" when viewing old trips. + # + # Putting this on the model so the business logic has one source of truth. + TRIPS_LOOKBACK = timedelta(days=365) + # When ordering trips which need approval, apply consistent ordering # (Defined here to keep the table's default ordering in sync with prev/next buttons ordering_for_approval: tuple[str, ...] = ( diff --git a/ws/templates/base.html b/ws/templates/base.html index 842a994c..7d151f38 100644 --- a/ws/templates/base.html +++ b/ws/templates/base.html @@ -78,7 +78,7 @@ @@ -91,7 +91,7 @@ {% else %} {# No need for a menu at all -- just show "trips" #} -
  • Trips
    • +
  • Trips
    • {% endif %} {% if user.is_superuser %} diff --git a/ws/templates/help/leaders/trip_admin.html b/ws/templates/help/leaders/trip_admin.html index de9f6752..9d1933c3 100644 --- a/ws/templates/help/leaders/trip_admin.html +++ b/ws/templates/help/leaders/trip_admin.html @@ -22,9 +22,8 @@

    What is the trip admin view?

    To explore the trip admin interface, first navigate to your trip (e.g. through - upcoming trips or - trips you're leading - ). + upcoming trips or + trips you're leading). Then, click the "Admin" tab to the right of the selected "Trip" tab.

    diff --git a/ws/templates/home.html b/ws/templates/home.html index a0f4a416..9a28141d 100644 --- a/ws/templates/home.html +++ b/ws/templates/home.html @@ -30,7 +30,12 @@

    Recent trips

    {% trip_list_table recent_trips %}
    -

    View all past trips

    +

    + + + Previous trips + +

    {% endif %} diff --git a/ws/templates/preferences/lottery/edit.html b/ws/templates/preferences/lottery/edit.html index 61ed76eb..707e6a3e 100644 --- a/ws/templates/preferences/lottery/edit.html +++ b/ws/templates/preferences/lottery/edit.html @@ -129,7 +129,7 @@

    {% if not ranked_signups %}

    You're not signed up for any upcoming trips.

      -
    1. Begin by signing up for any trips you're interested in.
      2. +
    2. Begin by signing up for any trips you're interested in.
      3. {% if has_paired_par %}
    3. Have your partner sign up for the same trips!
      4. {% endif %}
    4. Come back here to rank trips in order of preference.
    5. The next lottery run will try to place you on your most-preferred trip.
      6. @@ -139,7 +139,7 @@

      We strongly recommend signing up for at least four trips!

      Without some backup options, it's likely that you won't be placed on a trip at all. - Sign up for more trips to increase your odds of being placed on a trip. + Sign up for more trips to increase your odds of being placed on a trip.

      {% endif %} diff --git a/ws/templates/profile/membership.html b/ws/templates/profile/membership.html index 457ccda4..84a4d8b1 100644 --- a/ws/templates/profile/membership.html +++ b/ws/templates/profile/membership.html @@ -17,7 +17,7 @@

      Pay MITOC membership dues

      {% endif %} enables you to rent gear from the office, - participate in upcoming trips, + participate in upcoming trips, and stay at MITOC's cabins.

      diff --git a/ws/templates/trips/__create_or_edit.html b/ws/templates/trips/__create_or_edit.html index fc26e57f..7d73b22c 100644 --- a/ws/templates/trips/__create_or_edit.html +++ b/ws/templates/trips/__create_or_edit.html @@ -137,7 +137,7 @@

      Signup

      {% if form.instance.pk %} href="{% url 'view_trip' form.instance.pk %}" {% else %} - href="{% url 'upcoming_trips' %}" + href="{% url 'trips' %}" {% endif %} >Cancel diff --git a/ws/templates/trips/all/view.html b/ws/templates/trips/all/view.html index d1e68744..420e857c 100644 --- a/ws/templates/trips/all/view.html +++ b/ws/templates/trips/all/view.html @@ -56,11 +56,11 @@

      Past trips

      {% if one_year_prior %} - {# We hide this link when we're already viewing all past trips #} - + {# We hide this link when there are no trips earlier #} + Previous trips - + {% endif %}  MITOC home page

      diff --git a/ws/templates/trips/signup.html b/ws/templates/trips/signup.html index cd3b78d8..6e18a5a4 100644 --- a/ws/templates/trips/signup.html +++ b/ws/templates/trips/signup.html @@ -17,7 +17,7 @@

      Signup for {{ trip }}

      {% else %} -

      Not signing up for any given trip. Browse available trips?

      +

      Not signing up for any given trip. Browse available trips?

      {% endif %} {% endwith %} diff --git a/ws/tests/test_auth.py b/ws/tests/test_auth.py index ffbf8228..71a13250 100644 --- a/ws/tests/test_auth.py +++ b/ws/tests/test_auth.py @@ -78,7 +78,7 @@ def test_open_pages(self): "help-personal_info", "help-lottery", "help-signups", - "upcoming_trips", + "trips", "stats", ]: response = self.client.get(reverse(open_url)) diff --git a/ws/tests/views/test_trips.py b/ws/tests/views/test_trips.py index 66aac398..ab92491e 100644 --- a/ws/tests/views/test_trips.py +++ b/ws/tests/views/test_trips.py @@ -65,7 +65,7 @@ def _expect_link_for_date(soup, datestring): @freeze_time("2019-02-15 12:25:00 EST") -class UpcomingTripsViewTest(TestCase, Helpers): +class TripListViewTest(TestCase, Helpers): def test_upcoming_trips_without_filter(self): """With no default filter, we only show upcoming trips.""" response, soup = self._get("/trips/") diff --git a/ws/urls.py b/ws/urls.py index 58060519..4205c465 100644 --- a/ws/urls.py +++ b/ws/urls.py @@ -191,11 +191,11 @@ path("trips.rss", feeds.UpcomingTripsFeed(), name="rss-upcoming_trips"), # By default, `/trips/` shows only upcoming trips # Both views support filtering for trips after a certain date, though - path("trips/", views.UpcomingTripsView.as_view(), name="upcoming_trips"), + path("trips/", views.TripListView.as_view(), name="trips"), # With thousands of trips, we can no longer render them all on one page. path( "trips/all/", - RedirectView.as_view(url=reverse_lazy("upcoming_trips"), permanent=True), + RedirectView.as_view(url=reverse_lazy("trips"), permanent=True), name="all_trips", ), path("trips/signup/", views.SignUpView.as_view(), name="trip_signup"), diff --git a/ws/views/participant.py b/ws/views/participant.py index 696579cf..5c7b938d 100644 --- a/ws/views/participant.py +++ b/ws/views/participant.py @@ -537,15 +537,18 @@ def get_context_data(self, **kwargs): @staticmethod def render_landing_page(request: HttpRequest) -> HttpResponse: today = date_utils.local_date() - context = { - "current_trips": annotated_for_trip_list( - models.Trip.objects.filter(trip_date__gte=today).order_by( - "trip_date", "-time_created" - ) + current_trips = annotated_for_trip_list( + models.Trip.objects.filter(trip_date__gte=today).order_by( + "trip_date", "-time_created" ) + ) + + context = { + "current_trips": current_trips, + "previous_lookup_date": today - models.Trip.TRIPS_LOOKBACK, } - num_trips = len(context["current_trips"]) # Use len to avoid extra query + num_trips = len(current_trips) # Use len to avoid extra query # If we don't have many upcoming trips, show some recent ones if num_trips < 8: diff --git a/ws/views/signup.py b/ws/views/signup.py index 5abad8f5..6cc4a1ce 100644 --- a/ws/views/signup.py +++ b/ws/views/signup.py @@ -139,7 +139,7 @@ def dispatch(self, request, *args, **kwargs): class DeleteSignupView(DeleteView): model = models.SignUp - success_url = reverse_lazy("upcoming_trips") + success_url = reverse_lazy("trips") def get(self, request, *args, **kwargs): """Request is valid, but method is not (use POST).""" diff --git a/ws/views/trips.py b/ws/views/trips.py index b392794c..9658ca97 100644 --- a/ws/views/trips.py +++ b/ws/views/trips.py @@ -5,7 +5,7 @@ """ from collections import defaultdict -from datetime import date, timedelta +from datetime import date from typing import TYPE_CHECKING, Any, cast from urllib.parse import urlencode @@ -53,6 +53,8 @@ # There will never be any *older* trips, so we can save db queries. FIRST_TRIP_DATE = date(2015, 1, 10) +TRIPS_LOOKBACK = models.Trip.TRIPS_LOOKBACK + class TripView(DetailView): """Display the trip to both unregistered users and known participants. @@ -337,7 +339,7 @@ def dispatch(self, request, *args, **kwargs): class DeleteTripView(DeleteView, TripLeadersOnlyView): # type: ignore[misc] forbid_modifying_old_trips = True model = models.Trip - success_url = reverse_lazy("upcoming_trips") + success_url = reverse_lazy("trips") def get(self, request, *args, **kwargs): """Request is valid, but method is not (use POST).""" @@ -486,7 +488,6 @@ class TripListView(ListView): model = models.Trip template_name = "trips/all/view.html" context_object_name = "trip_queryset" - include_past_trips = True def get_queryset(self): trips = super().get_queryset() @@ -536,13 +537,13 @@ def get_context_data(self, **kwargs): if oldest_trip_date >= FIRST_TRIP_DATE: # Get approximately one year prior for use in paginating back in time. # (need not be exact/handle leap years) - context["one_year_prior"] = oldest_trip_date - timedelta(days=365) + context["one_year_prior"] = oldest_trip_date - TRIPS_LOOKBACK # By default, just show upcoming trips. context["current_trips"] = trips.filter(trip_date__gte=today) # However, if we've explicitly opted in to showing past trips, include them. - if self.include_past_trips or on_or_after_date: + if on_or_after_date: # Note that we need to sort past trips in descending order (most recent trips first). # This is because we have a cutoff in the past, and it would display strangely to sort ascending. context["past_trips"] = trips.filter(trip_date__lt=today).order_by( @@ -554,16 +555,6 @@ def get_context_data(self, **kwargs): return context -class UpcomingTripsView(TripListView): - """By default, view only upcoming (future) trips. - - If given a date, filter to only trips after that date (which may be past dates!) - """ - - # Default value, but past trips can appear by including a date filter - include_past_trips = False - - class TripSearchView(ListView, FormView): form_class = forms.TripSearchForm