fix(trends): fix hideWeekends zeroing out current in-progress week - #61791
Closed
Kd1880 wants to merge 1 commit into
Closed
fix(trends): fix hideWeekends zeroing out current in-progress week#61791Kd1880 wants to merge 1 commit into
Kd1880 wants to merge 1 commit into
Conversation
action['days'] was filtered using d.weekday() on raw date strings instead of parsed datetime objects, causing an AttributeError that corrupted results for all weekdays after the most recent weekend. Fix: parse date strings with strptime before calling .weekday(), consistent with how the main days/data/labels filtering works above. Fixes PostHog#61782
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
posthog/hogql_queries/insights/trends/trends_query_runner.py:747
**Missing error handling for unparseable `action["days"]` entries**
The main `days` loop (lines 712–717) wraps `strptime` in `try/except (ValueError, TypeError)` and keeps unparseable entries rather than crashing. This line has no such guard — if `action_days` ever contains a `None` or malformed entry, `strptime` raises `ValueError` uncaught, which would cause the entire `_filter_weekend_buckets` call to fail.
### Issue 2 of 3
posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py:1147-1150
**Conditional assertion may silently skip the fixed code path**
The entire `action["days"]` assertion is inside a guard that only executes when `result["action"]` is not `None` and contains a `"days"` key. For a plain `EventsNode` query these fields may not be populated, meaning the test would pass even if the fix on line 747 of `trends_query_runner.py` were reverted — the three-line block would simply never run. Either assert that `action` is not `None` for this query type, or use a query that is known to produce `action["days"]`.
### Issue 3 of 3
posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py:1115-1151
**Prefer parameterised tests per team style**
The codebase consistently uses `@parameterized.expand` for this kind of scenario check (see the `test_cohort_breakdown_with_lower_breakdown_limit` test immediately below). The fix is worth exercising across a few shapes, e.g. Mon–Tue, Mon–Wed, Mon–Thu, a full Mon–Sun (verifying weekends are still filtered), to make failures easier to localise and to match the team's stated preference for parameterised tests.
Reviews (1): Last reviewed commit: "fix(trends): fix hideWeekends zeroing ou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| action_days = new_result["action"]["days"] | ||
| new_result["action"] = {**new_result["action"]} | ||
| new_result["action"]["days"] = [d for d in action_days if d.weekday() < 5] | ||
| new_result["action"]["days"] = [d for d in action_days if datetime.strptime(d[:10], "%Y-%m-%d").weekday() < 5] |
Contributor
There was a problem hiding this comment.
action["days"] entries
The main days loop (lines 712–717) wraps strptime in try/except (ValueError, TypeError) and keeps unparseable entries rather than crashing. This line has no such guard — if action_days ever contains a None or malformed entry, strptime raises ValueError uncaught, which would cause the entire _filter_weekend_buckets call to fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql_queries/insights/trends/trends_query_runner.py
Line: 747
Comment:
**Missing error handling for unparseable `action["days"]` entries**
The main `days` loop (lines 712–717) wraps `strptime` in `try/except (ValueError, TypeError)` and keeps unparseable entries rather than crashing. This line has no such guard — if `action_days` ever contains a `None` or malformed entry, `strptime` raises `ValueError` uncaught, which would cause the entire `_filter_weekend_buckets` call to fail.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1147
to
+1150
| if result.get("action") is not None and "days" in result["action"]: | ||
| for day_str in result["action"]["days"]: | ||
| parsed = datetime.strptime(day_str[:10], "%Y-%m-%d") | ||
| assert parsed.weekday() < 5, f"Weekend day {day_str} found in action['days']" |
Contributor
There was a problem hiding this comment.
The entire action["days"] assertion is inside a guard that only executes when result["action"] is not None and contains a "days" key. For a plain EventsNode query these fields may not be populated, meaning the test would pass even if the fix on line 747 of trends_query_runner.py were reverted — the three-line block would simply never run. Either assert that action is not None for this query type, or use a query that is known to produce action["days"].
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py
Line: 1147-1150
Comment:
**Conditional assertion may silently skip the fixed code path**
The entire `action["days"]` assertion is inside a guard that only executes when `result["action"]` is not `None` and contains a `"days"` key. For a plain `EventsNode` query these fields may not be populated, meaning the test would pass even if the fix on line 747 of `trends_query_runner.py` were reverted — the three-line block would simply never run. Either assert that `action` is not `None` for this query type, or use a query that is known to produce `action["days"]`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1115
to
+1151
| def test_hide_weekends_action_days_mid_week_range(self): | ||
| """ | ||
| Regression test: hideWeekends zeroes out the current in-progress week. | ||
| action["days"] was filtered using d.weekday() on raw strings instead of | ||
| parsed datetimes, corrupting results for all days after the last weekend. | ||
| """ | ||
| self._create_test_events() | ||
|
|
||
| response = self._run_trends_query( | ||
| "2020-01-13", # Mon | ||
| "2020-01-16", # Thu — ends mid-week, no trailing weekend | ||
| IntervalType.DAY, | ||
| [EventsNode(event="$pageview")], | ||
| trends_filters=TrendsFilter(hideWeekends=True), | ||
| ) | ||
|
|
||
| result = response.results[0] | ||
|
|
||
| # All four days are weekdays — none should be filtered | ||
| assert result["days"] == [ | ||
| "2020-01-13", # Mon | ||
| "2020-01-14", # Tue | ||
| "2020-01-15", # Wed | ||
| "2020-01-16", # Thu | ||
| ], f"Expected all weekdays present, got: {result['days']}" | ||
|
|
||
| # Data must not be zeroed out | ||
| assert any(v > 0 for v in result["data"]), ( | ||
| f"All values zero — mid-week days incorrectly zeroed: {result['data']}" | ||
| ) | ||
|
|
||
| # action["days"] must not be corrupted | ||
| if result.get("action") is not None and "days" in result["action"]: | ||
| for day_str in result["action"]["days"]: | ||
| parsed = datetime.strptime(day_str[:10], "%Y-%m-%d") | ||
| assert parsed.weekday() < 5, f"Weekend day {day_str} found in action['days']" | ||
|
|
Contributor
There was a problem hiding this comment.
The codebase consistently uses @parameterized.expand for this kind of scenario check (see the test_cohort_breakdown_with_lower_breakdown_limit test immediately below). The fix is worth exercising across a few shapes, e.g. Mon–Tue, Mon–Wed, Mon–Thu, a full Mon–Sun (verifying weekends are still filtered), to make failures easier to localise and to match the team's stated preference for parameterised tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py
Line: 1115-1151
Comment:
**Prefer parameterised tests per team style**
The codebase consistently uses `@parameterized.expand` for this kind of scenario check (see the `test_cohort_breakdown_with_lower_breakdown_limit` test immediately below). The fix is worth exercising across a few shapes, e.g. Mon–Tue, Mon–Wed, Mon–Thu, a full Mon–Sun (verifying weekends are still filtered), to make failures easier to localise and to match the team's stated preference for parameterised tests.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
|
Thanks for the contribution! Sorry this wasn't assigned to us recently. I actually fixed this already too |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When "Hide weekend data" is enabled on a daily Trends insight with a date range ending mid-week (e.g. last 30 days ending on a Thursday), all weekday data points after the most recent Saturday/Sunday are zeroed out instead of only Saturday and Sunday being removed.
Closes #61782
Changes
In
_filter_weekend_buckets(trends_query_runner.py),action["days"]was filtered usingd.weekday()directly on raw date strings. Since strings don't have a.weekday()method, this raised anAttributeErrorthat corrupted the result for the entire current in-progress week.Fixed by parsing each date string with
datetime.strptime(d[:10], "%Y-%m-%d")before calling.weekday(), consistent with how the maindays/data/labelsfiltering already works in the same function.How did you test this code?
Added regression test
test_hide_weekends_action_days_mid_week_rangecovering a date range ending mid-week (Mon–Thu) with no trailing weekend, verifying:daysaction["days"]contains only valid weekday stringsAgent context
Investigated with Claude (claude.ai). Root cause was identified by reading
_filter_weekend_bucketsand spotting the.weekday()call on raw strings. Fix and test were written with AI assistance — I have read and understood all changed code and can explain every line.