Add Currency fields and set currency_fk, country fields #846
Conversation
WalkthroughAdds new models Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
commcare_connect/program/utils.py (1)
47-47: Drop unused# noqa: E203to satisfy RuffRuff flagged this as an unused
noqa(RUF100). Sinceqs[start : start + BATCH_SIZE]is fine as‑is, you can remove the# noqa: E203comment.commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py (1)
411-411: Use a plain ASCII apostrophe in"Pa’anga"to avoid encoding/lint issuesRuff flagged the RIGHT SINGLE QUOTATION MARK here (
RUF001). You can use a normal ASCII apostrophe instead:- ("Pa’anga", "TOP"), + ("Pa'anga", "TOP"),commcare_connect/program/models.py (1)
4-17: Program FKs align with Opportunity, but there are now two currency fieldsThe new
currency_fkandcountryFKs are defined consistently with Opportunity and with the migrations, which is good. Note that Program now has bothcurrency(CharField) andcurrency_fk(FK), so going forward it’d be good to treat one as the canonical source of truth (or add a sync path) to avoid the two drifting apart over time.commcare_connect/opportunity/models.py (2)
62-66: Add__str__method for better representation.The Currency model would benefit from a
__str__method to improve its representation in the Django admin interface and debugging output.Apply this diff to add a
__str__method:class Currency(models.Model): code = models.CharField(max_length=3, primary_key=True) # ISO 4217 name = models.CharField(max_length=64) is_valid = models.BooleanField(default=True) + + def __str__(self): + return f"{self.code} - {self.name}"
68-72: Add__str__method for better representation.The Country model would benefit from a
__str__method to improve its representation in the Django admin interface and debugging output.Apply this diff to add a
__str__method:class Country(models.Model): code = models.CharField(max_length=3, primary_key=True) # ISO 3166-1 alpha-3 name = models.CharField(max_length=128) currency = models.ForeignKey(Currency, on_delete=models.SET_NULL, null=True, blank=True) + + def __str__(self): + return self.name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py(1 hunks)commcare_connect/opportunity/models.py(2 hunks)commcare_connect/opportunity/tests/test_models.py(3 hunks)commcare_connect/program/migrations/0003_program_currency_fk.py(1 hunks)commcare_connect/program/models.py(2 hunks)commcare_connect/program/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
commcare_connect/program/models.py (1)
commcare_connect/opportunity/models.py (4)
Country(68-71)Currency(62-65)DeliveryType(53-59)Opportunity(74-227)
commcare_connect/program/utils.py (1)
commcare_connect/opportunity/models.py (2)
Currency(62-65)Country(68-71)
commcare_connect/program/migrations/0003_program_currency_fk.py (2)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py (1)
Migration(472-525)
commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py (3)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(62-65)Country(68-71)commcare_connect/program/migrations/0003_program_currency_fk.py (1)
Migration(13-39)
commcare_connect/opportunity/tests/test_models.py (2)
commcare_connect/opportunity/tests/factories.py (1)
OpportunityFactory(47-64)commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py (1)
populate_currency_and_country_fk(468-469)
🪛 Ruff (0.14.4)
commcare_connect/program/utils.py
47-47: Unused noqa directive (non-enabled: E203)
Remove unused noqa directive
(RUF100)
commcare_connect/program/migrations/0003_program_currency_fk.py
9-9: Unused function argument: schema_editor
(ARG001)
14-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
19-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py
411-411: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (4)
commcare_connect/program/utils.py (1)
26-68: Migration FK backfill utility looks solid and idempotentThe overall approach here (preloading
Currency/Country, normalizing codes, creating invalid placeholders viaget_or_create, and backfilling viabulk_updatein batches) looks correct and should be safely idempotent, including for invalid/unknown codes and multi‑country currencies.commcare_connect/opportunity/migrations/0084_currency_country_opportunity_currency_fk.py (1)
433-524: Currency/country seeding and FK wiring are consistent and idempotentThe migration correctly seeds
CurrencyandCountryviabulk_create, builds a code→Currency map, and uses the shared FK backfill utility forOpportunity. Withignore_conflicts=Trueon currencies and the delegated backfill, this should be safe to run once and re‑run (in tests) without duplicate rows or broken mappings.commcare_connect/program/migrations/0003_program_currency_fk.py (1)
9-39: Program FK migration correctly delegates to shared backfill logicDependencies, FK definitions, and the RunPython step all look correct, and delegating to
populate_currency_and_country_fk_for_modelensures Program gets the same currency/country population behavior as Opportunity without duplicating logic.commcare_connect/opportunity/tests/test_models.py (1)
1-184: New migration test gives good end‑to‑end coverage of FK backfill behaviorThe
test_populate_currency_and_country_fksetup and assertions look solid: it covers valid, single‑country, multi‑country, and invalid currency cases, verifiesis_valid=Falsefor placeholders, and checks that only currencies (not countries) are created for invalid codes. Calling the migration function withdjango_appsis appropriate given how the shared util usesapps.get_model.
calellowitz
left a comment
calellowitz
left a comment
There was a problem hiding this comment.
One real suggestion, one nit
|
|
||
| def populate_currency_and_country_fk_for_model(apps, model_name, app_label, total_label): | ||
| """ | ||
| Migration util to populate currency_fk and country fields for a opportunity/program |
There was a problem hiding this comment.
nit: If this is just for use by the migration, I think it can go inline in the migration file
There was a problem hiding this comment.
It's used in two migration files, so I had to pull this into a separate file.
There was a problem hiding this comment.
why does it need to be run twice?
There was a problem hiding this comment.
There are two models Opportunity and Program that have currency field. So this is used in those two migration files as a common helper.
| Country( | ||
| code=code, | ||
| name=name, | ||
| currency=code_to_currency.get(currency_code), |
There was a problem hiding this comment.
Are there missing currencies? Why are we expecting code_to_currency to not contain currency_code?
There was a problem hiding this comment.
Yes, there are invalid currencies. Please see the relevant note the description above
I have run a script to analyze current prod opportunity/currency data to check invalid currencies etc. I have compiled the detailed data here in the doc.
There was a problem hiding this comment.
I think we should fix the ones with invalid currencies, since that breaks a bunch of other behavior, rather than leaving them in that state (and enforce it going forwards). Most look like test ones, or like the intended currency is clear, but if there are real ones with open questions we can reach out to the program teams, because it is unlikely those opportunities will be working correctly.
There was a problem hiding this comment.
and enforce it going forwards
Yes, with this PR, currency validation is enforced by displaying only valid currencies to the user.
Regarding fixing the invalid currency opportunities, all the 14 invalid currency opportunities are test opportunities, only 3 of them are active (have end-date in future).
- BTC: https://connect.dimagi.com/a/conroys-organization/opportunity/808/
- US: https://connect.dimagi.com/a/dimagi-gw-chc-2024-2025/opportunity/817/
- US: https://connect.dimagi.com/a/abdoul-sandbox/opportunity/825/
So, these can safely be left as they are, and this PR provides the option to update these currencies after the release as well if needed.
This design actually allows releasing this PR without the need for co-ordinating with the opportunity owners, as it changes the underlying fields without any new constraints. Otherwise, at the time of deploy, I will have to re-run the scripts, go through any new invalid currencies and co-ordinate with opportunity owners to fix them.
This point was indeed discussed and planned as such during the tech spec discussion as well. So, this is following the same plan.
| class Country(models.Model): | ||
| code = models.CharField(max_length=3, primary_key=True) # ISO 3166-1 alpha-3 | ||
| name = models.CharField(max_length=128) | ||
| currency = models.ForeignKey(Currency, on_delete=models.SET_NULL, null=True, blank=True) |
There was a problem hiding this comment.
Ties in with my question in the relating migration file - why is this a nullable field?
There was a problem hiding this comment.
There are some countries with no official currency, please see the COUNTRIES data in the migration file.
|
@calellowitz Thanks for the review, PR is updated with your inputs. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (3)
266-430: Tidy up CURRENCIES data (duplicate code and ambiguous apostrophe).Two minor data/maintainability nits in
CURRENCIES:
("Moroccan Dirham", "MAD")appears twice (Lines 372 and 426). It’s harmless becausecodeis the PK andignore_conflicts=Trueis used, but the duplicate entry is confusing and can be removed."Pa’anga"(Line 411) uses a RIGHT SINGLE QUOTATION MARK, which Ruff flags as ambiguous (RUF001) and can cause copy/paste surprises. Consider using a plain ASCII apostrophe instead.Neither affects runtime behavior, but fixing them will keep the dataset and linters clean.
433-468: Renameschema_editorargs to avoid Ruff ARG001 without changing behavior.Both
load_currency_and_country_dataandpopulate_currency_and_country_fkacceptschema_editorbut don’t use it, which Ruff flags (ARG001). Django only cares about argument order, not names, so you can safely rename them to_schema_editorto satisfy the linter:-def load_currency_and_country_data(apps, schema_editor): +def load_currency_and_country_data(apps, _schema_editor): - def populate_currency_and_country_fk(apps, schema_editor): +def populate_currency_and_country_fk(apps, _schema_editor):This keeps the migration API intact while quieting the warnings.
468-524: Consider freezing FK‑population logic inside the migration instead of importing fromprogram.utils.
populate_currency_and_country_fkcallspopulate_currency_and_country_fk_for_modelfromcommcare_connect.program.utils. That utility will evolve over time, but migrations are meant to be historical snapshots. On a fresh install in a year, this migration will run with whatever the current implementation ofpopulate_currency_and_country_fk_for_modelis, which can subtly change or break the behavior you intended when the migration was authored.Consider copying the current implementation (or a slimmed‑down version of it) into this migration file so that the data migration logic is frozen in time and independent of future app‑level refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py(1 hunks)commcare_connect/opportunity/models.py(2 hunks)commcare_connect/program/models.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- commcare_connect/opportunity/models.py
- commcare_connect/program/models.py
🧰 Additional context used
🧬 Code graph analysis (1)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (2)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(61-64)Country(67-70)
🪛 Ruff (0.14.5)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py
411-411: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
sravfeyn
force-pushed
the
sr/currency_fk
branch
from
21bb702 to
bcc68ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
commcare_connect/opportunity/tests/test_models.py (1)
165-184: Test assertions validate the core migration behavior correctly.The test properly verifies:
- Valid currencies populate
currency_fk- Single-country currencies auto-set both
currency_fkandcountry- Multi-country currencies leave
countryas NULL- Invalid currencies create placeholder Currency records with
is_valid=FalseOptional: Consider adding a test case for a valid currency that exists but isn't used by any country to ensure complete edge case coverage.
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (2)
6-6: Note the cross-app dependency in migration.Importing
populate_currency_and_country_fk_for_modelfromprogram.utilscreates a cross-app dependency. While this promotes code reuse, it means this migration will fail if the program app structure changes. Consider documenting this dependency or inlining the logic if isolation is preferred.For better migration isolation, the utility could be defined inline within this migration file, though that would create duplication with the program migration.
478-498: Note theon_deletepolicy differences.The Country model uses
on_delete=SET_NULLfor its currency FK (line 494), which allows graceful handling of currency deletion. However, the Opportunity FKs added later (lines 502-504, 509-511) useDO_NOTHING, creating an inconsistency. Consider whether Opportunity should usePROTECTto prevent accidental deletion of reference data.Based on the models being reference data that shouldn't be deleted,
PROTECTwould be more explicit:migrations.AddField( model_name="opportunity", name="currency_fk", field=models.ForeignKey( - null=True, on_delete=django.db.models.deletion.DO_NOTHING, to="opportunity.currency" + null=True, on_delete=django.db.models.deletion.PROTECT, to="opportunity.currency" ), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py(1 hunks)commcare_connect/opportunity/tests/test_models.py(3 hunks)commcare_connect/program/migrations/0003_program_currency_fk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
commcare_connect/opportunity/tests/test_models.py (2)
commcare_connect/opportunity/tests/factories.py (1)
OpportunityFactory(47-64)commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (1)
populate_currency_and_country_fk(468-469)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (3)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(61-64)Country(67-70)commcare_connect/program/migrations/0003_program_currency_fk.py (1)
Migration(13-39)
commcare_connect/program/migrations/0003_program_currency_fk.py (2)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (1)
Migration(472-525)
🪛 Ruff (0.14.5)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py
411-411: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/program/migrations/0003_program_currency_fk.py
9-9: Unused function argument: schema_editor
(ARG001)
14-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
19-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (8)
commcare_connect/opportunity/tests/test_models.py (2)
1-4: LGTM - Imports support migration testing.The
importlibanddjango_appsimports are correctly used to dynamically load the migration module and retrieve model classes for testing the FK population logic.
15-15: LGTM - Factory import for test data creation.The
OpportunityFactoryimport is necessary for creating test opportunities with specific currency values.commcare_connect/program/migrations/0003_program_currency_fk.py (3)
9-10: LGTM - Standard Django migration pattern.The unused
schema_editorparameter is required by Django'sRunPythonsignature. The static analysis warning is a false positive.
20-33: Verify theon_delete=DO_NOTHINGpolicy for reference data.Both
currency_fkandcountryuseon_delete=DO_NOTHING, which means orphaned references could occur if Currency or Country records are deleted. While reference data typically shouldn't be deleted, usingPROTECTwould make this constraint explicit and prevent accidental deletions.Consider whether
PROTECTwould be more appropriate:field=models.ForeignKey( null=True, on_delete=django.db.models.deletion.PROTECT, to="opportunity.currency" ),This matches the pattern used in the Opportunity model Currency/Country fields shown in the relevant code snippets. Based on learnings from the PR objectives, these are reference data models that should remain stable.
34-38: LGTM - RunPython configuration is appropriate.The
run_on_secondary: Falsehint prevents unnecessary execution on read replicas, and thenoopreverse is acceptable since field removal will drop the data regardless.commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (3)
9-263: LGTM - Comprehensive country data from ISO sources.The COUNTRIES data is well-structured with ISO 3166-1 alpha-3 codes and appropriate handling of territories without official currencies (None values).
468-470: LGTM - Standard delegation to utility function.The unused
schema_editorparameter is required by Django'sRunPythonsignature. The static analysis warning is a false positive.
513-524: LGTM - RunPython operations ordered correctly.The migration properly sequences operations: first loading reference data, then populating FK relationships. The
run_on_secondary: Falsehints are appropriate for data migrations.
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py
Outdated
Show resolved
Hide resolved
commcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py
Show resolved
Hide resolved
| # Ensure currency / country rows exist (loaded via migration) | ||
| single_country_currency = "KES" # Kenya only | ||
| multi_country_currency = "USD" # Multiple ISO countries share USD | ||
| valid_currency_only = "INR" | ||
|
|
||
| assert CurrencyModel.objects.filter(code=valid_currency_only).exists() | ||
| assert CountryModel.objects.filter(code="KEN", currency_id=single_country_currency).exists() | ||
| assert CurrencyModel.objects.filter(code=multi_country_currency).exists() | ||
|
|
There was a problem hiding this comment.
Misleading variable name: INR is a single-country currency.
The variable valid_currency_only suggests INR only validates the currency FK without setting country, but based on the COUNTRIES data in the migration, INR maps to India ("IND", "India", "INR"), making it a single-country currency like KES. Consider renaming to avoid confusion.
Apply this diff:
- valid_currency_only = "INR"
+ valid_single_country = "INR" # India only
assert CurrencyModel.objects.filter(code=valid_currency_only).exists()
assert CountryModel.objects.filter(code="KEN", currency_id=single_country_currency).exists()And update the assertion at line 169:
# valid currency should map to fk
- assert opp_valid.currency_fk_id == valid_currency_only
+ assert opp_valid.currency_fk_id == valid_single_country
+ # INR is single-country, should also set country
+ assert opp_valid.country_id == CountryModel.objects.get(code="IND").pk📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Ensure currency / country rows exist (loaded via migration) | |
| single_country_currency = "KES" # Kenya only | |
| multi_country_currency = "USD" # Multiple ISO countries share USD | |
| valid_currency_only = "INR" | |
| assert CurrencyModel.objects.filter(code=valid_currency_only).exists() | |
| assert CountryModel.objects.filter(code="KEN", currency_id=single_country_currency).exists() | |
| assert CurrencyModel.objects.filter(code=multi_country_currency).exists() | |
| # Ensure currency / country rows exist (loaded via migration) | |
| single_country_currency = "KES" # Kenya only | |
| multi_country_currency = "USD" # Multiple ISO countries share USD | |
| valid_single_country = "INR" # India only | |
| assert CurrencyModel.objects.filter(code=valid_single_country).exists() | |
| assert CountryModel.objects.filter(code="KEN", currency_id=single_country_currency).exists() | |
| assert CurrencyModel.objects.filter(code=multi_country_currency).exists() |
🤖 Prompt for AI Agents
In commcare_connect/opportunity/tests/test_models.py around lines 140 to 148,
the variable valid_currency_only is misleading because INR is actually a
single-country currency per the migrations; rename it to something like
single_country_currency_ind (or valid_single_country_currency) and set it to
"INR", and then update the assertion at line 169 to assert that
CountryModel.objects.filter(code="IND", currency_id="INR").exists() so the test
checks India is associated with INR instead of implying INR is multi-country.
There was a problem hiding this comment.
Minor, unnecessary, resolving it.
There was a problem hiding this comment.
@sravfeyn, understood! I'll close this thread. Thanks for the feedback.
|
@calellowitz Bumping this for re-review/approval |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (4)
266-430: Static currency dataset looks good; consider removing duplicated MAD entry.
CURRENCIESincludes "Moroccan Dirham" /MADtwice (around Lines 372 and 426). This is harmless because ofignore_conflicts=Trueonbulk_create, but dropping the later duplicate would keep the dataset minimal and avoid a redundant insert attempt.You can remove the extra entry like:
- ("Moroccan Dirham", "MAD"),
433-470: RunPython helpers are correct; optionally silence Ruff ARG001 more cleanly.Both migration functions correctly follow Django’s
(apps, schema_editor)signature, and the data-loading / FK-population logic looks sound. Sinceschema_editorisn’t used, you can optionally silence Ruff’s ARG001 by renaming the parameter instead of adding ignores:-def load_currency_and_country_data(apps, schema_editor): +def load_currency_and_country_data(apps, _schema_editor): -def populate_currency_and_country_fk(apps, schema_editor): +def populate_currency_and_country_fk(apps, _schema_editor):Django passes these arguments positionally, so the name change is safe.
6-7: Consider avoiding migration-time dependency oncommcare_connect.program.utils.Importing and calling
populate_currency_and_country_fk_for_modelfromcommcare_connect.program.utilsis convenient but couples this migration to non-migration code that might change or be removed later, breaking new environments running migrations from scratch.If you expect that helper to evolve, consider inlining a stable copy of the logic here (or in a dedicated
migrations_utilsmodule that is treated as versioned migration code) so this migration remains self-contained over time.Also applies to: 468-470
472-525: Migration operations and model definitions align correctly; Ruff RUF012 is safe to ignore or filter.The ordering—
CreateModelforCurrencyandCountry, thenAddFieldonOpportunity, then the twoRunPythonsteps—matches the models and ensures data exists before FK population, which looks correct.Ruff’s RUF012 warnings about mutable class attributes (
dependencies,operations) are a common false positive on Django migrations; I’d generally prefer configuring Ruff to ignore this rule formigrations/rather than modifying standard Django migration patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py (3)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(61-64)Country(67-70)commcare_connect/program/migrations/0003_program_currency_fk.py (1)
Migration(13-39)
🪛 Ruff (0.14.6)
commcare_connect/opportunity/migrations/0087_currency_country_opportunity_currency_fk.py
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
calellowitz
left a comment
calellowitz
left a comment
There was a problem hiding this comment.
offlined with @sravfeyn, fine to go forward as is
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/opportunity/models.py (1)
61-65: LGTM! Currency model structure is correct.The model correctly uses ISO 4217 3-letter codes as the primary key and includes an
is_validflag to handle invalid currency codes as described in the PR objectives (14 invalid currencies marked withis_valid=False).Optionally, consider adding a
__str__method for better representation in Django admin and debugging:class Currency(models.Model): code = models.CharField(max_length=3, primary_key=True) # ISO 4217 name = models.CharField(max_length=64) is_valid = models.BooleanField(default=True) + + def __str__(self): + return f"{self.code} - {self.name}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py(1 hunks)commcare_connect/opportunity/models.py(2 hunks)commcare_connect/program/migrations/0004_program_currency_fk.py(1 hunks)commcare_connect/program/models.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
commcare_connect/program/models.py (1)
commcare_connect/opportunity/models.py (4)
Country(67-70)Currency(61-64)DeliveryType(52-58)Opportunity(73-226)
commcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py (3)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(61-64)Country(67-70)commcare_connect/program/migrations/0004_program_currency_fk.py (1)
Migration(13-39)
🪛 Ruff (0.14.8)
commcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/program/migrations/0004_program_currency_fk.py
9-9: Unused function argument: schema_editor
(ARG001)
14-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
19-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (11)
commcare_connect/opportunity/models.py (2)
67-71: LGTM! Country model structure is correct.The model correctly uses ISO 3166-1 alpha-3 codes as the primary key. The nullable currency field with
on_delete=SET_NULLis appropriate, as some countries have no official currency (e.g., Antarctica, State of Palestine per the COUNTRIES data in the migration).
100-101: LGTM! Foreign key additions are correct.The new
currency_fkandcountryforeign keys appropriately useon_delete=PROTECTto maintain referential integrity by preventing deletion of Currency or Country records that are in use. The nullable fields allow for gradual migration of existing data.commcare_connect/program/models.py (2)
4-4: LGTM! Import additions are correct.The imports of
CountryandCurrencyare necessary for the new foreign key fields added to the Program model.
16-17: LGTM! Program model changes mirror Opportunity model correctly.The new
currency_fkandcountryforeign keys are consistent with the Opportunity model changes, usingon_delete=PROTECTfor referential integrity andnull=Truefor gradual data migration.commcare_connect/program/migrations/0004_program_currency_fk.py (3)
9-10: LGTM! Data migration function correctly delegates to shared utility.The function appropriately delegates to the shared
populate_currency_and_country_fk_for_modelutility with correct parameters for the Program model.
14-17: LGTM! Migration dependencies are correct.The dependencies correctly ensure that:
- Currency and Country models exist (from opportunity 0088)
- Previous Program migration has run (program 0003)
19-39: LGTM! Migration operations are correctly structured.The operations correctly:
- Add the
currency_fkandcountryforeign key fields with appropriate constraints- Populate the new fields using the shared utility function
- Use
reverse_code=noopandrun_on_secondary=Falsehints appropriately for data migrationscommcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py (4)
12-430: LGTM! ISO data structures are correctly formatted.The COUNTRIES and CURRENCIES data structures are properly formatted as tuples sourced from ISO standards. Countries without official currencies correctly have
Nonefor the currency field (e.g., Antarctica, South Georgia, State of Palestine).
433-465: LGTM! Data loading function correctly creates Currency and Country records.The function properly:
- Creates Currency records with
ignore_conflicts=Truefor idempotency- Builds a lookup dictionary to link countries to currencies
- Creates Country records with appropriate currency references
- Uses
ignore_conflicts=Truefor Country creation (as addressed in past reviews)
468-470: LGTM! Data migration function correctly delegates to shared utility.The function appropriately delegates to the shared
populate_currency_and_country_fk_for_modelutility with correct parameters for the Opportunity model, consistent with the Program migration.
472-525: LGTM! Migration operations are correctly ordered and structured.The migration operations follow the correct sequence:
- Create Currency and Country models
- Add foreign key fields to Opportunity
- Load reference data (Currency and Country records)
- Populate foreign keys on existing Opportunity records
The use of
reverse_code=noopandrun_on_secondary=Falsehints is appropriate for data migrations.
sravfeyn
force-pushed
the
sr/currency_fk
branch
from
a464180 to
7f901d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
commcare_connect/opportunity/tests/test_models.py (1)
1-4: Migration population test covers key scenarios wellThis test exercises the important paths (valid, single-country, multi-country, and invalid currencies) and asserts both FK values and row counts, which should catch regressions in the migration logic. The mixed use of
OpportunityFactoryanddjango_apps.get_modelis reasonable here for driving the same table via current models and the migration-style API.If you want to make the invalid-currency assertion slightly more robust against future fixtures/seeding, you could also assert that
invalid_currency_codedoes not exist before callingpopulate_currency_and_country_fk, but that’s optional and not a blocker.Also applies to: 130-185
commcare_connect/opportunity/models.py (1)
62-72: Currency/Country model design and new FKs look sound; consider form-opt-in and display helpersThe normalized models and PROTECT FKs on
Opportunityline up well with the migration and data model you described; I don’t see correctness issues here.Two optional improvements you might consider:
For
Opportunity.currency_fkandOpportunity.country, they’renull=Trueand populated via migration/logic; if these ever appear on admin or ModelForms, addingblank=Truewill better reflect that they’re optional at the form/validation layer too.currency_fk = models.ForeignKey(Currency, on_delete=models.PROTECT, null=True)
country = models.ForeignKey(Country, on_delete=models.PROTECT, null=True)
- currency_fk = models.ForeignKey(Currency, on_delete=models.PROTECT, null=True, blank=True)
- country = models.ForeignKey(Country, on_delete=models.PROTECT, null=True, blank=True)
- Adding simple `__str__` implementations for `Currency` and `Country` (e.g., `return self.code` or `f"{self.code} - {self.name}"`) would make them much easier to work with in the Django admin and shell. Both are non-blocking and can be deferred if you prefer. Also applies to: 100-102 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 213bf43b10f8c2d76b42f6e52f80e0b99edf3234 and 7f901d626732c90acdac746ec5e95ef334d89d0f. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `commcare_connect/opportunity/models.py` (2 hunks) * `commcare_connect/opportunity/tests/test_models.py` (3 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>commcare_connect/opportunity/tests/test_models.py (2)</summary><blockquote> <details> <summary>commcare_connect/opportunity/tests/factories.py (1)</summary> * `OpportunityFactory` (47-64) </details> <details> <summary>commcare_connect/opportunity/migrations/0088_currency_country_opportunity_currency_fk.py (1)</summary> * `populate_currency_and_country_fk` (468-469) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: pytest </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/opportunity/tests/test_models.py (1)
144-144: Test coverage gap: Verify that INR (single-country currency) sets the country field.The variable
valid_currency_only = "INR"was flagged in a previous review as misleading because INR maps to a single country (India) according to the COUNTRIES data in the migration. While the previous comment was marked as resolved, the test still doesn't verify that thecountryfield is populated for INR, unlike the assertion for KES at lines 173-174.If INR is indeed a single-country currency, the test should verify that both
currency_fkandcountryare set, similar to the KES test case.🔎 Suggested enhancement to verify country is set for INR
+ # INR should also set country since it maps to a single country (India) + assert opp_valid.country_id == CountryModel.objects.get(code="IND").pkOptionally, consider renaming the variable to better reflect its purpose:
- valid_currency_only = "INR" + single_country_currency_inr = "INR" # India onlyThen update line 146 and 152 accordingly:
- assert CurrencyModel.objects.filter(code=valid_currency_only).exists() + assert CurrencyModel.objects.filter(code=single_country_currency_inr).exists()- opp_valid = OpportunityFactory(currency=valid_currency_only, currency_fk=None, country=None) + opp_valid = OpportunityFactory(currency=single_country_currency_inr, currency_fk=None, country=None)- assert opp_valid.currency_fk_id == valid_currency_only + assert opp_valid.currency_fk_id == single_country_currency_inr + assert opp_valid.country_id == CountryModel.objects.get(code="IND").pkAlso applies to: 169-171
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commcare_connect/opportunity/models.pycommcare_connect/opportunity/tests/test_models.pycommcare_connect/program/migrations/0004_program_currency_fk.py
🧰 Additional context used
🧬 Code graph analysis (2)
commcare_connect/opportunity/tests/test_models.py (1)
commcare_connect/opportunity/tests/factories.py (1)
OpportunityFactory(47-64)
commcare_connect/program/migrations/0004_program_currency_fk.py (1)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)
🪛 Ruff (0.14.8)
commcare_connect/program/migrations/0004_program_currency_fk.py
9-9: Unused function argument: schema_editor
(ARG001)
14-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
19-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (4)
commcare_connect/opportunity/tests/test_models.py (1)
130-186: Well-structured migration test with comprehensive scenario coverage.The test effectively validates the migration logic across multiple scenarios: valid single-country currencies, multi-country currencies, and invalid currency codes. The use of
importlibto dynamically load the migration function and the verification of counts before and after migration demonstrate thorough testing practices.commcare_connect/opportunity/models.py (2)
62-72: Well-designed Currency and Country models following ISO standards.The models correctly implement:
- ISO 4217 currency codes (3-character primary keys)
- ISO 3166-1 alpha-3 country codes (3-character primary keys)
is_validflag on Currency to track placeholder/invalid currencies created during migration- Nullable
currencyFK on Country withSET_NULLto handle countries without official currenciesThis design provides a solid foundation for normalizing currency and country data.
101-102: LGTM: Appropriate use of PROTECT for referential integrity.The use of
on_delete=models.PROTECTfor bothcurrency_fkandcountryensures that Currency and Country reference data cannot be accidentally deleted while in use. The nullable design allows for backward compatibility during migration from the existingcurrencyCharField.commcare_connect/program/migrations/0004_program_currency_fk.py (1)
1-39: Well-structured migration with proper dependency ordering and shared utility reuse.The migration correctly:
- Adds
currency_fkandcountryforeign keys to the Program model- Uses
on_delete=PROTECTconsistently with the Opportunity model- Reuses the shared utility
populate_currency_and_country_fk_for_modelfor data migration- Declares dependencies on the opportunity migration that creates Currency and Country models
- Sets
run_on_secondary=Falseto prevent execution on read replicasNote: The static analysis hints about unused
schema_editorandClassVarannotations are false positives for Django migration patterns and can be safely ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py
🧰 Additional context used
🧬 Code graph analysis (1)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (2)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-70)commcare_connect/opportunity/models.py (2)
Currency(62-65)Country(68-71)
🪛 Ruff (0.14.8)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py
433-433: Unused function argument: schema_editor
(ARG001)
468-468: Unused function argument: schema_editor
(ARG001)
473-475: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
477-525: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (5)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (5)
1-6: LGTM!The imports are appropriate for this migration. Good use of the shared utility function from
commcare_connect.program.utilsto avoid code duplication.
12-263: LGTM!The country data is well-structured and correctly sourced from ISO standards. The None values for currencies (Antarctica, South Georgia, Palestine) are appropriate for territories without official currencies.
433-464: LGTM!The data loading logic is well-structured:
- Creates currencies first with
ignore_conflicts=Truefor safe re-runs- Efficiently builds lookup dictionary
- Correctly handles countries without currencies (returns None from
.get())- Uses bulk operations for performance
The unused
schema_editorparameter flagged by static analysis is a false positive—it's required by Django's migration signature.
468-470: LGTM!Good delegation to the shared utility function for FK population. This promotes code reuse and maintains consistency with the Program model migration.
The unused
schema_editorparameter flagged by static analysis is a false positive—it's required by Django's migration signature.
472-525: LGTM!The migration structure is correct:
- Model definitions match the final state in
models.py- Proper sequencing: create models → add fields → load data → populate FKs
ignore_conflicts=Trueenables safe re-runsnoopreverse is acceptable for data migrationsrun_on_secondary=Falsecorrectly prevents redundant execution on replica databasesThe
on_deletepolicies establish appropriate data protection:
PROTECTon Opportunity FKs prevents accidental deletion of currencies/countries in useSET_NULLon Country.currency allows reference data maintenance when not constrained by operational dataThe ClassVar annotations flagged by static analysis are false positives for Django migration classes.
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
commcare_connect/opportunity/tests/test_models.py (1)
169-170: Incomplete assertion: INR is a single-country currency that should also set country.The test verifies that
opp_valid.currency_fk_idis set to"INR"but doesn't check whether thecountryfield is populated. According to the COUNTRIES data in the migration (line 119), INR maps to a single country ("IND", "India", "INR"), so the country field should also be set, similar to how KES is tested at lines 173-174.🔎 Proposed fix to add country assertion for INR
# valid currency should map to fk assert opp_valid.currency_fk_id == valid_currency_only + # INR is single-country (India), should also set country + assert opp_valid.country_id == CountryModel.objects.get(code="IND").pk
🧹 Nitpick comments (2)
commcare_connect/program/utils.py (2)
47-47: Remove unusednoqadirective.The
noqa: E203directive is unused since Ruff does not enable the E203 rule by default.🔎 Proposed fix
- batch = list(qs[start : start + BATCH_SIZE]) # noqa: E203 + batch = list(qs[start : start + BATCH_SIZE])
55-60: Redundant dictionary assignment for invalid currency codes.When an invalid currency code is encountered, line 58 reassigns the USD currency object back to the
code_to_currencydictionary usingcode_to_currency[currency_obj.code] = currency_obj. Sincecurrency_objis retrieved fromcode_to_currency["USD"], USD should already exist in the dictionary, making this assignment unnecessary.🔎 Proposed fix
if raw_code not in code_to_currency: # Set to USD when code is incorrect currency_obj = code_to_currency["USD"] - code_to_currency[currency_obj.code] = currency_obj else: currency_obj = code_to_currency[raw_code]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.pycommcare_connect/opportunity/models.pycommcare_connect/opportunity/tests/test_models.pycommcare_connect/program/utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
commcare_connect/program/utils.py (1)
commcare_connect/opportunity/models.py (2)
Currency(62-64)Country(67-70)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (2)
commcare_connect/program/utils.py (1)
populate_currency_and_country_fk_for_model(26-68)commcare_connect/opportunity/models.py (2)
Currency(62-64)Country(67-70)
🪛 Ruff (0.14.8)
commcare_connect/program/utils.py
47-47: Unused noqa directive (non-enabled: E203)
Remove unused noqa directive
(RUF100)
commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py
432-432: Unused function argument: schema_editor
(ARG001)
466-466: Unused function argument: schema_editor
(ARG001)
471-473: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
475-522: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (8)
commcare_connect/program/utils.py (1)
26-44: LGTM: Function setup and lookup logic is correct.The function signature, model retrieval, and dictionary construction for currency/country lookups are well-structured for efficient batch processing.
commcare_connect/opportunity/tests/test_models.py (1)
130-149: LGTM: Test setup correctly loads migration and verifies data.The test properly uses
importlibto dynamically load the migration module and retrieves models viadjango_apps. The currency/country setup assertions ensure the test data is properly initialized.commcare_connect/opportunity/models.py (2)
62-71: LGTM: Currency and Country models are well-defined.The models follow ISO standards (ISO 4217 for currencies, ISO 3166-1 alpha-3 for countries). Using
on_delete=SET_NULLforCountry.currencyis appropriate since some countries have no official currency.
100-101: LGTM: Foreign key constraints use appropriateon_deletebehavior.Using
on_delete=PROTECTfor bothcurrency_fkandcountryprevents accidental deletion of Currency or Country records that are referenced by Opportunity records, maintaining referential integrity as discussed in prior reviews.commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (4)
9-263: LGTM: COUNTRIES data is comprehensive and properly sourced.The country data follows ISO 3166-1 alpha-3 standards and correctly handles countries without official currencies by using
None. The data source attribution is helpful for future maintenance.
266-429: LGTM: CURRENCIES data is comprehensive and follows ISO 4217.The currency data is properly sourced from ISO standards and covers a comprehensive set of global currencies. The duplicate entry previously flagged has been resolved.
432-462: LGTM: Data loading function is efficient and idempotent.The function uses
bulk_createwithignore_conflicts=Truefor efficient, idempotent data loading. The lookup dictionary approach correctly handles the Currency-Country relationships, including countries without official currencies.Note: The Ruff warning about unused
schema_editorparameter is a false positive—Django migration functions require this signature.
466-522: LGTM: Migration structure is correct and properly sequenced.The migration correctly:
- Delegates to the shared utility function for FK population
- Sequences operations to create models before adding fields
- Uses
RunPython.noopfor reverse operations (appropriate for data migrations)- Sets
run_on_secondary=Falseto prevent duplicate execution on database replicasNote: The Ruff warnings about unused
schema_editorandClassVarannotations are false positives—these are standard Django migration patterns.
Replace currency field with currency_fk
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
commcare_connect/opportunity/forms.py (2)
1629-1629: Update to use currency_fk instead of currency field.Line 1629 references
self.opportunity.currency, but the model now usescurrency_fk. This should be updated toself.opportunity.currency_fk.code if self.opportunity.currency_fk else "".🔎 Proposed fix
- label=f"Amount ({self.opportunity.currency})", + label=f"Amount ({self.opportunity.currency_fk.code if self.opportunity.currency_fk else ''})",
1684-1684: Update to use currency_fk instead of currency field.Lines 1684 and 1696 reference
self.opportunity.currency, but the model now usescurrency_fk. These should be updated to useself.opportunity.currency_fk.code.🔎 Proposed fix
- exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date) + exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency_fk.code if self.opportunity.currency_fk else None, date) if not exchange_rate: raise ValidationError("Exchange rate not available for selected date.") cleaned_data["exchange_rate"] = exchange_rate cleaned_data["amount_usd"] = round(amount / exchange_rate.rate, 2) cleaned_data["title"] = None cleaned_data["start_date"] = None cleaned_data["end_date"] = None cleaned_data["notes"] = None - exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date) + # This line appears to be a duplicate - consider removing if already handled above + exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency_fk.code if self.opportunity.currency_fk else None, date)Also applies to: 1696-1696
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
commcare_connect/opportunity/forms.pycommcare_connect/opportunity/models.pycommcare_connect/opportunity/tests/factories.pycommcare_connect/opportunity/tests/test_forms.pycommcare_connect/opportunity/tests/test_models.pycommcare_connect/program/forms.pycommcare_connect/program/tests/factories.pycommcare_connect/program/tests/test_forms.pycommcare_connect/program/tests/test_views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- commcare_connect/opportunity/models.py
🧰 Additional context used
🧬 Code graph analysis (7)
commcare_connect/opportunity/tests/factories.py (1)
commcare_connect/opportunity/models.py (1)
Currency(62-67)
commcare_connect/program/tests/factories.py (1)
commcare_connect/opportunity/models.py (1)
Currency(62-67)
commcare_connect/opportunity/tests/test_models.py (2)
commcare_connect/opportunity/tests/factories.py (1)
OpportunityFactory(47-64)commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (1)
populate_currency_and_country_fk(466-467)
commcare_connect/program/forms.py (2)
commcare_connect/opportunity/models.py (2)
Country(70-76)Currency(62-67)commcare_connect/program/models.py (1)
Program(9-28)
commcare_connect/program/tests/test_forms.py (1)
commcare_connect/program/forms.py (1)
ProgramForm(13-87)
commcare_connect/program/tests/test_views.py (1)
commcare_connect/opportunity/models.py (1)
Currency(62-67)
commcare_connect/opportunity/forms.py (1)
commcare_connect/opportunity/models.py (16)
Country(70-76)Currency(62-67)Meta(266-267)Meta(284-289)Meta(390-395)Meta(470-473)Meta(511-512)Meta(576-577)Meta(775-783)Meta(800-803)Meta(842-846)Meta(886-887)Meta(900-901)Opportunity(79-232)ExchangeRate(464-484)latest_exchange_rate(476-484)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (10)
commcare_connect/opportunity/tests/factories.py (1)
3-3: LGTM! Factory updated correctly to use currency_fk.The factory appropriately uses
LazyFunctionto fetch the Currency object dynamically. This ensures the Currency record exists before attempting to create an Opportunity.Also applies to: 7-7, 60-60
commcare_connect/program/tests/factories.py (1)
1-1: LGTM! Factory correctly updated to use currency_fk.The changes align with the new Currency model and use
LazyFunctionappropriately to fetch the Currency object at factory build time.Also applies to: 4-4, 15-15
commcare_connect/opportunity/tests/test_forms.py (1)
61-61: LGTM! Tests correctly updated to use currency_fk.The test updates consistently reference
currency_fkand access the code viacurrency_fk.codewhere needed. The changes align well with the new Currency model structure.Also applies to: 77-77, 90-90, 103-103, 196-199, 302-302, 310-310, 367-367
commcare_connect/program/forms.py (2)
14-25: LGTM! Currency and Country fields properly added.The new
currency_fkandcountryfields are correctly defined asModelChoiceFields with appropriate querysets, widgets, and labels.
97-102: LGTM! Managed opportunities correctly inherit program currency and country.The loop correctly sets both
currency_fkandcountryfields to readonly and disabled, ensuring managed opportunities use the program's values.commcare_connect/opportunity/tests/test_models.py (1)
130-186: LGTM! Comprehensive migration test with good coverage.The test thoroughly exercises the currency and country FK population logic, covering:
- Valid currency codes mapping to currency_fk
- Single-country currencies auto-setting country FK
- Multi-country currencies leaving country as NULL
- Invalid currencies defaulting to USD without creating new records
The test correctly verifies that Currency and Country counts remain unchanged, confirming no spurious records are created.
commcare_connect/opportunity/forms.py (2)
112-123: LGTM! Currency and Country fields properly added to forms.The
currency_fkandcountryfields are correctly defined asModelChoiceFields with appropriate querysets, widgets, and labels for bothOpportunityChangeFormandOpportunityInitForm.Also applies to: 316-327
1019-1022: LGTM! Correctly uses currency_fk.code for label.The code safely accesses the currency code via
currency_fk.codewith appropriate null checking.commcare_connect/program/tests/test_views.py (1)
47-47: Thecurrency_fkfield in ProgramForm is aModelChoiceFieldthat by default uses the model's primary key for validation. ModelChoiceField's to_python method usesself.to_field_name or 'pk'to query the queryset. The Currency model definescodeas its primary key (code = models.CharField(max_length=3, primary_key=True)), so passing the string"USD"at line 47 is correct and consistent with line 73—both provide the primary key value. No changes needed.commcare_connect/program/tests/test_forms.py (1)
40-40: No action needed—test data is consistent.The
currency_fkfield correctly uses string codes ("USD", "EUR") because theCurrencymodel definescodeas the primary key.ModelChoiceFieldexpects primary keys by default, so passing"USD"is the correct format. Bothtest_forms.pyline 40 andtest_views.pyline 73 use the code string (the primary key) consistently, confirming the test data is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commcare_connect/opportunity/forms.py (1)
1681-1702: Usecurrency_fkconsistently and remove duplicate code.Lines 1681 and 1693 reference the old
currencyfield instead of the newcurrency_fk.codepattern used elsewhere in this file. Additionally, the exchange rate calculation logic is duplicated in lines 1681-1686 and 1693-1702.This creates several issues:
- Inconsistency: Line 1626 in the same form uses
currency_code, while lines 1681 and 1693 use the oldcurrencyfield- Inconsistency with other forms:
PaymentInvoiceForm.clean()at line 1450 usescurrency_fk.code- Potential breakage: If the old
currencyCharField is deprecated or removed, this code will fail- Missing null safety: Unlike line 1450, there's no defensive check for
currency_fkexistence🔎 Proposed fix to use currency_fk and eliminate duplication
def clean(self): cleaned_data = super().clean() amount = cleaned_data.get("amount") date = cleaned_data.get("date") if amount is None or date is None: return cleaned_data # Let individual field errors handle missing values if not self.is_service_delivery: - exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date) + currency = getattr(self.opportunity, "currency_fk", None) + exchange_rate = ExchangeRate.latest_exchange_rate(currency.code if currency else None, date) if not exchange_rate: raise ValidationError("Exchange rate not available for selected date.") cleaned_data["exchange_rate"] = exchange_rate - cleaned_data["amount_usd"] = round(amount / exchange_rate.rate, 2) cleaned_data["title"] = None cleaned_data["start_date"] = None cleaned_data["end_date"] = None cleaned_data["notes"] = None - exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date) - if not exchange_rate: - raise ValidationError("Exchange rate not available for selected date.") - if cleaned_data.get("usd_currency"): cleaned_data["amount_usd"] = amount cleaned_data["amount"] = round(amount * exchange_rate.rate, 2) else: cleaned_data["amount"] = amount cleaned_data["amount_usd"] = round(amount / exchange_rate.rate, 2) else: start_date = cleaned_data.get("start_date") end_date = cleaned_data.get("end_date") if not start_date: raise ValidationError({"start_date": "Start date is required for service delivery invoices."}) if not end_date: raise ValidationError({"end_date": "End date is required for service delivery invoices."}) if end_date < start_date: raise ValidationError({"end_date": "End date cannot be earlier than start date."}) return cleaned_data
🧹 Nitpick comments (14)
commcare_connect/opportunity/tests/test_completed_work.py (1)
143-144: Same as lines 83-84: Consider more explicit FK assignment.Same concern as the previous test method. Assigning
"EUR"directly to the ForeignKeycurrency_fkis implicit. Usecurrency_fk_id = "EUR"or fetch the Currency instance for clarity and reliability.🔎 Proposed refactor
- opp_access.opportunity.currency_fk = "EUR" + opp_access.opportunity.currency_fk_id = "EUR" opp_access.opportunity.save()commcare_connect/opportunity/tasks.py (1)
293-296: Add fallback for currency_code in notification message.If
currency_codereturnsNone(whencurrency_fkis not set), the notification message will display "None" instead of a currency code, which could confuse users.🔎 Proposed fix
"body": gettext( "You have received a payment of " - f"{opportunity.currency_code} {payment.amount} for {opportunity.name}.", + f"{opportunity.currency_code or ''} {payment.amount} for {opportunity.name}.", ),commcare_connect/templates/opportunity/user_visit_verification.html (2)
134-134: Add fallback for currency_code in label.If
currency_codereturnsNone, the label will display "Accrued Amount (None)" which looks unprofessional. Consider adding a default filter.🔎 Proposed fix
- <span>Accrued Amount ({{ opportunity.currency_code }})</span> + <span>Accrued Amount ({{ opportunity.currency_code|default:'' }})</span>
147-147: Add fallback for currency_code in label.If
currency_codereturnsNone, the label will display "Paid Amount (None)" which looks unprofessional. Consider adding a default filter.🔎 Proposed fix
- <span>Paid Amount ({{ opportunity.currency_code }})</span> + <span>Paid Amount ({{ opportunity.currency_code|default:'' }})</span>commcare_connect/templates/opportunity/email/invoice_paid.txt (1)
9-9: Add fallback for currency_code in email template.If
currency_codereturnsNone, the email will display "None" as the currency, which could confuse recipients. Since this is a plain text template, consider using a filter or conditional logic to provide a fallback.🔎 Proposed fix
-Amount Paid: {{opportunity.currency_code}} {{ invoice.amount }} +Amount Paid: {{opportunity.currency_code|default:''}} {{ invoice.amount }}commcare_connect/opportunity/tables.py (3)
414-414: Add fallback for currency_code in column header.If
currency_codereturnsNone, the column header will display "Amount (None)" which looks unprofessional.🔎 Proposed fix
- self.base_columns["amount"].verbose_name = f"Amount ({self.opportunity.currency_code})" + self.base_columns["amount"].verbose_name = f"Amount ({self.opportunity.currency_code or ''})"
647-654: Add fallback for currency_code in rendered value.If
currency_codereturnsNone, the cell will display "None" before the amount, which looks unprofessional.🔎 Proposed fix
def render_payments_due(self, value, record): if value is None: value = 0 - value = f"{record.currency_code} {intcomma(value)}" + value = f"{record.currency_code or ''} {intcomma(value)}" return self.render_worker_list_url_column( value=value, opp_id=record.id, url_slug="worker_payments", sort="sort=-total_paid" )
735-740: Add fallback for currency_code in rendered value.If
currency_codereturnsNone, the cell will display "None" before the amount, which looks unprofessional.🔎 Proposed fix
def render_worker_earnings(self, value, record): url = reverse("opportunity:worker_payments", args=(self.org_slug, record.id)) url += "?sort=-payment_accrued" - value = f"{record.currency_code} {intcomma(value)}" + value = f"{record.currency_code or ''} {intcomma(value)}" value = format_html('<a href="{}">{}</a>', url, value) return self._render_div(value, extra_classes=self.stats_style)commcare_connect/templates/components/worker_page/payment_history.html (1)
35-35: Add fallback for currency_code in confirmation message.If
currency_codereturnsNone, the confirmation message will display "None" as the currency, which could confuse users.🔎 Proposed fix
- Please confirm to rollback the payment of <b>{{ latest_payment.amount }} {{ opportunity.currency_code }}</b> + Please confirm to rollback the payment of <b>{{ latest_payment.amount }} {{ opportunity.currency_code|default:'' }}</b>commcare_connect/templates/opportunity/email/invoice_paid.html (1)
14-14: Add fallback for currency_code in email template.If
currency_codereturnsNone, the email will display "None" as the currency, which could confuse recipients.🔎 Proposed fix
- <li><strong>{% trans "Amount Paid" %}:</strong> {{ opportunity.currency_code }} {{ invoice.amount }}</li> + <li><strong>{% trans "Amount Paid" %}:</strong> {{ opportunity.currency_code|default:'' }} {{ invoice.amount }}</li>commcare_connect/program/tests/test_views.py (1)
40-50: Program create/update tests correctly switched to FK-based currency fieldsUsing
"currency_fk": "USD"/"INR"and"country": "USA"/"IND"matches the new FK definitions (PK is the 3‑letter code), and assertingself.program.currency_fk_id == data["currency_fk"]verifies the update path. Consider also assertingcountry_idif you want full coverage of the new field, but this is optional.Also applies to: 67-85
commcare_connect/program/views.py (1)
329-345: Useselect_related("currency_fk")to avoid per-row queries forcurrency_codeSwitching to
f"{data.currency_code} {data.pending_payment}"is correct with the new FK model, but withoutselect_related("currency_fk")this can issue one extra query per opportunity. Consider:Proposed optimization
pending_payments_data_opps = ( - Opportunity.objects.filter(managed=True, organization=org) + Opportunity.objects.filter(managed=True, organization=org).select_related("currency_fk") .annotate( pending_payment=Sum("opportunityaccess__payment_accrued") - Sum("opportunityaccess__payment__amount") ) .filter(pending_payment__gte=0) )commcare_connect/opportunity/views.py (1)
454-460: Consistent migration fromcurrencytocurrency_codeacross opportunity viewsAll these call sites now correctly use
request.opportunity.currency_code(orresult.currency_code) for display, reporting, and table labeling, and pass the code into helpers (InvoiceLineItemsTable,InvoiceDeliveriesTable,payment_report,exchange_rate_preview) that expect a currency string. This aligns with the new FK-backed currency model without changing semantics.If you want to polish UX for opportunities lacking a currency, you could conditionally omit the currency prefix (as you already do in
amount_with_currency) wherever the code might beNone, but it’s not required for correctness.Also applies to: 667-675, 1295-1305, 1529-1538, 2610-2612, 2764-2787, 2813-2836, 2884-2887, 2920-2921
commcare_connect/conftest.py (1)
1-4: Autouse fixture for seeding Currency/Country data is soundThe
ensure_currency_country_datafixture correctly reuses the 0092 migration’sload_currency_and_country_datafunction and guards onCurrency.objects.exists(), so tests always have ISO currency/country rows without duplicating logic. The unuseddbparameter is intentional to activate the database; you can ignore Ruff’s ARG001 here or silence it with# noqa: ARG001if it’s noisy.Also applies to: 155-163
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
commcare_connect/conftest.pycommcare_connect/opportunity/forms.pycommcare_connect/opportunity/models.pycommcare_connect/opportunity/tables.pycommcare_connect/opportunity/tasks.pycommcare_connect/opportunity/tests/factories.pycommcare_connect/opportunity/tests/test_completed_work.pycommcare_connect/opportunity/utils/completed_work.pycommcare_connect/opportunity/views.pycommcare_connect/opportunity/visit_import.pycommcare_connect/program/models.pycommcare_connect/program/tests/factories.pycommcare_connect/program/tests/test_views.pycommcare_connect/program/views.pycommcare_connect/templates/components/worker_page/payment_history.htmlcommcare_connect/templates/opportunity/email/invoice_paid.htmlcommcare_connect/templates/opportunity/email/invoice_paid.txtcommcare_connect/templates/opportunity/invoice_payment_report.htmlcommcare_connect/templates/opportunity/user_visit_verification.htmlcommcare_connect/templates/program/pm_home.html
✅ Files skipped from review due to trivial changes (1)
- commcare_connect/templates/opportunity/invoice_payment_report.html
🚧 Files skipped from review as they are similar to previous changes (2)
- commcare_connect/program/tests/factories.py
- commcare_connect/program/models.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-11T08:11:43.757Z
Learnt from: sravfeyn
Repo: dimagi/commcare-connect PR: 804
File: commcare_connect/opportunity/views.py:909-910
Timestamp: 2025-11-11T08:11:43.757Z
Learning: In commcare_connect/opportunity/views.py, when calling update_payment_accrued, it's acceptable and more efficient to pass a QuerySet (e.g., visits.values_list("user_id", flat=True).distinct()) rather than converting to a list, as the function only uses it in a filter(user__in=users) context where Django natively supports QuerySets.
Applied to files:
commcare_connect/opportunity/visit_import.py
🧬 Code graph analysis (9)
commcare_connect/opportunity/utils/completed_work.py (3)
commcare_connect/opportunity/visit_import.py (1)
get_exchange_rate(423-441)commcare_connect/opportunity/models.py (1)
currency_code(119-123)commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/opportunity/visit_import.py (2)
commcare_connect/opportunity/models.py (1)
currency_code(119-123)commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/opportunity/tests/factories.py (2)
commcare_connect/commcarehq/tests/factories.py (1)
HQServerFactory(14-21)commcare_connect/opportunity/models.py (3)
Country(70-76)Currency(62-67)VisitValidationStatus(462-468)
commcare_connect/opportunity/views.py (3)
commcare_connect/opportunity/models.py (3)
currency_code(119-123)ExchangeRate(471-491)latest_exchange_rate(483-491)commcare_connect/program/models.py (1)
currency_code(31-35)commcare_connect/opportunity/tables.py (2)
InvoiceLineItemsTable(1582-1611)InvoiceDeliveriesTable(1614-1641)
commcare_connect/opportunity/tasks.py (2)
commcare_connect/opportunity/models.py (1)
currency_code(119-123)commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/program/views.py (2)
commcare_connect/opportunity/models.py (1)
currency_code(119-123)commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/opportunity/tables.py (2)
commcare_connect/opportunity/models.py (1)
currency_code(119-123)commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/opportunity/models.py (1)
commcare_connect/program/models.py (1)
currency_code(31-35)
commcare_connect/conftest.py (2)
commcare_connect/opportunity/models.py (1)
Currency(62-67)commcare_connect/opportunity/migrations/0092_currency_country_opportunity_currency_fk.py (1)
load_currency_and_country_data(432-462)
🪛 Ruff (0.14.8)
commcare_connect/opportunity/visit_import.py
318-318: Avoid specifying long messages outside the exception class
(TRY003)
commcare_connect/conftest.py
155-155: Unused function argument: db
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (15)
commcare_connect/opportunity/tests/test_completed_work.py (1)
83-84: The code is correct and follows standard Django patterns. Django supports CharField primary keys on models, and ForeignKey fields can be assigned by the primary key value directly. The EUR currency is guaranteed to exist because migration 0092 bulk creates all currencies, and conftest.py's autouse fixture ensures they're loaded for all tests. Usingcurrency_fk = "EUR"is standard and clear; the suggested alternatives are purely optional style preferences.Likely an incorrect or invalid review comment.
commcare_connect/templates/program/pm_home.html (1)
93-93: LGTM: Currency display updated correctly.The budget line now properly renders the currency using
currency_codewith a fallback to an empty string when the currency is not set.commcare_connect/opportunity/utils/completed_work.py (2)
126-126: LGTM: Exchange rate calculation updated correctly.The code now uses
currency_codefor exchange rate lookups. Theget_exchange_ratefunction (shown in relevant code snippets) properly validates thatcurrency_codeis notNoneand raises anImportExceptionif it is, ensuring robust error handling.
224-226: LGTM: Exchange rate calculation updated correctly.The code now uses
currency_codefor exchange rate lookups. Theget_exchange_ratefunction properly validates thatcurrency_codeis notNoneand raises anImportExceptionif it is, ensuring robust error handling.commcare_connect/opportunity/tables.py (1)
1139-1147: LGTM: Proper error handling for currency_code access.The try-except block correctly handles potential
IndexErrorandAttributeErrorwhen accessingcurrency_code, defaulting to an empty string. This defensive programming ensures the table renders gracefully even when data is missing.commcare_connect/opportunity/visit_import.py (1)
302-320: Currency handling in payment import correctly migrated tocurrency_codeUsing
opportunity.currency_codefor both the initialexchange_rate_todayand dated lookups keeps behavior consistent with the new FK-backed model and withget_exchange_rate’s expectations. The updated error text (“Currency code … is invalid”) is also clearer and aligned with the new field.No further changes needed here.
Also applies to: 392-403
commcare_connect/opportunity/tests/factories.py (1)
3-7: OpportunityFactory defaults correctly wired to Currency/CountryUsing
LazyFunction(lambda: Currency.objects.get(code="USD"))andCountry.objects.get(code="USA")is a good way to keep factory defaults in sync with the seeded ISO data. This pairs cleanly with theensure_currency_country_datafixture and keeps tests usingOpportunityFactoryon the new FK-backed currency/country model without leaking integer IDs.Nothing blocking here.
Also applies to: 47-62
commcare_connect/opportunity/models.py (1)
62-77: New Currency/Country models and Opportunity FKs look correct
Currency/Countryare modeled with sensible 3‑char PKs,Country.currencyusesSET_NULLappropriately, andOpportunity.currency_fk/countryusePROTECT, which matches earlier review guidance for referential integrity. Thecurrency_codeproperty is a simple, reusable accessor that downstream views/tests are already using consistently.No issues from a modeling or API perspective.
Also applies to: 104-107, 118-123
commcare_connect/opportunity/forms.py (7)
21-23: LGTM!The imports for
CountryandCurrencymodels are correctly added.
112-123: LGTM!The
currency_fkandcountryModelChoiceField definitions are correctly implemented with appropriate widgets and ordering.
131-132: LGTM!The Meta fields and layout correctly reference the new
currency_fkandcountryfields instead of the deprecatedcurrencyfield.Also applies to: 191-192
316-327: LGTM!The
OpportunityInitFormcorrectly implements the samecurrency_fkandcountryfield pattern asOpportunityChangeForm.Also applies to: 335-336, 369-370
588-590: LGTM!The initial field population correctly includes
currency_fkandcountryinstead of the oldcurrencyfield.
1019-1020: LGTM!The conditional label update using
currency_codeproperty is defensively coded and handles the case when currency information might be unavailable.
1388-1388: LGTM!The
PaymentInvoiceFormuses defensive patterns withgetattrand conditional access to safely retrieve currency information fromcurrency_fk.Also applies to: 1402-1402, 1449-1450
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/program/tests/test_forms.py (1)
65-75: Consider renaming test to reflect actual validation.The test name
test_program_form_currency_lengthis now misleading since it validates that an invalid currency code (not a length constraint) is rejected. A more descriptive name would betest_program_form_invalid_currency.🔎 Suggested rename
- def test_program_form_currency_length(self, program_manager_org_user_admin, program_manager_org, delivery_type): + def test_program_form_invalid_currency(self, program_manager_org_user_admin, program_manager_org, delivery_type):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
commcare_connect/opportunity/tests/test_completed_work.pycommcare_connect/opportunity/tests/test_forms.pycommcare_connect/opportunity/utils/completed_work.pycommcare_connect/program/tests/test_forms.pycommcare_connect/reports/helpers.pycommcare_connect/reports/tests/test_reports.py
🚧 Files skipped from review as they are similar to previous changes (3)
- commcare_connect/opportunity/utils/completed_work.py
- commcare_connect/opportunity/tests/test_forms.py
- commcare_connect/opportunity/tests/test_completed_work.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-11T08:11:43.757Z
Learnt from: sravfeyn
Repo: dimagi/commcare-connect PR: 804
File: commcare_connect/opportunity/views.py:909-910
Timestamp: 2025-11-11T08:11:43.757Z
Learning: In commcare_connect/opportunity/views.py, when calling update_payment_accrued, it's acceptable and more efficient to pass a QuerySet (e.g., visits.values_list("user_id", flat=True).distinct()) rather than converting to a list, as the function only uses it in a filter(user__in=users) context where Django natively supports QuerySets.
Applied to files:
commcare_connect/reports/helpers.py
🧬 Code graph analysis (1)
commcare_connect/program/tests/test_forms.py (1)
commcare_connect/program/forms.py (1)
ProgramForm(13-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (6)
commcare_connect/reports/helpers.py (1)
126-127: LGTM! Currency filter successfully migrated to FK-based approach.The update to filter by
currency_fkinstead of the denormalizedcurrencyfield is correct and consistent across both filter dictionaries. The Django ORM will properly handle the FK relationship.commcare_connect/reports/tests/test_reports.py (1)
249-249: LGTM! Correct FK ID assignment in test factory.The update to use
opportunity__currency_fk_idcorrectly assigns the Currency FK by its primary key (ISO code). The test properly validates that currency-based filtering works as expected with the new FK relationship.commcare_connect/program/tests/test_forms.py (4)
34-44: LGTM!Test data correctly updated to use
currency_fkandcountryfields with string code values, consistent with the ModelChoiceField configuration in the form.
77-91: LGTM!The assertion correctly accesses
currency_fk.codeto compare against the string value submitted in the form data.
124-130: LGTM!The test correctly verifies that
currency_fkis initialized from the program and marked as readonly/disabled to prevent modification.
149-165: LGTM!The test correctly validates that
currency_fkis propagated from the program to the managed opportunity upon form save.
ajeety4
left a comment
ajeety4
left a comment
There was a problem hiding this comment.
The latest change looks good to me
Product Description
https://dimagi.atlassian.net/browse/CCCT-1864
No product facing changes.
Technical Summary
I have run a script to analyze current prod opportunity/currency data to check invalid currencies etc. I have compiled the detailed data here in the doc. In summary
invalid=TrueSafety Assurance
Safety story
Tested locally. Added relevant tests, and ran the analysis on prod
Automated test coverage
Added
QA Plan
Will be testing on staging to make sure migration goes out smoothly.
Labels & Review