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.

Sign up for GitHub

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

Jump to bottom

fix(#1701): Events page displays year for events scheduled to start or end at a future year #2500

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+130 −9
Open

fix(#1701): Events page displays year for events scheduled to start or end at a future year #2500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1502c10
Add Methods To Check If Event Starts And Ends This Year
alvindera97 Dec 25, 2020
4198812
Set Up Templates For Querying Start And End Years
alvindera97 Dec 25, 2020
ff1ad8b
Insert New Test Data And Update test_views.py
alvindera97 Dec 25, 2020
e3125d9
Time Tag Now Shows Year For Events With Details Not Within The Curren…
alvindera97 Dec 25, 2020
178742b
Move All Test Data To Functional Test
alvindera97 Dec 25, 2020
e0fdf7c
Functional Test For Displaying Year Of Event For Future Events Now Im…
alvindera97 Dec 27, 2020
115af08
Remove Functional Test For Displaying Year Of Future Event
alvindera97 Dec 27, 2020
aa4d55b
Handle Case When Call To Next Event Returns None
alvindera97 Apr 9, 2022
207a0af
Fix Erroneous Addition To Dev Requirements
alvindera97 Aug 8, 2024
c7cfabd
Merge remote-tracking branch 'upstream/main' into fix-issue-1701
alvindera97 Aug 8, 2024
fa6e0ce
Merge branch 'main' into fix-issue-1701
JacobCoffee Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions events/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,20 @@ def next_time(self):
except IndexError:
return None

def is_scheduled_to_start_this_year(self) -> bool:
if self.next_time:
current_year: int = datetime.datetime.now().year
if self.next_time.dt_start.year == current_year:
return True
return False

def is_scheduled_to_end_this_year(self) -> bool:
if self.next_time:
current_year: int = datetime.datetime.now().year
if self.next_time.dt_end.year == current_year:
return True
return False

@property
def previous_time(self):
now = timezone.now()
Expand Down
23 changes: 22 additions & 1 deletion events/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ def test_recurring_event(self):
self.assertEqual(self.event.next_time.dt_start, recurring_time_dtstart)
self.assertTrue(rt.valid_dt_end())


rt.begin = now - datetime.timedelta(days=5)
rt.finish = now - datetime.timedelta(days=3)
rt.save()
Expand Down Expand Up @@ -186,3 +185,25 @@ def test_event_previous_event(self):
# 'Event.previous_event' can return None if there is no
# OccurringRule or RecurringRule found.
self.assertIsNone(self.event.previous_event)

def test_scheduled_to_start_this_year_method(self):
now = seconds_resolution(timezone.now())

occurring_time_dtstart = now + datetime.timedelta(days=3)
Copy link
Contributor Author

@alvindera97 alvindera97 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmmm... Thinking about this test again. On the off chance that tests are run between the 29th of December and the 31st of December, this test will fail since the next year will be set to 3 days ahead...

So if tests are run on the 29th of December, occurring_time_dtstart will be set to 3 days from then which would be the 1st of January (the following year).

Since the date comparison is made against datetime.datetime.now().year at Event.is_scheduled_to_start_this_year and Event.is_scheduled_to_end_this_year, (https://github.com/alvindera97/pythondotorg/blob/fa6e0ced4827a1652aa2eef0a80db22b1a2c4fe2/events/models.py#L184-L189) having a timedelta of 3 days makes running tests within the last 3 days of the year flaky.

Copy link
Member

Choose a reason for hiding this comment

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

You can use something like https://github.com/spulec/freezegun to freeze time so the tests run reliably.

Copy link
Contributor Author

@alvindera97 alvindera97 Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You can use something like https://github.com/spulec/freezegun to freeze time so the tests run reliably.

Thanks, @hugovk, for suggesting freezegun to handle the time-dependent tests. I have tested the updates locally, and I’m ready to push the changes to the current pull request. However, before proceeding, I wanted to get clarity on one remaining concern regarding the introduction of external dependencies into the project.

Specifically, I’m unsure of the current team policy regarding the addition of new dependencies. My concern revolves around potential risks that come with relying on third-party libraries, such as maintenance issues, security vulnerabilities, and compatibility with future versions of Python/Django.

In this case, while freezegun is a robust library for freezing time, I wonder if it’s necessary to introduce an entire dependency solely to address the time-delta requirements of the Event object in the tests. I'm considering whether there might be an alternative approach that avoids the need for an external library, such as directly mocking time or using built-in Python features to clamp dates.

I would appreciate your thoughts on whether the use of freezegun is acceptable for this situation, or if the team would prefer exploring another solution.

However, as an example, I thought of reducing the time-deltas. Here's a snippet without freezegun:

    def test_scheduled_to_start_this_year_method(self):
        now = seconds_resolution(timezone.now())

        occurring_time_dtstart = now + datetime.timedelta(seconds=10)
        OccurringRule.objects.create(
            event=self.event,
            dt_start=occurring_time_dtstart,
            dt_end=occurring_time_dtstart + datetime.timedelta(days=3)
        )
        self.assertTrue(self.event.is_scheduled_to_start_this_year())

        OccurringRule.objects.get(event=self.event).delete()

        event_not_scheduled_to_start_this_year_occurring_time_dtstart = now + datetime.timedelta(days=365)
        OccurringRule.objects.create(
            event=self.event,
            dt_start=event_not_scheduled_to_start_this_year_occurring_time_dtstart,
            dt_end=event_not_scheduled_to_start_this_year_occurring_time_dtstart + datetime.timedelta(days=3)
        )

        self.assertFalse(self.event.is_scheduled_to_start_this_year())

    def test_scheduled_to_end_this_year_method(self):
        now = seconds_resolution(timezone.now())
        occurring_time_dtstart = now + datetime.timedelta(seconds=10)

        OccurringRule.objects.create(
            event=self.event,
            dt_start=occurring_time_dtstart,
            dt_end=occurring_time_dtstart
        )

        self.assertTrue(self.event.is_scheduled_to_end_this_year())

        OccurringRule.objects.get(event=self.event).delete()

        event_not_scheduled_to_end_this_year_occurring_time_dt_end = now + datetime.timedelta(days=365)

        OccurringRule.objects.create(
            event=self.event,
            dt_start=now,
            dt_end=event_not_scheduled_to_end_this_year_occurring_time_dt_end
        )

        self.assertFalse(self.event.is_scheduled_to_end_this_year())

Looking forward to your feedback 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your question, it's always good to keep these things in mind. My feeling is that it would be okay, as there are already a number of third-party dependencies, but I'll leave it to the maintainers to answer more concretely.

An alternative would be to use something like unitest.mock to fake the date returned by datetime.timedelta.

OccurringRule.objects.create(
event=self.event,
dt_start=occurring_time_dtstart,
dt_end=occurring_time_dtstart
)
self.assertTrue(self.event.is_scheduled_to_start_this_year())

def test_scheduled_to_end_this_year_method(self):
now = seconds_resolution(timezone.now())

occurring_time_dtstart = now + datetime.timedelta(days=3)
Copy link
Contributor Author

@alvindera97 alvindera97 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same comment here concerning

occurring_time_dtstart = now + datetime.timedelta(days = 3)

with previous comment at https://github.com/python/pythondotorg/pull/2500/files#r1764230987

If it's fine with everyone, I'll await feedback first before making any more changes to the pull request so I'd attempt resolving all concerns with as minimal back-and-forth as possible.

Thank you for your time and best regards everyone.

OccurringRule.objects.create(
event=self.event,
dt_start=occurring_time_dtstart,
dt_end=occurring_time_dtstart
)
self.assertTrue(self.event.is_scheduled_to_end_this_year())
63 changes: 57 additions & 6 deletions events/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from django.test import TestCase
from django.utils import timezone

from ..models import Calendar, Event, EventCategory, EventLocation, RecurringRule
from ..models import Calendar, Event, EventCategory, EventLocation, RecurringRule, OccurringRule
from ..templatetags.events import get_events_upcoming
from users.factories import UserFactory

Expand All @@ -18,6 +18,11 @@ def setUpTestData(cls):
cls.calendar = Calendar.objects.create(creator=cls.user, slug="test-calendar")
cls.event = Event.objects.create(creator=cls.user, calendar=cls.calendar)
cls.event_past = Event.objects.create(title='Past Event', creator=cls.user, calendar=cls.calendar)
cls.event_single_day = Event.objects.create(title="Single Day Event", creator=cls.user, calendar=cls.calendar)
cls.event_starts_at_future_year = Event.objects.create(title='Event Starts Following Year',
creator=cls.user, calendar=cls.calendar)
cls.event_ends_at_future_year = Event.objects.create(title='Event Ends Following Year',
creator=cls.user, calendar=cls.calendar)

cls.now = timezone.now()

Expand All @@ -34,12 +39,32 @@ def setUpTestData(cls):
begin=cls.now - datetime.timedelta(days=2),
finish=cls.now - datetime.timedelta(days=1),
)
cls.rule_single_day = OccurringRule.objects.create(
event=cls.event_single_day,
dt_start=recurring_time_dtstart,
dt_end=recurring_time_dtstart
)
cls.rule_future_start_year = OccurringRule.objects.create(
event=cls.event_starts_at_future_year,
dt_start=recurring_time_dtstart + datetime.timedelta(weeks=52),
dt_end=recurring_time_dtstart + datetime.timedelta(weeks=53),
)
cls.rule_future_end_year = OccurringRule.objects.create(
event=cls.event_ends_at_future_year,
dt_start=recurring_time_dtstart,
dt_end=recurring_time_dtend + datetime.timedelta(weeks=52)
)

def tearDown(self) -> None:
from django.core.cache import cache
cache.clear()

def test_events_homepage(self):
url = reverse('events:events')
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
self.assertEqual(len(response.context['object_list']), 1)
self.assertEqual(len(response.context['object_list']), 4)
self.assertIn(Event.objects.last().title, response.content.decode())

def test_calendar_list(self):
calendars_count = Calendar.objects.count()
Expand All @@ -54,7 +79,7 @@ def test_event_list(self):
response = self.client.get(url)

self.assertEqual(response.status_code, 200)
self.assertEqual(len(response.context['object_list']), 1)
self.assertEqual(len(response.context['object_list']), 4)

url = reverse('events:event_list_past', kwargs={"calendar_slug": 'unexisting'})
response = self.client.get(url)
Expand Down Expand Up @@ -114,7 +139,7 @@ def test_event_list_date(self):
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
self.assertEqual(response.context['object'], dt.date())
self.assertEqual(len(response.context['object_list']), 2)
self.assertEqual(len(response.context['object_list']), 5)

def test_eventlocation_list(self):
venue = EventLocation.objects.create(
Expand Down Expand Up @@ -150,13 +175,39 @@ def test_event_detail(self):
self.assertEqual(self.event, response.context['object'])

def test_upcoming_tag(self):
self.assertEqual(len(get_events_upcoming()), 1)
self.assertEqual(len(get_events_upcoming()), 4)
self.assertEqual(len(get_events_upcoming(only_featured=True)), 0)
self.rule.begin = self.now - datetime.timedelta(days=3)
self.rule.finish = self.now - datetime.timedelta(days=2)
self.rule.save()
self.assertEqual(len(get_events_upcoming()), 0)
self.assertEqual(len(get_events_upcoming()), 3)

def test_event_starting_future_year_displays_relevant_year(self):
event = self.event_starts_at_future_year
url = reverse('events:events')
response = self.client.get(url)
self.assertIn(
f'<span id="start-{event.id}">',
response.content.decode()
)

def test_event_ending_future_year_displays_relevant_year(self):
event = self.event_ends_at_future_year
url = reverse('events:events')
response = self.client.get(url)
self.assertIn(
f'<span id="end-{event.id}">',
response.content.decode()
)

def test_events_scheduled_current_year_does_not_display_current_year(self):
event = self.event_single_day
url = reverse('events:events')
response = self.client.get(url)
self.assertIn( # start date
f'<span id="start-{event.id}" class="say-no-more">',
response.content.decode()
)

class EventSubmitTests(TestCase):
event_submit_url = reverse_lazy('events:event_submit')
Expand Down
8 changes: 8 additions & 0 deletions templates/events/event_list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@ <h2 class="widget-title"><span aria-hidden="true" class="icon-calendar"></span>U
<h3 class="event-title"><a href="{{ object.get_absolute_url }}">{{ object.title|striptags }}</a></h3>
<p>
{% with object.next_time as next_time %}
{% with object.is_scheduled_to_start_this_year as scheduled_start_this_year %}
{% with object.is_scheduled_to_end_this_year as scheduled_end_this_year %}
{% include "events/includes/time_tag.html" %}
{% endwith %}
{% endwith %}
{% endwith %}

{% if object.venue %}
<span class="event-location">{% if object.venue.url %}<a href="{{ object.venue.url }}">{% endif %}{{ object.venue.name }}{% if object.venue.url %}</a>{% endif %}{% if object.venue.address %}, {{ object.venue.address }}{% endif %}</span>
Expand All @@ -65,8 +69,12 @@ <h3 class="widget-title just-missed">You just missed...</h3>
<h3 class="event-title"><a href="{{ object.get_absolute_url }}">{{ object.title|striptags }}</a></h3>
<p>
{% with object.previous_time as next_time %}
{% with object.is_scheduled_to_start_this_year as scheduled_start_this_year %}
{% with object.is_scheduled_to_end_this_year as scheduled_end_this_year %}
{% include "events/includes/time_tag.html" %}
{% endwith %}
{% endwith %}
{% endwith %}

{% if object.venue %}
<span class="event-location">{% if object.venue.url %}<a href="{{ object.venue.url }}">{% endif %}{{ object.venue.name }}{% if object.venue.url %}</a>{% endif %}{% if object.venue.address %}, {{ object.venue.address }}{% endif %}</span>
Expand Down
31 changes: 29 additions & 2 deletions templates/events/includes/time_tag.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
{% if next_time.single_day %}
<time datetime="{{ next_time.dt_start|date:'c' }}">{{ next_time.dt_start|date:"d N" }}<span class="say-no-more"> {{ next_time.dt_start|date:"Y" }}</span>{% if not next_time.all_day %} {{ next_time.dt_start|date:"fA"|lower }} {{ next_time.dt_start|date:"e" }}{% if next_time.valid_dt_end %} – {{ next_time.dt_end|date:"fA"|lower }} {{ next_time.dt_end|date:"e" }}{% endif %}{% endif %}</time>
<time datetime="{{ next_time.dt_start|date:'c' }}">{{ next_time.dt_start|date:"d N" }}
<span id="start-{{ object.id }}"{% if scheduled_start_this_year %} class="say-no-more"{% endif %}>
{{ next_time.dt_start|date:"Y" }}
</span>

<span id="start-{{ object.id }}"{% if scheduled_end_this_year %} class="say-no-more"{% endif %}>
{{ next_time.dt_start|date:"Y" }}
</span>

{% if not next_time.all_day %}
{{ next_time.dt_start|date:"fA"|lower }} {{ next_time.dt_start|date:"e" }}
{% if next_time.valid_dt_end %} – {{ next_time.dt_end|date:"fA"|lower }}
{{ next_time.dt_end|date:"e" }}
{% endif %}
{% endif %}
</time>
{% else %}
<time datetime="{{ next_time.dt_start|date:'c' }}">{{ next_time.dt_start|date:"d N" }}{% if next_time.valid_dt_end %} &ndash; {{ next_time.dt_end|date:"d N" }}{% endif %} <span class="say-no-more"> {{ next_time.dt_end|date:"Y" }}</span></time>
<time datetime="{{ next_time.dt_start|date:'c' }}">{{ next_time.dt_start|date:"d N" }}
<span id="start-{{ object.id }}"{% if scheduled_start_this_year %} class="say-no-more"{% endif %}>
{{ next_time.dt_start|date:"Y" }}
</span>

{% if next_time.valid_dt_end %} &ndash;
{{ next_time.dt_end|date:"d N" }}
{% endif %}

<span id="end-{{ object.id }}"{% if scheduled_end_this_year %} class="say-no-more"{% endif %}>
{{ next_time.dt_end|date:"Y" }}
</span>
</time>
{% endif %}