Added generator in sdk and modified the calls in api #165
Conversation
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Warning Rate Limit Exceeded@Hrishabh17 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 38 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on refining data handling and pagination across various modules in the Sage 300 and Sage Desktop SDK systems. Key enhancements include the introduction of generators for efficient data streaming and updates to method structures for better data manipulation and API interaction, ensuring smoother operations and data processing. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 24
Out of diff range and nitpick comments (3)
apps/sage300/models.py (2)
Line range hint
1-1: Remove unused imports to clean up the code.- from typing import Dict, ListThe
DictandListimports from thetypingmodule are unused and should be removed to clean up the code.
34-44: Ensure the generator pattern is correctly implemented in thebulk_create_or_updatemethod.Consider simplifying the nested loops for processing categories. This could improve readability and potentially enhance performance by reducing the complexity of the data structure manipulation.
tests/test_sage300/test_utils.py (1)
Line range hint
5-5: The importsage_desktop_sdk.core.schema.read_only.Categoryis unused and should be removed to clean up the code.- from sage_desktop_sdk.core.schema.read_only import Category
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | ||
| """ | ||
| Gets all the objects of a particular type for query type GET calls | ||
| :param url: GET URL of object | ||
| :param object_type: type of object | ||
| :param is_paginated: is paginated | ||
| :return: Generator of objects | ||
| """ | ||
| page_number = 0 | ||
|
|
||
| request_url = '{0}{1}'.format(self.__api_url, url) | ||
| api_headers = { | ||
| 'Cookie': self.__cookie, | ||
| 'Accept': 'application/json' | ||
| } | ||
|
|
||
| while True: | ||
| try: | ||
| if is_paginated: | ||
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | ||
| else: | ||
| response = requests.get(url=request_url, headers=api_headers) | ||
|
|
||
| data = json.loads(response.text) | ||
|
|
||
| if not data: | ||
| break | ||
|
|
||
| yield data | ||
|
|
||
| if is_paginated: | ||
| page_number += 1 | ||
| else: | ||
| break | ||
|
|
||
| except requests.exceptions.HTTPError as err: | ||
| if err.response.status_code == 400: | ||
| raise WrongParamsError('Some of the parameters are wrong', response.text) | ||
|
|
||
| if err.response.status_code == 406: | ||
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | ||
|
|
||
| if err.response.status_code == 404: | ||
| raise NotFoundItemError('Not found item with ID', response.text) | ||
|
|
||
| if err.response.status_code == 500: | ||
| raise InternalServerError('Internal server error', response.text) | ||
|
|
||
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) |
There was a problem hiding this comment.
The implementation of _query_get_all_generator method is generally robust, but consider handling potential infinite loops and API rate limits.
while True:
+ # Ensure that the loop will not run indefinitely
+ if page_number > MAX_PAGE_LIMIT:
+ break
try: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.
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) | |
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| # Ensure that the loop will not run indefinitely | |
| if page_number > MAX_PAGE_LIMIT: | |
| break | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) |
Consider adding logging for better traceability and debugging of the generator function.
while True:
+ logging.debug(f"Fetching page {page_number} from {request_url}")
try: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.
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) | |
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| logging.debug(f"Fetching page {page_number} from {request_url}") | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) |
Ensure proper exception handling for network-related errors which are not caught by the current implementation.
raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text)
+ except requests.exceptions.RequestException as e:
+ logging.error(f"Network error occurred: {e}")
+ raise SageDesktopSDKError('Network error', str(e))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.
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) | |
| def _query_get_all_generator(self, url: str, is_paginated: bool = False) -> Generator[Dict, None, None]: | |
| """ | |
| Gets all the objects of a particular type for query type GET calls | |
| :param url: GET URL of object | |
| :param object_type: type of object | |
| :param is_paginated: is paginated | |
| :return: Generator of objects | |
| """ | |
| page_number = 0 | |
| request_url = '{0}{1}'.format(self.__api_url, url) | |
| api_headers = { | |
| 'Cookie': self.__cookie, | |
| 'Accept': 'application/json' | |
| } | |
| while True: | |
| try: | |
| if is_paginated: | |
| response = requests.get(url=request_url.format(page_number), headers=api_headers) | |
| else: | |
| response = requests.get(url=request_url, headers=api_headers) | |
| data = json.loads(response.text) | |
| if not data: | |
| break | |
| yield data | |
| if is_paginated: | |
| page_number += 1 | |
| else: | |
| break | |
| except requests.exceptions.HTTPError as err: | |
| if err.response.status_code == 400: | |
| raise WrongParamsError('Some of the parameters are wrong', response.text) | |
| if err.response.status_code == 406: | |
| raise NotAcceptableClientError('Forbidden, the user has insufficient privilege', response.text) | |
| if err.response.status_code == 404: | |
| raise NotFoundItemError('Not found item with ID', response.text) | |
| if err.response.status_code == 500: | |
| raise InternalServerError('Internal server error', response.text) | |
| raise SageDesktopSDKError('Error: {0}'.format(err.response.status_code), response.text) | |
| except requests.exceptions.RequestException as e: | |
| logging.error(f"Network error occurred: {e}") | |
| raise SageDesktopSDKError('Network error', str(e)) |
apps/sage300/utils.py
Outdated
| def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names): | ||
| destination_attributes = [] | ||
|
|
||
| detail = {field: getattr(item, field) for field in field_names} | ||
| if item.name: | ||
| destination_attributes.append(self._create_destination_attribute( | ||
| attribute_type, | ||
| display_name, | ||
| " ".join(item.name.split()), | ||
| item.id, | ||
| item.is_active, | ||
| detail | ||
| )) | ||
|
|
||
| return destination_attributes | ||
|
|
There was a problem hiding this comment.
The method _add_to_destination_attributes efficiently constructs destination attributes, but consider handling potential attribute errors when accessing properties of item.
detail = {field: getattr(item, field) for field in field_names}
+ # Handle potential AttributeError if item does not have the field
+ detail = {field: getattr(item, field, None) for field in field_names}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.
| def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names): | |
| destination_attributes = [] | |
| detail = {field: getattr(item, field) for field in field_names} | |
| if item.name: | |
| destination_attributes.append(self._create_destination_attribute( | |
| attribute_type, | |
| display_name, | |
| " ".join(item.name.split()), | |
| item.id, | |
| item.is_active, | |
| detail | |
| )) | |
| return destination_attributes | |
| def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names): | |
| destination_attributes = [] | |
| # Handle potential AttributeError if item does not have the field | |
| detail = {field: getattr(item, field, None) for field in field_names} | |
| if item.name: | |
| destination_attributes.append(self._create_destination_attribute( | |
| attribute_type, | |
| display_name, | |
| " ".join(item.name.split()), | |
| item.id, | |
| item.is_active, | |
| detail | |
| )) | |
| return destination_attributes |
| def _get_attribute_class(self, attribute_type: str): | ||
| """ | ||
| Get the attribute class for the attribute type | ||
| :param attribute_type: Type of the attribute | ||
| :return: Attribute class | ||
| """ | ||
| ATTRIBUTE_CLASS_MAP = { | ||
| 'ACCOUNT': 'Account', | ||
| 'VENDOR': 'Vendor', | ||
| 'JOB': 'Job', | ||
| 'STANDARD_COST_CODE': 'StandardCostCode', | ||
| 'STANDARD_CATEGORY': 'StandardCategory', | ||
| 'COMMITMENT': 'Commitment', | ||
| 'COMMITMENT_ITEM': 'CommitmentItem', | ||
| 'COST_CODE': 'CostCode' | ||
| } | ||
|
|
||
| return ATTRIBUTE_CLASS_MAP[attribute_type] |
There was a problem hiding this comment.
The method _get_attribute_class uses a hard-coded mapping which could be externalized to a configuration file or database for easier maintenance.
Consider externalizing the ATTRIBUTE_CLASS_MAP to a configuration file or database to facilitate easier updates and maintenance without needing to modify the code.
| endpoint = Accounts.GET_ACCOUNTS + '?page={0}' | ||
| if version: | ||
| # Append the version query parameter if provided | ||
| query_params = '?version={0}'.format(version) | ||
| endpoint = Accounts.GET_ACCOUNTS + query_params | ||
| else: | ||
| endpoint = Accounts.GET_ACCOUNTS | ||
| query_params = f'&version={version}' | ||
| endpoint += query_params | ||
|
|
||
| # Query the API to get all accounts | ||
| accounts = self._query_get_all(endpoint) | ||
| for account in accounts: | ||
| yield Account.from_dict(account) | ||
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield accounts |
There was a problem hiding this comment.
Ensure consistent use of string formatting for query parameters.
- endpoint = Accounts.GET_ACCOUNTS + '?page={0}'
+ endpoint = Accounts.GET_ACCOUNTS + f'?page={0}'This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.
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.
| endpoint = Accounts.GET_ACCOUNTS + '?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = '?version={0}'.format(version) | |
| endpoint = Accounts.GET_ACCOUNTS + query_params | |
| else: | |
| endpoint = Accounts.GET_ACCOUNTS | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| # Query the API to get all accounts | |
| accounts = self._query_get_all(endpoint) | |
| for account in accounts: | |
| yield Account.from_dict(account) | |
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield accounts | |
| endpoint = Accounts.GET_ACCOUNTS + f'?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| # Query the API to get all accounts | |
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield accounts |
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield accounts |
There was a problem hiding this comment.
Yielding a generator object directly may not be the intended behavior.
- yield accounts
+ for account in accounts:
+ yield accountDirectly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each account individually.
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.
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield accounts | |
| accounts = self._query_get_all_generator(endpoint, is_paginated=True) | |
| for account in accounts: | |
| yield account |
| jobs = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield jobs |
There was a problem hiding this comment.
Yielding a generator object directly may not be the intended behavior.
- yield jobs
+ for job in jobs:
+ yield jobDirectly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each job individually.
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.
| jobs = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield jobs | |
| jobs = self._query_get_all_generator(endpoint, is_paginated=True) | |
| for job in jobs: | |
| yield job |
| endpoint = Jobs.GET_COST_CODES + '?page={0}' | ||
| if version: | ||
| # Append the version query parameter if provided | ||
| query_params = '?version={0}'.format(version) | ||
| endpoint = Jobs.GET_COST_CODES + query_params | ||
| else: | ||
| endpoint = Jobs.GET_COST_CODES | ||
|
|
||
| # Query the API to get all standard cost codes | ||
| costcodes = self._query_get_all(endpoint) | ||
| query_params = f'&version={version}' | ||
| endpoint += query_params | ||
|
|
||
| for costcode in costcodes: | ||
| # Convert each cost code dictionary to a StandardCostCode object and yield it | ||
| yield StandardCostCode.from_dict(costcode) | ||
| # Query the API to get all jobs | ||
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield cost_codes |
There was a problem hiding this comment.
Ensure consistent use of string formatting for query parameters.
- endpoint = Jobs.GET_COST_CODES + '?page={0}'
+ endpoint = Jobs.GET_COST_CODES + f'?page={0}'This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.
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.
| endpoint = Jobs.GET_COST_CODES + '?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = '?version={0}'.format(version) | |
| endpoint = Jobs.GET_COST_CODES + query_params | |
| else: | |
| endpoint = Jobs.GET_COST_CODES | |
| # Query the API to get all standard cost codes | |
| costcodes = self._query_get_all(endpoint) | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| for costcode in costcodes: | |
| # Convert each cost code dictionary to a StandardCostCode object and yield it | |
| yield StandardCostCode.from_dict(costcode) | |
| # Query the API to get all jobs | |
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield cost_codes | |
| endpoint = Jobs.GET_COST_CODES + f'?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| # Query the API to get all jobs | |
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield cost_codes |
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield cost_codes |
There was a problem hiding this comment.
Yielding a generator object directly may not be the intended behavior.
- yield cost_codes
+ for cost_code in cost_codes:
+ yield cost_codeDirectly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each cost code individually.
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.
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield cost_codes | |
| cost_codes = self._query_get_all_generator(endpoint, is_paginated=True) | |
| for cost_code in cost_codes: | |
| yield cost_code |
| endpoint = Jobs.GET_CATEGORIES + '?page={0}' | ||
| if version: | ||
| # Append the version query parameter if provided | ||
| query_params = '?version={0}'.format(version) | ||
| endpoint = Jobs.GET_CATEGORIES + query_params | ||
| else: | ||
| endpoint = Jobs.GET_CATEGORIES | ||
| query_params = f'&version={version}' | ||
| endpoint += query_params | ||
|
|
||
| # Query the API to get all standard categories | ||
| categories = self._query_get_all(endpoint) | ||
|
|
||
| for category in categories: | ||
| # Convert each category dictionary to a StandardCategory object and yield it | ||
| yield StandardCategory.from_dict(category) | ||
| # Query the API to get all jobs | ||
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield categories |
There was a problem hiding this comment.
Ensure consistent use of string formatting for query parameters.
- endpoint = Jobs.GET_CATEGORIES + '?page={0}'
+ endpoint = Jobs.GET_CATEGORIES + f'?page={0}'This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.
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.
| endpoint = Jobs.GET_CATEGORIES + '?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = '?version={0}'.format(version) | |
| endpoint = Jobs.GET_CATEGORIES + query_params | |
| else: | |
| endpoint = Jobs.GET_CATEGORIES | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| # Query the API to get all standard categories | |
| categories = self._query_get_all(endpoint) | |
| for category in categories: | |
| # Convert each category dictionary to a StandardCategory object and yield it | |
| yield StandardCategory.from_dict(category) | |
| # Query the API to get all jobs | |
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield categories | |
| endpoint = Jobs.GET_CATEGORIES + f'?page={0}' | |
| if version: | |
| # Append the version query parameter if provided | |
| query_params = f'&version={version}' | |
| endpoint += query_params | |
| # Query the API to get all jobs | |
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield categories |
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | ||
| yield categories |
There was a problem hiding this comment.
Yielding a generator object directly may not be the intended behavior.
- yield categories
+ for category in categories:
+ yield categoryDirectly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each category individually.
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.
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | |
| yield categories | |
| categories = self._query_get_all_generator(endpoint, is_paginated=True) | |
| for category in categories: | |
| yield category |
|
apps/sage300/models.py
Outdated
| @@ -30,14 +30,16 @@ class Meta: | |||
| db_table = 'cost_category' | |||
|
|
|||
| @staticmethod | |||
| def bulk_create_or_update(categories_generator: List[Dict], workspace_id: int): | |||
| def bulk_create_or_update(categories_generator, workspace_id: int): | |||
There was a problem hiding this comment.
we should not pass generator, we should pass current page items here. Line 41 ka category value for example
| )) | ||
| if is_gen: | ||
| for data in data_gen: | ||
| for items in data: |
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/sage300/models.py (3 hunks)
- apps/sage300/utils.py (5 hunks)
- tests/test_sage300/test_models.py (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- apps/sage300/models.py
- apps/sage300/utils.py
- tests/test_sage300/test_models.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/sage300/utils.py (5 hunks)
- sage_desktop_sdk/core/client.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/sage300/utils.py
- sage_desktop_sdk/core/client.py
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sage_desktop_sdk/core/client.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sage_desktop_sdk/core/client.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 94.80% 94.61% -0.20%
==========================================
Files 99 100 +1
Lines 5235 5272 +37
==========================================
+ Hits 4963 4988 +25
- Misses 272 284 +12 ☔ View full report in Codecov by Sentry. |
apps/sage300/utils.py
Outdated
|
|
||
| return ATTRIBUTE_CLASS_MAP[attribute_type] | ||
|
|
||
| def _sync_data(self, data_gen, attribute_type, display_name, workspace_id, field_names, is_gen: bool = True): |
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
|
|
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation