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

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

Conversation

alvindera97
Copy link
Contributor

@alvindera97 alvindera97 commented Aug 8, 2024

What

Closes

Passed variables to the time_tag template [time_tag.html] that checks if an event was scheduled to start or end with the current year.
More events are created to test particular scenarios of events especially events set to start or end at a future year.
…t Year

The time tag now displays the year when an event will occur. This is only for events that have been scheduled to start or end in at a future year. The accompanying functional tests have also been included.
All test data concerning the provision of data to serve the functional tests have been moved to the functional test.
As it improves readability.
All other test data at test_views.py was reset
to accommodate for the reduction in number of test data instances.
Since the current CI at the main branch does not support selenium [web driver] operations, the functional test which depends on selenium to run has been removed.
@alvindera97
Copy link
Contributor Author

alvindera97 commented Aug 28, 2024

Good day everyone, I hope you're all doing well.

I initially worked on this issue (#1701) some time ago, and it was opened in 2020. The problem involved events with start or end dates that are not in the current year (i.e. not matching datetime.datetime.now().year on the server). These events were displayed without indicating the year, which could cause confusion for users.

While event names often include the year, displaying the year directly with the event dates on the frontend provides additional clarity and improves user experience by avoiding any ambiguity.

If possible, I'd appreciate a review of my pull request. I’m happy to answer any questions you may have and will do my best to clarify any details.

Thank you for your time, and I look forward to your feedback.

Best regards,
Godwin

@alvindera97
Copy link
Contributor Author

After reviewing some other pull requests on the repo, I decided that adding screenshots of before and after would better give a visual summary of what has been done

Before:

Screenshot from 2024-09-15 17-01-50

After:

Screenshot from 2024-09-15 16-57-00

@JacobCoffee JacobCoffee changed the title Fix issue 1701: Events page displays year for events scheduled to start or end at a future year fix(#1701): Events page displays year for events scheduled to start or end at a future year Sep 16, 2024
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

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

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.

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

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.

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.

Calendar events should show the year for events which are not in the current year
3 participants