feat: Add support for Contract Lines API #101
Conversation
WalkthroughThis pull request introduces several enhancements to the Sage Intacct SDK. A new class, Changes
Possibly related PRs
Suggested reviewers
Poem
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: 3
🧹 Outside diff range and nitpick comments (7)
sageintacctsdk/apis/contract_lines.py (3)
1-3: Enhance module documentationThe current docstring is too brief. Consider expanding it to include:
- Purpose and functionality of the Contract Lines API
- Link to the Intacct API documentation
- Basic usage examples
""" -Sage Intacct contract lines +Sage Intacct Contract Lines API implementation + +This module provides integration with the Sage Intacct Contract Lines API, +allowing management of contract line items. For detailed API specifications, see: +https://developer.intacct.com/api/contracts-rev-mgmt/contract-lines/ + +Example: + contract_lines = ContractLines() + contract_lines.get_all() # Retrieve all contract lines """
9-9: Improve class documentationThe class docstring should provide more details about:
- Available methods and their usage
- Required parameters for different operations
- Return value formats
- Possible exceptions
1-11: Consider adding supporting structuresTo build a robust Contract Lines API implementation, consider adding:
- Data Models/TypedDicts for contract line properties
- Custom exceptions for Contract Lines specific errors
- Request/Response validation schemas
- Constants for contract line statuses and other enums
- Helper methods for common operations
This will improve type safety, error handling, and maintainability.
sageintacctsdk/sageintacctsdk.py (2)
4-4: Consider breaking long import line for better readabilityThe import line is getting quite long. Consider splitting it into multiple lines with logical grouping of related APIs.
-from .apis import ApiBase, Contacts, Contracts, ContractLines, Locations, Employees, Accounts, ExpenseTypes, Attachments, ExpenseReports,\ - Vendors, Bills, Projects, Departments, ChargeCardAccounts, ChargeCardTransactions, Customers, Items,\ - APPayments, Reimbursements, CheckingAccounts, SavingsAccounts, Tasks, ExpensePaymentTypes, Dimensions,\ - DimensionValues, LocationEntities, ARInvoices, ARPayments, TaxDetails, GLDetail, Classes, JournalEntries,\ - RevRecSchedules, RevRecScheduleEntries, CostTypes, OrderEntryTransactions, Allocations, AllocationEntry +from .apis import ( + # Core APIs + ApiBase, Contacts, Contracts, ContractLines, + # Financial APIs + Accounts, Bills, APPayments, ARInvoices, ARPayments, + # Employee & Expense APIs + Employees, ExpenseTypes, ExpenseReports, ExpensePaymentTypes, + # ... continue with logical groupings +)
Line range hint
4-222: LGTM! Clean and consistent integrationThe ContractLines API integration follows the established architectural patterns:
- Proper import declaration
- Consistent instance initialization
- Complete integration with all necessary update methods
- Maintains backward compatibility
The implementation aligns well with the PR objective of adding Contract Lines API support.
sageintacctsdk/apis/constants.py (1)
90-136: LGTM! Consider minor improvements for maintainability.The CONTRACTDETAIL mapping is well-structured and comprehensive. Consider these maintainability improvements:
- Add comments to group related fields (e.g., // Identification fields, // Billing fields)
- Add WHENCREATED and WHENMODIFIED fields for consistency with other dimensions
sageintacctsdk/apis/api_base.py (1)
207-213: Use appropriate logging levels for failure casesWhen any of the statuses (
control_status,auth_status,result_status) indicate'failure', the response is logged at theinfolevel. To better signal issues and aid in troubleshooting, consider logging at theerrorlevel instead.Apply this diff to adjust the logging level:
-if control_status == 'failure' or auth_status == 'failure' or result_status == 'failure': - logger.info('Response for post request: %s', raw_response.text) +if control_status == 'failure' or auth_status == 'failure' or result_status == 'failure': + logger.error('Response for post request: %s', raw_response.text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
sageintacctsdk/apis/__init__.py(2 hunks)sageintacctsdk/apis/api_base.py(2 hunks)sageintacctsdk/apis/constants.py(1 hunks)sageintacctsdk/apis/contract_lines.py(1 hunks)sageintacctsdk/sageintacctsdk.py(6 hunks)
🔇 Additional comments (6)
sageintacctsdk/apis/contract_lines.py (1)
10-11: Consider adding parameter validation in constructor
While inheriting from ApiBase, it might be worth adding validation for any Contract Lines specific initialization requirements.
sageintacctsdk/apis/__init__.py (2)
18-18: LGTM! Import statement follows established patterns
The import statement for ContractLines is properly placed and follows the alphabetical ordering convention used in the file.
48-48: LGTM! Export list updated correctly
The ContractLines class is properly added to the all list, maintaining alphabetical order and ensuring the new API is accessible to SDK users.
sageintacctsdk/sageintacctsdk.py (2)
40-40: LGTM! Proper initialization and placement
The ContractLines instance is correctly initialized and logically placed next to the Contracts API initialization.
88-88: LGTM! Verify integration testing
The ContractLines API is properly integrated into all necessary update methods. Please ensure that integration tests cover these new update method calls.
Also applies to: 132-132, 178-178, 222-222
sageintacctsdk/apis/constants.py (1)
90-136: Verify field completeness against Intacct API documentation.
Please double-check that all required fields from the Contract Lines API documentation (https://developer.intacct.com/api/contracts-rev-mgmt/contract-lines/) are included in the mapping.
| class ContractLines(ApiBase): | ||
| """Class for Contract Lines APIs.""" | ||
| def __init__(self): | ||
| ApiBase.__init__(self, dimension='CONTRACTDETAIL') |
There was a problem hiding this comment.
Expand ContractLines class implementation
The class needs essential CRUD operations to be useful for Contract Lines API integration.
Based on the Intacct API documentation, the class should implement methods for:
- Creating contract lines
- Reading contract line details
- Updating contract lines
- Deleting contract lines
- Listing/querying contract lines
Would you like me to help generate the implementation for these methods?
|
|
||
|
|
||
| if control_status == 'failure' or auth_status == 'failure' or result_status == 'failure': | ||
| logger.info('Response for post request: %s', raw_response.text) | ||
| else: | ||
| logger.debug('Response for post request: %s', raw_response.text) | ||
|
|
There was a problem hiding this comment.
Avoid logging sensitive data at 'info' level
Logging the entire raw_response.text at the info level when a failure occurs may expose sensitive information in the logs. Consider logging only essential details or sanitizing the response before logging to prevent potential exposure of sensitive data.
| @@ -255,7 +255,7 @@ def __post_request(self, dict_body: dict, api_url: str): | |||
| if 'result' in parsed_response: | |||
| if 'errormessage' in parsed_response['result']: | |||
| parsed_response = parsed_response['result']['errormessage'] | |||
|
|
|||
There was a problem hiding this comment.
Correct the key used when checking for 'errormessage'
In the condition if 'errormessage' in parsed_response:, the code checks for 'errormessage' directly in parsed_response. However, 'errormessage' is likely within parsed_response['result']. Adjust the condition to properly access the error message.
Apply this diff to fix the issue:
-if 'errormessage' in parsed_response:
+if 'errormessage' in parsed_response['result']:Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sageintacctsdk/apis/constants.py (1)
88-133: Consider adding field documentation.While the field names are self-explanatory for those familiar with Intacct, consider adding docstring or comments explaining the purpose and expected values of key fields like BILLINGMETHOD, BILLINGOPTIONS, etc., to improve maintainability.
Example addition:
"CONTRACTDETAIL": [ + # Unique identifier for the contract line "RECORDNO", + # Parent contract identifier "CONTRACTID", + # Item being contracted "ITEMID", + # Billing configuration "BILLINGMETHOD", # Values: "Fixed", "Usage", etc. "BILLINGOPTIONS", # Values: "Auto", "Manual", etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sageintacctsdk/apis/constants.py(1 hunks)
🔇 Additional comments (2)
sageintacctsdk/apis/constants.py (2)
88-133: LGTM! The CONTRACTDETAIL mapping is well-structured and complete.
The new CONTRACTDETAIL dimension mapping includes all essential fields from the Intacct Contract Lines API, following the established pattern in the codebase.
1-790: Verify field completeness against the API documentation.
Let's ensure all fields from the Intacct Contract Lines API are covered in the CONTRACTDETAIL mapping.
✅ Verification successful
The field mappings for CONTRACT and CONTRACTDETAIL dimensions appear to be self-contained
Based on the codebase analysis, the field mappings for both CONTRACT and CONTRACTDETAIL dimensions are defined within the constants.py file and appear to be comprehensive, covering all essential fields including record identifiers, dates, status fields, and relationship fields. Without any external API documentation to compare against, and no evidence of missing fields from the actual usage in the codebase, we can consider these mappings to be complete as they stand.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any potential field references in markdown documentation
# Expected results: Find any field names mentioned in documentation that might be missing from our mapping
# Search for field names in markdown files
rg -i -t md 'contract.*field|field.*contract'
# Search for RECORDNO pattern to find other potential dimension files that might contain relevant field references
rg 'RECORDNO.*CONTRACTDETAIL|CONTRACTDETAIL.*RECORDNO'
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search to find any contract-related API documentation or field definitions
# Search for contract-related files
fd -t f -e md -e rst -e txt contract
# Search for any files mentioning contract fields or dimensions
rg -l "contract.*dimension|contract.*field|contractdetail"
# Search for any Python files that might define contract-related schemas or models
fd -e py | xargs rg -l "contract.*schema|contract.*field|contractdetail"
Length of output: 174
Script:
#!/bin/bash
# Let's analyze the actual usage of contract fields in the codebase
# Search for contract-related API calls or field usage
ast-grep --pattern 'dimensions_fields_mapping["CONTRACTDETAIL"]'
# Look for any contract-related class or function definitions
ast-grep --pattern 'class $_(Contract|ContractDetail)'
# Search for any direct string references to contract fields
rg '"(CONTRACTDETAIL|CONTRACT)"' -A 5 -B 5
Length of output: 1345
|
@godhand1607 Thanks for your contribution! Can check it out here: https://pypi.org/project/sageintacctsdk/ |
https://developer.intacct.com/api/contracts-rev-mgmt/contract-lines/
Summary by CodeRabbit
New Features
ContractLinesclass for managing contract line items within the SDK.Bug Fixes
Documentation
ContractLinesclass to describe its purpose.Chores