Skip to content

Add cached_method decorator and refactor Flight views/clicks caching#1153

Open
Copilot wants to merge 3 commits intomainfrom
copilot/add-tests-for-db-queries
Open

Add cached_method decorator and refactor Flight views/clicks caching#1153
Copilot wants to merge 3 commits intomainfrom
copilot/add-tests-for-db-queries

Conversation

Copy link
Contributor

Copilot AI commented Feb 26, 2026

  • Understand the existing code structure and PR Remove aggregation from critical path - use local cache #1150
  • Add cached_method decorator to adserver/utils.py
  • Refactor Flight.views_today() and Flight.clicks_today() to use @cached_method
  • Add tests for cached_method in test_utils.py
  • Add DB query count tests for views_today/clicks_today in test_models.py
  • Run linting (pre-commit run --all-files) - all hooks pass
  • Run full test suite - all tests pass (1 pre-existing unrelated failure in test_ad_bulk_create_view)
  • CodeQL security check - no alerts
  • Reduce FakeModel duplication in CachedMethodTest via _make_obj factory helper

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Co-authored-by: ericholscher <25510+ericholscher@users.noreply.github.com>
Copilot AI changed the title [WIP] Add tests for existing DB queries using TDD Add cached_method decorator and refactor Flight views/clicks caching Feb 26, 2026
@ericholscher ericholscher marked this pull request as ready for review February 26, 2026 06:12
@ericholscher ericholscher requested a review from a team as a code owner February 26, 2026 06:12
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked over this and it seems generally ready.

@copilot reduce the fake model duplication in the tests if it isn't too clunky.

@ericholscher
Copy link
Member

@copilot research other places in the code where a similar approach would work well, and open a new PR adding the decorator there.

Co-authored-by: ericholscher <25510+ericholscher@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 26, 2026

Looked over this and it seems generally ready.

@copilot reduce the fake model duplication in the tests if it isn't too clunky.

Done in 3e28d82. Each test now calls self._make_obj(**attrs) which defines FakeModel and the decorated method once, then sets pk and any annotated attributes on the returned instance.

@copilot research other places in the code where a similar approach would work well, and open a new PR adding the decorator there.

I'm not able to open new PRs from this environment. Looking at the codebase, the most promising candidate is Campaign.total_value() (models.py ~line 740) which already has a manual hasattr(self, "campaign_total_value") guard but no local-cache layer — it would benefit directly from @cached_method("campaign_total_value"). The Advertisement.total_views() / total_clicks() methods (models.py ~line 2307-2317) also do bare aggregations with no caching at all. Happy to add those in a follow-up if you'd like to open a tracking issue or point me at a branch.

@davidfischer
Copy link
Collaborator

I gave a cursory look and this looks good. I'm not planning to merge this today for the release. Specifically, what I'm most concerned about, is the admin dashboard performance where we're fetching 100s of flights at a time and we had an in-built optimization there that this removes. There's probably a quick fix for that though.

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.

3 participants