fix: exchange rate for ccp #706
Conversation
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request introduces significant modifications to the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![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 (2)
apps/quickbooks_online/utils.py (2)
1052-1054: Consider caching 'general_settings' and 'qbo_home_currency' in the constructorThe variables
general_settingsandqbo_home_currencyare fetched here but may be used across multiple methods. To reduce redundant database queries and improve performance, consider initializing them once in the__init__method and storing them as instance variables.
1070-1070: Simplify exchange rate retrieval usingget()methodYou can simplify the retrieval of the exchange rate by using the
get()method with a default value.Apply this diff:
- credit_card_purchase_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1 + credit_card_purchase_payload['ExchangeRate'] = exchange_rate.get('Rate', 1)🧰 Tools
🪛 Ruff (0.8.2)
1070-1070: Use
exchange_rate.get('Rate', 1)instead of anifblockReplace with
exchange_rate.get('Rate', 1)(SIM401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/quickbooks_online/utils.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
apps/quickbooks_online/utils.py
1070-1070: Use exchange_rate.get('Rate', 1) instead of an if block
Replace with exchange_rate.get('Rate', 1)
(SIM401)
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/quickbooks_online/utils.py (1)
1051-1077: Add docstring parameters and return typeThe method's docstring is missing parameter and return type documentation.
Apply this diff to improve the documentation:
def __construct_credit_card_purchase(self, credit_card_purchase: CreditCardPurchase, credit_card_purchase_lineitems: List[CreditCardPurchaseLineitem]) -> Dict: """ Create a credit card purchase - :param credit_card_purchase: credit card purchase object extracted from database - :return: constructed credit card purchase + Args: + credit_card_purchase: Credit card purchase object extracted from database + credit_card_purchase_lineitems: List of credit card purchase line items + Returns: + Dict: Constructed credit card purchase payload for QBO API + Raises: + Exception: If exchange rate is not found for the given currency """🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Use
exchange_rate.get('Rate', 1)instead of anifblockReplace with
exchange_rate.get('Rate', 1)(SIM401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/quickbooks_online/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
apps/quickbooks_online/utils.py
1074-1074: Use exchange_rate.get('Rate', 1) instead of an if block
Replace with exchange_rate.get('Rate', 1)
(SIM401)
🔇 Additional comments (1)
apps/quickbooks_online/utils.py (1)
1072-1074: 🛠️ Refactor suggestion
Improve exchange rate handling
The exchange rate handling can be improved in two ways:
- Use dict.get() for cleaner code
- Add error handling for missing exchange rates
Apply this diff to improve the exchange rate handling:
if general_settings.is_multi_currency_allowed and fyle_home_currency != qbo_home_currency and qbo_home_currency:
exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=fyle_home_currency)
- credit_card_purchase_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1
+ if not exchange_rate:
+ raise Exception(f'Exchange rate not found for currency {fyle_home_currency}')
+ credit_card_purchase_payload['ExchangeRate'] = exchange_rate.get('Rate', 1)Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Use exchange_rate.get('Rate', 1) instead of an if block
Replace with exchange_rate.get('Rate', 1)
(SIM401)
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/quickbooks_online/utils.py (1)
735-736: Refactor to eliminate code duplication when handling exchange ratesThe logic for fetching the exchange rate, assigning it to the payload, and saving it to the object is duplicated in both
purchase_object_payloadand__construct_billmethods. Refactor this logic into a separate helper method to enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle.Consider creating a helper method like
set_exchange_rate:def set_exchange_rate(self, source_currency_code): exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=source_currency_code) if not exchange_rate: raise Exception(f'Exchange rate not found for currency {source_currency_code}') return exchange_rate['Rate'] if "Rate" in exchange_rate else 1Then update the original methods:
In
purchase_object_payload:- exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=purchase_object.currency) - purchase_object_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1 + purchase_object_payload['ExchangeRate'] = self.set_exchange_rate(purchase_object.currency) purchase_object.exchange_rate = purchase_object_payload['ExchangeRate'] purchase_object.save(update_fields=['exchange_rate'])In
__construct_bill:- exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=fyle_home_currency) - bill_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1 + bill_payload['ExchangeRate'] = self.set_exchange_rate(fyle_home_currency) bill.exchange_rate = bill_payload['ExchangeRate'] bill.save(update_fields=['exchange_rate'])Also applies to: 737-738
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/quickbooks_online/utils.py(1 hunks)
🔇 Additional comments (2)
apps/quickbooks_online/utils.py (2)
735-736: Add error handling for missing exchange rates
If self.connection.exchange_rates.get_by_source returns None, accessing exchange_rate['Rate'] will raise an exception. Ensure that you handle the case where exchange_rate might be None to prevent runtime errors.
Apply this diff to add error handling:
exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=purchase_object.currency)
+ if not exchange_rate:
+ raise Exception(f'Exchange rate not found for currency {purchase_object.currency}')
purchase_object_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1
purchase_object.exchange_rate = purchase_object_payload['ExchangeRate']737-738: Add error handling for missing exchange rates
Similarly, in the __construct_bill method, handle the case where exchange_rate might be None to prevent runtime errors when accessing exchange_rate['Rate'].
Apply this diff to add error handling:
exchange_rate = self.connection.exchange_rates.get_by_source(source_currency_code=fyle_home_currency)
+ if not exchange_rate:
+ raise Exception(f'Exchange rate not found for currency {fyle_home_currency}')
bill_payload['ExchangeRate'] = exchange_rate['Rate'] if "Rate" in exchange_rate else 1
bill.exchange_rate = bill_payload['ExchangeRate']
|
Description
Clickup
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor