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

RecurringEvents.iter_after() not producing correct results #3022

Open
JoeyStk opened this issue Aug 26, 2024 · 9 comments · May be fixed by #3026
Open

RecurringEvents.iter_after() not producing correct results #3022

JoeyStk opened this issue Aug 26, 2024 · 9 comments · May be fixed by #3026
Assignees
Labels
bug Something isn't working prio: low Not urgent, can be resolved in the distant future.
Milestone

Comments

@JoeyStk
Copy link
Contributor

JoeyStk commented Aug 26, 2024

Describe the Bug

The recurrence rule in #3018 is (still) broken for iter_after(). As long as this is not resolved, our release is blocked

Steps to Reproduce

  1. Start your dev server, go to events
  2. (delete any existing events if you want to make step 5 easier)
  3. Create a new event, set as recurrence rule every last Thursday starting in August (every month, last week, Thursday, every 1st...)
  4. In your browser, go to the path f"/api/v3/{region_slug}/{language_slug}/events/" of your running dev server
  5. See that for every month the 29th is returned

Expected Behavior

Show for an event that happens every last Thursday the list of last Thursdays (so August 29th, September 26th, October 31st, November 28th, etc.).

Actual Behavior

(1) The current state on production is that only August 29th, October 31st and January are shown (months that have a fifth week). (2) #2992 attempted to fix this, but on the test system now the 29th of a month is shown.

Additional Information

App screenshot from the test system (potentially showing a different bug in the app)

grafik State on production, before #2992
grafik State after #2992

Both are not what we expect

@JoeyStk JoeyStk added bug Something isn't working prio: urgent Needs to be resolved now(?) labels Aug 26, 2024
@JoeyStk JoeyStk added this to the 24Q3 milestone Aug 26, 2024
@PeterNerlich
Copy link
Collaborator

On a side note, I think the way we implement and display monthly recurrence for a weekday is misleading. We display First/Second/Third/Last Week, but we actually mean the nth occurrence of that weekday in the month. (I would expect the month to be segmented into weeks from weekend to weekend and the counting be based on that with what we current show – but the start of those equivalency groups is actually based on which weekday the month starts or ends on)

@PeterNerlich PeterNerlich self-assigned this Aug 26, 2024
@PeterNerlich
Copy link
Collaborator

Ah, #2992 misunderstood the original issue #2989 for an invalid rendering of the iCal format, a bug they likely determined to be the intended target because of the difficulties of identifying the part of the CMS where the event data is handed over to the app (navigating from the function implementing the generation of event instances to our actual API endpoint exposing it proved confusing, even if it was not the worst one I encountered so far in this project).
I decided to change how we store and handle the intention of "last occurrence in month", rendering the functional changes of #2992 unnecessary, but it should be closer to how iCal does it and be less ambiguous (using negative values to count from the end of the month instead of the start)

@david-venhoff
Copy link
Member

I think this issue is invalid:
The test 5 Woche event is set to reccur every 3rd week (contradicting the title), if this is the correct event that is shown in the screenshot: https://cms-test.integreat-app.de/muenchen/events/de/4599/edit/

However, that still does not explain the dates listed at https://webnext.integreat.app/muenchen/de/events/test-5-woche ^^

That appears to be an app issue though, because we pass the correct recurrence rule to it, as can be seen in the network manager in the browser devtools of the page. Here is the relevant snippet:
image

As can be seen, the recurrence rule is correctly set to repeat every third week of the month.

Because the combine_recurring parameter is set, our iter_after function never gets called even if it is technically incorrect right now.

@david-venhoff
Copy link
Member

I think the bug is that the frontend somehow ignores the byday completely. To reproduce:

  1. open https://webnext.integreat.app/testumgebung/de/events/test-monthly
  2. Open your browser devtools and inspect the request to cms-test.integreat-app.de/api/v3/testumgebung/de/events/?combine_recurring=True
  3. See the the recurrence rule is DTSTART:20240827T220000RRULE:FREQ=MONTHLY;BYDAY=-1WE, which is correct
  4. Click on export as ical (Als ical exportieren)
  5. Select Diese und alle zukünftigen Veranstaltungen
  6. open the file in a text editor and see the the recurrence rule is missing the BYDAY parameter, which explains the listed dates in the frontend: RRULE:FREQ=MONTHLY;INTERVAL=1

@PeterNerlich
Copy link
Collaborator

Are we positive that the app only uses the endpoint with ?combine_recurring=True?

@david-venhoff
Copy link
Member

I am pretty sure that we currently use the endpoint with this flag only, at least for the the event pages (As can be seen in the browser devtools). I also had a call with Leandra, and she is also relatively positive that this is an app issue. If that is the case, she plans to open an issue at the app repository

@LeandraH
Copy link

Yes, I could fix the issue in the frontend in my local build. I rewrote the parsing of the recurrence rule a couple of weeks ago to fix a different bug and seem to have introduced this one, sorry! I'm currently adding more tests and will open a PR (and an issue) in the app repo later today.

@LeandraH
Copy link

Are we positive that the app only uses the endpoint with ?combine_recurring=True?

Yes, at least currently :)

@PeterNerlich PeterNerlich changed the title Recurrence rule in release is broken RecurringEvents.iter_after() not producing correct results Aug 28, 2024
@PeterNerlich PeterNerlich added prio: low Not urgent, can be resolved in the distant future. and removed prio: urgent Needs to be resolved now(?) labels Aug 28, 2024
@PeterNerlich PeterNerlich modified the milestones: 24Q3, Backlog Sep 23, 2024
@PeterNerlich
Copy link
Collaborator

Moved to backlog since the API endpoint is unused anyway at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working prio: low Not urgent, can be resolved in the distant future.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@PeterNerlich @LeandraH @JoeyStk @david-venhoff and others