fix: get the key safely #592
Conversation
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request introduces modifications to the Sage Intacct models, focusing on enhancing employee and charge card account retrieval. The changes include updating the Changes
Sequence DiagramsequenceDiagram
participant Expense
participant GeneralMapping
participant Function as get_ccc_account_id()
Expense->>Function: Provide expense details
GeneralMapping->>Function: Provide general mappings
alt Corporate Card Mapping Exists
Function-->>Expense: Return Corporate Card Account ID
else Employee Mapping Available
Function-->>Expense: Return Employee Card Account ID
else No Mapping Found
Function-->>Expense: Return Default Charge Card ID
end
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (2)
apps/sage_intacct/models.py (2)
526-529: Good change, but let's make it even safer!While using
.get()is safer than direct dictionary access, there's still room for improvement:
- Line 527 still uses direct dictionary access
employee.detail[object_type]which could raise KeyError- The function implicitly returns None when conditions aren't met
Consider this safer implementation:
- if employee and employee.detail.get(object_type): - default_employee_object = employee.detail[object_type] - return default_employee_object + if employee and employee.detail.get(object_type): + return employee.detail.get(object_type) + return None
Line range hint
530-545: Well-structured function with clear fallback logic!The implementation is good, but could benefit from better documentation and type safety.
Consider these improvements:
-def get_ccc_account_id(general_mappings: GeneralMapping, expense: Expense, description: str): +def get_ccc_account_id(general_mappings: GeneralMapping, expense: Expense, description: Dict[str, Any]) -> str: + """Get the charge card account ID based on various mapping rules. + + Args: + general_mappings: General mappings configuration + expense: The expense object + description: Dictionary containing expense group description + + Returns: + str: The charge card account ID from either corporate card mapping, + employee mapping, or default charge card ID + """ + if not general_mappings: + raise ValueError("General mappings cannot be None") + card_mapping = Mapping.objects.filter( source_type='CORPORATE_CARD', destination_type='CHARGE_CARD_NUMBER', source__source_id=expense.corporate_card_id, workspace_id=general_mappings.workspace ).first()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/sage_intacct/models.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
|
Description
fix detination attribute get detail dict value safely
Clickup
https://app.clickup.com/
Summary by CodeRabbit
Bug Fixes
New Features