Add needs_price_update field to Purchase model #94

2026-05-12T12:05:30Z

lukas commented

2026-05-12 12:05:30 +00:00

Resolves #92

Replace fragile price change detection in Purchase.save() with a lazy dirty flag approach. A pre_save/post_save signal pair detects price/currency changes without extra DB queries, and convert_prices() uses the flag to determine which purchases need conversion.

Changes

Add needs_price_update BooleanField with db_index
Add pre_save signal to store old price/currency values
Add post_save signal to set needs_price_update=True when price/currency changes
Simplify Purchase.save() to remove DB reload + comparison logic
Remove price_or_currency_differ_from() method
Update convert_prices() to filter on needs_price_update flag
Extract _get_exchange_rate() and _save_converted_price() helpers
Add tests for the new behavior

Resolves #92 Replace fragile price change detection in Purchase.save() with a lazy dirty flag approach. A pre_save/post_save signal pair detects price/currency changes without extra DB queries, and convert_prices() uses the flag to determine which purchases need conversion. ### Changes - Add `needs_price_update` BooleanField with db_index - Add pre_save signal to store old price/currency values - Add post_save signal to set needs_price_update=True when price/currency changes - Simplify Purchase.save() to remove DB reload + comparison logic - Remove `price_or_currency_differ_from()` method - Update `convert_prices()` to filter on needs_price_update flag - Extract `_get_exchange_rate()` and `_save_converted_price()` helpers - Add tests for the new behavior

lukas added 1 commit 2026-05-12 12:05:30 +00:00

Add needs_price_update field to Purchase model

Django CI/CD / test (push) Successful in 22s

Details

Django CI/CD / build-and-push (push) Has been skipped

Details

e3b53cd4a9

Replace fragile price change detection in Purchase.save() with a
lazy dirty flag approach. A pre_save/post_save signal pair detects
price/currency changes without extra DB queries, and convert_prices()
uses the flag to determine which purchases need conversion.

- Add needs_price_update BooleanField with db_index
- Add pre_save signal to store old price/currency values
- Add post_save signal to set needs_price_update=True when price/currency changes
- Simplify Purchase.save() to remove DB reload + comparison logic
- Remove price_or_currency_differ_from() method
- Update convert_prices() to filter on needs_price_update flag
- Extract _get_exchange_rate() and _save_converted_price() helpers
- Add tests for the new behavior

lukas reviewed 2026-05-12 12:48:50 +00:00

lukas left a comment

Solid PR. The signal-based dirty flag pattern is a common Django idiom for this kind of problem. No blocking concerns — ready to approve and merge.

Here are a few observations:

1. games/signals.py
The signal approach adds an extra DB query per update (to fetch the old instance). The old code also fetched from DB in save(), so it's a wash query-wise. Worth noting it's a trade-off, not strictly fewer queries.

2. games/migrations/0016_add_needs_price_update.py
The raw SQL UPDATE in the migration backfill could lock the table on large datasets. Consider chunked updates if the table grows significantly.

3. games/tasks.py
_get_exchange_rate() returns a bare float (not an ExchangeRate object), but the variable exchange_rate is misleading. Consider renaming to rate.

4. games/tests.py
override_settings(CACHES=...) in test_convert_prices_sets_flag_to_false seems unnecessary since convert_prices() doesn't use caching. Could be removed if left from an earlier version.

5. Log level in games/tasks.py
Exchange rate lookups use logger.info(). Consider logger.debug() for these routine operations and keep info for noteworthy events (success, failure).

Solid PR. The signal-based dirty flag pattern is a common Django idiom for this kind of problem. No blocking concerns — ready to approve and merge. Here are a few observations: **1. `games/signals.py`** The signal approach adds an extra DB query per update (to fetch the old instance). The old code also fetched from DB in `save()`, so it's a wash query-wise. Worth noting it's a trade-off, not strictly fewer queries. **2. `games/migrations/0016_add_needs_price_update.py`** The raw SQL `UPDATE` in the migration backfill could lock the table on large datasets. Consider chunked updates if the table grows significantly. **3. `games/tasks.py`** `_get_exchange_rate()` returns a bare float (not an `ExchangeRate` object), but the variable `exchange_rate` is misleading. Consider renaming to `rate`. **4. `games/tests.py`** `override_settings(CACHES=...)` in `test_convert_prices_sets_flag_to_false` seems unnecessary since `convert_prices()` doesn't use caching. Could be removed if left from an earlier version. **5. Log level in `games/tasks.py`** Exchange rate lookups use `logger.info()`. Consider `logger.debug()` for these routine operations and keep `info` for noteworthy events (success, failure).

lukas reviewed 2026-05-12 12:48:56 +00:00

lukas left a comment

Inline comments on the diff.

games/migrations/0016_add_needs_price_update.py

lukas commented

2026-05-12 12:48:56 +00:00

The raw SQL UPDATE in this migration could lock the table on large datasets. Consider chunked updates if the table grows significantly.

lukas marked this conversation as resolved

games/signals.py

lukas commented

2026-05-12 12:48:56 +00:00

This signal handler fetches the old instance from the DB on every update. The old code in save() also fetched from DB, so it's a wash query-wise — not strictly fewer queries. Just worth noting the trade-off.

This signal handler fetches the old instance from the DB on every update. The old code in `save()` also fetched from DB, so it's a wash query-wise — not strictly fewer queries. Just worth noting the trade-off.

lukas marked this conversation as resolved

games/tasks.py

lukas commented

2026-05-12 12:48:56 +00:00

_get_exchange_rate() returns a bare float (not an ExchangeRate object), but the variable name is misleading. Consider renaming to rate.

_get_exchange_rate() returns a bare float (not an ExchangeRate object), but the variable name is misleading. Consider renaming to `rate`.

lukas commented

2026-05-12 12:48:56 +00:00

Exchange rate lookups use logger.info() — consider logger.debug() for these routine operations.

lukas marked this conversation as resolved

games/tests.py

lukas commented

2026-05-12 12:48:56 +00:00

override_settings(CACHES=...) seems unnecessary since convert_prices() doesn't use caching. Could be removed if left from an earlier version.

lukas marked this conversation as resolved

lukas reviewed 2026-05-12 12:49:01 +00:00

lukas left a comment

Inline comments on the diff.

games/migrations/0016_add_needs_price_update.py

lukas commented

2026-05-12 12:48:56 +00:00

The raw SQL UPDATE in this migration could lock the table on large datasets. Consider chunked updates if the table grows significantly.

lukas marked this conversation as resolved

games/signals.py

lukas commented

2026-05-12 12:48:56 +00:00

This signal handler fetches the old instance from the DB on every update. The old code in save() also fetched from DB, so it's a wash query-wise — not strictly fewer queries. Just worth noting the trade-off.

This signal handler fetches the old instance from the DB on every update. The old code in `save()` also fetched from DB, so it's a wash query-wise — not strictly fewer queries. Just worth noting the trade-off.

lukas marked this conversation as resolved

games/tasks.py

lukas commented

2026-05-12 12:48:56 +00:00

_get_exchange_rate() returns a bare float (not an ExchangeRate object), but the variable name is misleading. Consider renaming to rate.

_get_exchange_rate() returns a bare float (not an ExchangeRate object), but the variable name is misleading. Consider renaming to `rate`.

lukas commented

2026-05-12 12:48:56 +00:00

Exchange rate lookups use logger.info() — consider logger.debug() for these routine operations.

lukas marked this conversation as resolved

games/tests.py

lukas commented

2026-05-12 12:48:56 +00:00

override_settings(CACHES=...) seems unnecessary since convert_prices() doesn't use caching. Could be removed if left from an earlier version.

lukas marked this conversation as resolved

lukas commented

2026-05-12 12:56:38 +00:00

Done. Three changes applied:
tasks.py:

Fix 3 — Renamed exchange_rate → rate throughout _get_exchange_rate() and the call site
Fix 5 — Changed DB lookup and API fetch attempt logs to logger.debug(). Kept logger.info() for "Got X, saving..." (successful fetch) and the exception handler (failure), since those are noteworthy events.
tests.py:
Fix 4 — Removed override_settings import and the with override_settings(CACHES=...) wrapper around convert_prices()

Done. Three changes applied: tasks.py: - Fix 3 — Renamed exchange_rate → rate throughout _get_exchange_rate() and the call site - Fix 5 — Changed DB lookup and API fetch attempt logs to logger.debug(). Kept logger.info() for "Got X, saving..." (successful fetch) and the exception handler (failure), since those are noteworthy events. tests.py: - Fix 4 — Removed override_settings import and the with override_settings(CACHES=...) wrapper around convert_prices()

lukas added 2 commits 2026-05-12 12:57:01 +00:00

Add info on statuses 4ba3ed555f

PR review

Django CI/CD / test (push) Successful in 28s

Details

Django CI/CD / build-and-push (push) Successful in 55s

Details

5003b739d3

lukas merged commit 5003b739d3 into main

2026-05-12 13:03:34 +00:00

lukas deleted branch purchase-needs-price-update

2026-05-12 13:03:34 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lukas/timetracker#94