Tests invoice #8
Conversation
There was a problem hiding this comment.
The tests are okay. Remember to also test the creation of Integration Request (Integration Request doctype) records, and also confirm whether details such as headers, the JSON payloads, etc are captured therein. You can also check to ensure the status of the integration request updates accordingly from Queued to either Failed/Success. All requests create an integration request.
Also, ensure that you apply the linting rules specified in the .flake8 file in the repo's root to ensure the code's quality. I'm seeing plenty of unnecessary whitespace, and imports not organised according to the project's linting rules. Ensure you configure pre-commit hooks for your local setup to check for these issues as you make commits.
| @@ -258,7 +271,14 @@ def get_stock_entry_movement_items_details( | |||
| ) -> list[dict]: | |||
| items_list = [] | |||
|
|
|||
| for item in records: | |||
| for i in records: | |||
There was a problem hiding this comment.
This exception handling isn't needed since the properties being accessed of the item will always be bound.
|
|
||
| try: | ||
| server_url = get_server_url(company_name, record.branch) | ||
| except Exception as e: |
There was a problem hiding this comment.
Avoid catching the general Exception. Capture specific exceptions that you have a strategy to handle
| current_item = list( | ||
| filter(lambda item: item["itemNm"] == doc.item_code, items_list) | ||
| ) | ||
| current_item = [item for item in items_list if item["itemNm"] in [i.item_code for i in doc.items]] |
There was a problem hiding this comment.
You've replaced the loop with one that runs in Quadratic time. This operation will be less efficient
| headers = build_headers(company_name, record.branch) | ||
| except Exception as e: | ||
| frappe.log_error(message=str(e), title="Header Building Error") | ||
| headers = { |
There was a problem hiding this comment.
Building the headers is strictly handled using the build_headers() function. Avoid hard-coding header information as it is critical for the communication with the etims servers.
| @@ -35,7 +35,7 @@ def on_update(doc: Document, method: str | None = None) -> None: | |||
| "regTyCd": "M", | |||
| "custTin": None, | |||
| "custNm": None, | |||
| "custBhfId": get_warehouse_branch_id(doc.warehouse) or None, | |||
| "custBhfId": get_warehouse_branch_id(doc.get('warehouse')) or None, | |||
There was a problem hiding this comment.
This is the same difference. Use the get method if you anticipate the key being accessed can be None, and you can provide a default value. In this case, the warehouse property will always be bound, so no reason for the get method.
| # TODO: Handle discounts properly | ||
| "totDcAmt": 0, | ||
| "taxTyCd": fetched_item.custom_taxation_type_code or "B", | ||
| "taxTyCd": fetched_item.custom_taxation_type or "B", |
There was a problem hiding this comment.
It's important to select the custom_taxation_type_code field as this field will always hold the actual code value documented in the etims api. Revert this.
| @@ -297,7 +313,14 @@ def get_stock_recon_movement_items_details( | |||
| items_list = [] | |||
| # current_qty | |||
|
|
|||
| for item in records: | |||
| for i in records: | |||
| try: | |||
There was a problem hiding this comment.
Once again, the exception handling isn't needed since the properties will be bound at runtime. The properties, valuation_rate and quantity_difference are also provided in the item/i object when iteration begins negating the need to make a db call using get_doc(). The records object already has all the relevant information.
| @@ -339,7 +354,14 @@ def get_purchase_docs_items_details( | |||
| ) -> list[dict]: | |||
| items_list = [] | |||
|
|
|||
| for item in items: | |||
| for i in items: | |||
There was a problem hiding this comment.
See previous comment on the same issue
| return branch_id.custom_branch | ||
| if branch_id: | ||
| return branch_id.custom_branch | ||
| except: |
There was a problem hiding this comment.
always avoid catching general exceptions. Only catch errors/exception you will handle. If something should fail, let if fail.
Noted. I will work on it |
No description provided.