-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): correctly query open periods within date range #103924
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
Conversation
7cbcd78 to
efbbc31
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| group_open_periods = ( | ||
| GroupOpenPeriod.objects.filter( | ||
| group=group, | ||
| date_started__lte=query_end, | ||
| ) | ||
| .filter(Q(date_ended__gte=query_start) | Q(date_ended__isnull=True)) | ||
| .order_by("-date_started") | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's a bit easier to grok if you phrase it as
"Either the end time is between the [start, end] or the start time is between [start, end]"
| group_open_periods = ( | |
| GroupOpenPeriod.objects.filter( | |
| group=group, | |
| date_started__lte=query_end, | |
| ) | |
| .filter(Q(date_ended__gte=query_start) | Q(date_ended__isnull=True)) | |
| .order_by("-date_started") | |
| ) | |
| group_open_periods = GroupOpenPeriod.objects.filter( | |
| Q(date_ended__lte=query_end) & Q(date_ended__gte=query_start) | |
| | (Q(date_started__lte=query_end) & Q(date_started__gte=query_start)), | |
| group=group, | |
| ).order_by("-date_started") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately this doesn't account for open periods that extend beyond the query period in both directions 🥲
ex. if i have an open period from day 0 to day 7 and i query for day 2 to 3, then the open period wouldn't be returned
i'll update the comment to make it easier to grok the current logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sadge
perhaps add this specific case as another condition to the query then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, explain what cases the current query covers, I think that's what got me
we were only returning open periods that started during/after
query_start, which does not include open periods that started beforequery_startbut were still ongoing during the query range