[COST-7252] Implement constant currency Phase 1#6005
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the Constant Currency feature, introducing static exchange rate management and dynamic rate locking to ensure historical reporting accuracy. Key architectural changes include new tenant-scoped models, a CRUD API for rate pairs, and a transition to subquery-based rate resolution in the reporting pipeline. The review feedback primarily identifies opportunities to adhere to DRY principles by refactoring duplicated exchange rate annotation logic across query and forecast handlers. Additionally, it is recommended to enhance error reporting by specifying missing currency pairs to improve system debuggability.
Add static exchange rate CRUD, per-month rate storage via MonthlyExchangeRate, currency enablement settings, and Subquery-based exchange rate resolution in query handlers and forecasts. New models (tenant-scoped in cost_models app): - StaticExchangeRate: user-defined rates with monthly validity periods - MonthlyExchangeRate: single source of truth for all months - EnabledCurrency: controls dropdown visibility per tenant New endpoints: - GET/POST/PUT/DELETE /exchange-rate-pairs/ (static rate CRUD) - GET/PUT /settings/currency/enabled-currencies/ - GET /settings/currency/available-currencies/ Pipeline changes: - get_daily_currency_rates: currency discovery, per-tenant MonthlyExchangeRate upsert, CURRENCY_URL skip when empty - QueryHandler/ReportQueryHandler: Subquery annotation from MonthlyExchangeRate with earliest-rate fallback - OCP query handler: dual Subquery (cost model + infra currency) - Forecast handlers: same Subquery pattern - Report responses: exchange_rates_applied metadata - Cache invalidation on both write paths Made-with: Cursor
7aab645 to
48f49de
Compare
- Renumber migrations 0012-0014 to 0013-0015 to resolve conflict with 0012_add_rate_model from main - Remove unused ValidationError import in test_static_exchange_rate_serializer - Extract exchange rate annotation logic into cost_models/exchange_rate_annotations.py to eliminate duplication across QueryHandler, ReportQueryHandler, Forecast, OCPReportQueryHandler, and OCPForecast (DRY) - Include missing base currencies in _validate_exchange_rates error message for easier debugging Made-with: Cursor
Remove unused imports flagged by flake8 F401 across serializer, view, and test files. Apply black formatting fixes for line wrapping. Made-with: Cursor
- Replace TruncMonth(OuterRef()) with ExtractYear/ExtractMonth to avoid Django's ResolvedOuterRef.output_field AttributeError - Use **self.headers in view tests instead of force_authenticate (Koku's IdentityHeaderMiddleware requires x-rh-identity) - Restore log_json import removed in prior cleanup (F821) - Refactor get_daily_currency_rates to reduce cyclomatic complexity (C901) Made-with: Cursor
Made-with: Cursor
…lookup Django auto-generates the reverse accessor without underscores for CostModelMap -> CostModel FK. All existing code uses costmodelmap. Made-with: Cursor
Use OuterRef(OuterRef("source_uuid")) to correctly reference the
outermost queryset when the CostModel subquery is nested inside
the MonthlyExchangeRate subquery.
Made-with: Cursor
Django 5.2 resolves OuterRef to ResolvedOuterRef without output_field, which breaks datetime Trunc* transforms. Pass DateField on OuterRef so TruncMonth can resolve the lhs field type. Made-with: Cursor
Made-with: Cursor
OuterRef inherits F; output_field is not a valid kwarg in our Django version, causing TypeError across report and forecast tests. Made-with: Cursor
Replace TruncMonth(OuterRef("usage_start")) with ExtractYear/ExtractMonth
field lookups. TruncBase.resolve_expression accesses copy.lhs.output_field
directly, which crashes on ResolvedOuterRef. Extract.resolve_expression uses
getattr with a None fallback, avoiding the AttributeError.
Also fix CostModel reverse FK lookup: cost_model_map -> costmodelmap
(Django auto-generates the accessor without underscores).
These two bugs caused all 688 unit test failures in CI.
Made-with: Cursor
_get_exchange_rates_applied queries MonthlyExchangeRate (a tenant-scoped model in cost_models app) but runs outside the tenant_context block in execute_query. Wrap the query with tenant_context(self.tenant) so PostgreSQL finds the table in the correct tenant schema. Made-with: Cursor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6005 +/- ##
=======================================
- Coverage 94.4% 94.3% -0.0%
=======================================
Files 362 367 +5
Lines 32101 32448 +347
Branches 3538 3585 +47
=======================================
+ Hits 30290 30613 +323
- Misses 1176 1186 +10
- Partials 635 649 +14 🚀 New features to boost your workflow:
|
- Remove unused VALID_RATE_TYPES constant (superseded by RateType enum) - Remove uncalled _validate_exchange_rates method and its ValidationError import - Remove unused logging imports and LOG variables from price_list_serializer, price_list_view, and static_exchange_rate_view Made-with: Cursor
This helper is only used within exchange_rate_annotations.py, so prefix it with an underscore to signal internal-only usage. Made-with: Cursor
Accept a pre-built Django expression for base_currency instead of a field name string, eliminating duplicated subquery logic in build_ocp_exchange_rate_annotation_dict. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Static rate CRUD now enables currencies in EnabledCurrency on create/update, so AvailableCurrencyView only needs to query EnabledCurrency instead of unioning with StaticExchangeRate. Made-with: Cursor
DRF permission classes check request.user.is_authenticated, which defaults to False on non-Django-auth models. Returning True is safe because User instances are only created after middleware authentication. Co-authored-by: Cursor <cursoragent@cursor.com>
Validate that exchange rate values are non-null and positive before storing them, preventing bad data from corrupting the rates table. Co-authored-by: Cursor <cursoragent@cursor.com>
myersCody
left a comment
There was a problem hiding this comment.
You should just combine the migrations.
|
|
||
| _ISO_4217_CURRENCIES = get_global("all_currencies") |
There was a problem hiding this comment.
It appears you are allowing the customer to select any currency that is in the get_global("all_currencies") library. However, for our dynamic rates logic pulls pulls the exchange rates from: https://open.er-api.com/v6/latest/USD
@ELK4N4 @bacciotti Has anyone done any checking to ensure the list provided by get_global does not include currencies that can not be found in URL we use to grab dynamic exchange rates?
There was a problem hiding this comment.
A user can defines a currency that doesn't exist in https://open.er-api.com/v6/latest/USD using static rates
There was a problem hiding this comment.
The current process from what I can see allows user to enable a currency that can not be found in the CURRENCY_URL. Then if they try to utilize it they git a:
{"currency": ["No exchange rate available for <CODE>. Ask your administrator to configure static exchange rates or enable dynamic exchange rates."]}
The problem is that they did enable the dynamic rate, it just can't be supported.
I think we may need some type of validation for the dynamic flow that ensure they currency code is found in whatever url they decide to use? Thoughts?
That way they can "enable a dynamic rate" without being able to get a rate if that makes sense.
There was a problem hiding this comment.
I can change the error message to
No exchange rate available for <CODE>. Ask your administrator to configure static exchange rates
if CURRENCY_URL is defined.
In general, I think the enabled currencies capability should be decoupled from the actual available rates as long the list is ISO 4217
There was a problem hiding this comment.
I have checked the differences between Babel and the API.
Babel support all ISO 4217 currencies(~300) and the API support only 162.
The exchange rate API (https://open.er-api.com/v6/latest/USD) returns 6 currency
codes that are not in babel's ISO 4217 registry. These are silently skipped
during the exchange rate fetch in masu/celery/tasks.py.
| Code | Currency | Territory | Official Currency | Pegged To |
|---|---|---|---|---|
| FOK | Faroese Króna | Faroe Islands | DKK (Danish Krone) | 1:1 DKK |
| GGP | Guernsey Pound | Guernsey | GBP (British Pound) | 1:1 GBP |
| IMP | Isle of Man Pound | Isle of Man | GBP (British Pound) | 1:1 GBP |
| JEP | Jersey Pound | Jersey | GBP (British Pound) | 1:1 GBP |
| KID | Kiribati Dollar | Kiribati | AUD (Australian Dollar) | 1:1 AUD |
| TVD | Tuvaluan Dollar | Tuvalu | AUD (Australian Dollar) | 1:1 AUD |
There was a problem hiding this comment.
I think the design is correct. decoupling "which currencies are enabled" from "which currencies have a dynamic rate" is the right call. however id say we have two things that worth addressing:
-
fix the error message: if saas customer enables a currency thats not in the er-api and tries to use it, they get told to "enable dynamic exchange rates" -> but they already did
-
would be good to, at least, warn the admin at "enable-time". Foe example: if
CURRENCY_URLis configured and the currency being enabled isnt inExchangeRateDictionary, return a warnings field in the response saying, maybe, "this currency has no dynamic rate available, you'll need to configure a static rate before users can use it."
There was a problem hiding this comment.
From the SaaS perspective they don't get to control the CURRENCY url. Therefore, there is no way for them to "fix" it to where they could provide a dynamic rate to the currencies not in the URL. It is probably worth a discussion a team level on how to handle it.
There was a problem hiding this comment.
after the team discussion, i think we landed on two things that need to be addressed before merge:
-
error message: if a currency has no rate available, the current message suggests enabling dynamic exchange rates, but that won't help if the currency simply isn't in the er-api. the message needs to be clear and actionable, something like "this currency requires a static exchange rate to be configured by your administrator." -
static-only signal at enable-time: if the currency being enabled isn't in the er-api, the admin should know upfront that it's static only. without a static rate configured, users will hit an error in reports.
the dynamic fallback only makes sense for currencies that exist in the er-api. for everything else, static is the only path, and both the admin and the end user need to know that clearly.
| code = code.upper() | ||
| try: | ||
| name = get_currency_name(code, locale="en_US") | ||
| symbol = get_currency_symbol(code, locale="en_US") |
There was a problem hiding this comment.
The symbol and description are directly utilize by our UI. The previous approach utilize Unicode escape sequence for the more complex currency symbols. The purpose around this is because the UX supports multiple locals.
For example:
{
"code": "CNY",
"name": "Chinese Yuan",
"symbol": "CN\u00a5",
"description": "CNY (CN\u00a5) - Chinese Yuan",
},
Would return CN¥ for en_US local; however, if your local is Chinese it just returns ¥.
This approach forces all customers into the en_US local for currency. A minor change, but also different functionality from our previous approach. Let me ping the team to see if there is any impact to this change that I don't recognize real quick.
There was a problem hiding this comment.
Today the UI converts from code to symbol? If that the case I can just returns the code and the UI will do the rest
Only suggest enabling dynamic exchange rates when CURRENCY_URL is configured. On-prem deployments without a rate API get a simpler message pointing to static exchange rates only. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Return a warning in the POST response when CURRENCY_URL is configured but the requested currency has no dynamic rate available, prompting the admin to configure a static rate. Also default CURRENCY_URL to None in on-prem mode since external rate APIs are typically unavailable. Co-authored-by: Cursor <cursoragent@cursor.com>
b83a806 to
2bb4ecf
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Swap the error messages so that when CURRENCY_URL is configured, we only suggest static rates (dynamic is already available), and when it's not configured, we suggest both options. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 CI Triager — Warning Check: Migration convention Migrations found: Action: Since these migrations are in different apps ( Generated automatically. Review before applying. |
Check MonthlyExchangeRate (the actual source of truth for reports) instead of ExchangeRateDictionary, and always warn regardless of whether CURRENCY_URL is configured. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 CI Triager — Warning Check: Migration convention Action: Please squash the migrations within each app into one. Run: If the migrations are in separate apps and each app has only one new migration, no squash is needed — but confirm that each app has exactly one new migration file, not multiple. In this case, two separate apps each have one new migration, which may be acceptable if they are both intentionally part of this PR. Please confirm with the team that both migration files are required in the same PR. Generated automatically. Review before applying. |
|
🤖 CI Triager — Diagnosis Check: Units - 3.11 Action: In # Line ~41 in initialize_request():
# Before:
"authentication": {"credentials": {"cluster_id": self.fake.word()}},
# After:
"authentication": {"credentials": {"cluster_id": str(self.fake.uuid4())}},This file is not in the PR diff, but the fix is a one-line change. Alternatively, re-run CI with a clean test database ( Generated automatically. Review before applying. |
|
🤖 CI Triager — Warning Check: Migration convention Action: These migrations are in different apps ( If each app has exactly one new migration file, no action is needed — this warning is informational. Generated automatically. Review before applying. |
Summary
Implements Phase 1 of the constant currency feature, enabling per-month exchange rate resolution in report queries and forecasts instead of using a single daily rate.
cost_modelsapp, tenant-scoped):StaticExchangeRate(user-defined rates with monthly validity),MonthlyExchangeRate(single source of truth per month),EnabledCurrency(UI dropdown control)exchange-rate-pairs/(static rate CRUD),settings/currency/enabled-currencies/(list/update enablement),settings/currency/available-currencies/(dropdown source)get_daily_currency_ratesCelery task now discovers currencies, createsEnabledCurrencyrecords, and upserts dynamic rates intoMonthlyExchangeRate(respecting static overrides)Subqueryannotation fromMonthlyExchangeRatefor per-month rate resolution with earliest-rate fallbackexchange_rates_appliedinmetafor transparencyTest plan
StaticExchangeRateserializer (validation, versioning,MonthlyExchangeRateside effects)StaticExchangeRateViewSet(CRUD operations, filtering, error cases)EnabledCurrencyViewandAvailableCurrencyViewMonthlyExchangeRateandEnabledCurrencymodel constraintsMonthlyExchangeRateMonthlyExchangeRatefromExchangeRateDictionaryMade with Cursor