feat: add exchage rate support for JE #700
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/quickbooks_online/migrations/0017_journalentry_exchange_rate.py(1 hunks)apps/quickbooks_online/models.py(2 hunks)apps/quickbooks_online/utils.py(2 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql
🧰 Additional context used
🪛 Ruff (0.8.0)
apps/quickbooks_online/utils.py
1242-1242: Use exchange_rate.get('Rate', 1) instead of an if block
Replace with exchange_rate.get('Rate', 1)
(SIM401)
🔇 Additional comments (4)
apps/quickbooks_online/utils.py (1)
1244-1245: Confirm that the exchange_rate field is nullable if not always available
Since you're saving journal_entry.exchange_rate = journal_entry_payload['ExchangeRate'], ensure that the exchange_rate field in the JournalEntry model is nullable or has a default value to handle cases where the exchange rate might not be available.
Verify the JournalEntry model field:
✅ Verification successful
Exchange rate field is properly nullable in the model
The exchange_rate field in the JournalEntry model is correctly defined as nullable with null=True, which safely handles cases where the exchange rate might not be available in the payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the `exchange_rate` field in the `JournalEntry` model is nullable.
# Test: Search for the `exchange_rate` field definition in the models.py file.
# Expect: The field should have `null=True` and `blank=True` if it can be empty.
fd -e py 'models.py' --exec grep -nH 'exchange_rate' {}
Length of output: 285
apps/quickbooks_online/migrations/0017_journalentry_exchange_rate.py (1)
13-17: Consider adding currency fields for complete exchange rate tracking
The current schema only tracks the exchange rate without explicitly storing the source and target currencies. This might lead to ambiguity in multi-currency scenarios. Consider adding source_currency and target_currency fields to maintain a clear record of the currency conversion context.
Let's verify if currency information is stored elsewhere in the model:
apps/quickbooks_online/models.py (2)
13-13: LGTM!
The import statement changes look good. The imported mapping classes are used throughout the file for handling various mapping operations.
741-741: Verify exchange rate handling in related components.
The addition of the exchange_rate field to the JournalEntry model looks good and aligns with the PR objective. Since this is a new field that affects currency conversion:
- Ensure the exchange rate is properly populated when creating journal entries
- Verify that the exchange rate is correctly used in calculations
Let's verify the exchange rate handling implementation:
✅ Verification successful
Exchange rate handling is properly implemented across components
The verification shows that exchange rate handling is correctly implemented:
-
The
exchange_ratefield is properly populated in both Bills and JournalEntries when creating entries:- Exchange rates are fetched from the connection's exchange rates service
- The rate is stored in the model only when multi-currency is allowed and currencies differ
- There's a fallback to rate=1 when no exchange rate is found
-
The implementation follows consistent patterns across both Bill and JournalEntry components, ensuring uniform currency conversion handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for exchange rate usage in related files
# Look for exchange rate related code
rg -l "exchange_rate" --type py
# Check for currency conversion logic
ast-grep --pattern 'def $FUNC($$$) {
$$$
exchange_rate
$$$
}'
Length of output: 357
Script:
#!/bin/bash
# Let's examine the exchange rate usage in the utils and models files
rg "exchange_rate" apps/quickbooks_online/utils.py -B 2 -A 2
# Check models.py for exchange rate field definitions and usage
rg "exchange_rate" apps/quickbooks_online/models.py -B 2 -A 2
# Look for currency conversion related functions
ast-grep --pattern 'def $FUNC($$$) {
$$$
currency
$$$
}'
Length of output: 2031
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
* feat: add exchage rate support for JE * fix tests --------- Co-authored-by: GitHub Actions <[email protected]>
Description
feat: add exchage rate support for JE
Clickup
https://app.clickup.com/t/86cx48zz2
Summary by CodeRabbit
New Features
exchange_ratefield to the journal entry model, enabling multi-currency transaction support.Bug Fixes
Chores