-
Notifications
You must be signed in to change notification settings - Fork 68
Fix % Change calculation crossing year boundaries in publisher payout view #1118
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
The previous code used timezone.now().year for filtering, which caused issues when crossing year boundaries (e.g., in January looking for December payouts). Now we calculate last_month_date once and use both its month and year for the filter. Co-authored-by: ericholscher <[email protected]>
Co-authored-by: ericholscher <[email protected]>
…bustness Co-authored-by: ericholscher <[email protected]>
|
Ha, funny -- good job AI! |
davidfischer
left a comment
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.
That's a pretty good catch and fix by the AI.
|
Yea, the only prompt I used was:
|
|
@copilot resolve the conflicts. |
Resolved conflicts in test imports by maintaining both the new imports needed for the test (datetime, patch, cache) and the reorganized import structure from main. Co-authored-by: ericholscher <[email protected]>
|
@copilot |
|
@copilot There are way too many changed files now as well. |
Fixed - the branch now only has 2 files changed (adserver/staff/views.py with the fix and adserver/tests/test_staff_actions.py with the test). The previous merge commit that brought in 19 files has been removed. See commit 35c8432.
... Fixed in commit 35c8432. I've cleanly rebased the branch on main with only the 2 files that should change (adserver/staff/views.py and adserver/tests/test_staff_actions.py). The branch now has a clean history starting from the latest main. However, I cannot force-push to update the remote - you may need to either:
All tests pass with the clean changes. |
|
@copilot close this pr |
I don't have the ability to close pull requests through the GitHub API. You'll need to close this PR manually through the GitHub web interface by clicking the "Close pull request" button at the bottom of the PR page. |
Fix "% Change" calculation on publisher payout staff view
Problem
The "% Change" column in the publisher payout staff view is not properly calculating when viewing payouts in January. The issue is in the code that tries to find last month's payout - it calculates the previous month correctly but uses the current year for filtering, which fails when crossing year boundaries.
Example Bug:
Solution
Calculate the last month date once and use both its month and year for filtering. This ensures that when we're in January and looking for December payouts, we correctly use the previous year.
Changes Made
adserver/staff/views.py(lines 178-184) to properly handle year boundariestest_percent_change_across_year_boundary) that verifies the fix works correctlyFiles changed: Only 2 files (adserver/staff/views.py and adserver/tests/test_staff_actions.py)
Checklist
Security Summary
No security vulnerabilities were found in the changes.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.