refactor: re-structure the Sketch Order Form

[DHINESH00]

PR Type

Enhancement

---

Description

• Refactored Sketch Order Form class with improved code organization

• Extracted complex date calculation logic into separate helper functions

• Simplified method implementations through better separation of concerns

• Improved code readability with clearer variable names and structure

---

Diagram Walkthrough

flowchart LR
  SOF["Sketch Order Form<br/>Document"] -->|validate| VAL["validate_category<br/>_subcategory"]
  SOF -->|on_submit| HS["handle_submit"]
  HS -->|creates| CSO["create_sketch_order"]
  HS -->|creates| CPO["create_purchase_order"]
  CSO -->|applies dates| APD["apply_parent_dates"]
  CSO -->|applies dates| AOC["apply_order_criteria_dates"]
  AOC -->|calculates| CSD["calculate_sketch_delivery"]
  AOC -->|calculates| CID["calculate_ibm_delivery"]
  SOF -->|on_update| UDD["update_delivery_dates"]
  SOF -->|on_cancel| CLS["cancel_linked_sketch_orders"]

Loading

File Walkthrough

Relevant files

Enhancement

sketch_order_form.py Refactor with extracted helper functions and improved organization jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py Reorganized class methods with improved naming and structure Extracted date calculation logic into dedicated helper functions (calculate_sketch_delivery, calculate_ibm_delivery, apply_parent_dates, apply_order_criteria_dates) Refactored create_sketch_order function to use extracted helpers and improve readability Simplified create_purchase_order function with cleaner initialization and field assignment Extracted field copying logic into copy_parent_fields function for better maintainability Improved get_customer_orderType function formatting and clarity Removed unused delete_auto_created_sketch_order function and commented-out code Fixed typo in method name from validate_category_subcaegory to validate_category_subcategory +233/-245

---

---

🛠️ Relevant configurations:

These are the relevant configurations for this tool:

[config]

is_auto_command: True
is_new_pr: True
model_reasoning: vertex_ai/gemini-2.5-pro
model: gpt-5.2-2025-12-11
model_turbo: anthropic/claude-haiku-4-5-20251001
fallback_models: ['anthropic/claude-sonnet-4-5-20250929', 'bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0']
second_model_for_exhaustive_mode: o4-mini
git_provider: github
publish_output: True
publish_output_no_suggestions: True
publish_output_progress: True
verbosity_level: 0
use_extra_bad_extensions: False
publish_logs: False
debug_mode: False
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
use_global_wiki_settings_file: False
disable_auto_feedback: False
ai_timeout: 150
response_language: en-US
clone_repo_instead_of_fetch: True
always_clone: False
add_repo_metadata: True
clone_repo_time_limit: 300
publish_inline_comments_fallback_batch_size: 5
publish_inline_comments_fallback_sleep_time: 2
max_model_tokens: 32000
custom_model_max_tokens: -1
patch_extension_skip_types: ['.md', '.txt']
extra_allowed_extensions: []
allow_dynamic_context: True
allow_forward_dynamic_context: True
max_extra_lines_before_dynamic_context: 12
patch_extra_lines_before: 5
patch_extra_lines_after: 1
ai_handler: litellm
cli_mode: False
TRIAL_GIT_ORG_MAX_INVOKES_PER_MONTH: 75
TRIAL_RATIO_CLOSE_TO_LIMIT: 0.8
invite_only_mode: False
enable_request_access_msg_on_new_pr: False
check_also_invites_field: False
calculate_context: True
disable_checkboxes: False
output_relevant_configurations: True
large_patch_policy: clip
seed: -1
temperature: 0.2
allow_dynamic_context_ab_testing: False
choose_dynamic_context_ab_testing_ratio: 0.5
ignore_pr_title: ['^\\[Auto\\]', '^Auto', '^\\[Bump\\]']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: ['do-not-merge', 'skip-qodo']
ignore_ticket_labels: []
allow_only_specific_folders: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
enable_ai_metadata: True
present_reasoning: True
max_tickets: 10
max_tickets_chars: 8000
prevent_any_approval: False
enable_comment_approval: False
enable_auto_approval: False
auto_approve_for_low_review_effort: -1
auto_approve_for_no_suggestions: False
ensure_ticket_compliance: False
new_diff_format: True
new_diff_format_add_external_references: True
enable_custom_labels: False

[pr_description]

final_update_message: False
publish_labels: False
add_original_user_description: True
generate_ai_title: False
extra_instructions: Highlight any database schema changes (Doctype/child table modifications), new background jobs (frappe.enqueue / scheduler events),
and any changes that could increase DB queries or write amplification.

enable_pr_type: True
enable_help_text: False
enable_help_comment: False
bring_latest_tag: False
enable_pr_diagram: True
publish_description_as_comment: False
publish_description_as_comment_persistent: True
enable_semantic_files_types: True
collapsible_file_list: adaptive
collapsible_file_list_threshold: 8
inline_file_summary: False
use_description_markers: False
include_generated_by_header: True
enable_large_pr_handling: True
max_ai_calls: 4
auto_create_ticket: False

[qodo-free-for-open-source-projects]

PR Reviewer Guide 🔍

(Review updated until commit 5ca9132)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🏅 Score: 72

🧪 No relevant tests

✅ No TODO sections

🔒 No security concerns identified

🔀 No multiple PR themes

⚡ Recommended focus areas for review N+1 Query Pattern Loop at line 82 calls make_sketch_order which internally calls get_mapped_doc and doc.save() for each row in doc.order_details. This creates 1 + N database operations (N saves + potential queries inside mapping). Consider batching the saves or using frappe.db.bulk_insert if possible, or at minimum move the frappe.msgprint outside to reduce overhead. order_name = make_sketch_order( "Sketch Order Form Detail", row.name, doc, order_criteria, department_shifts ) created_orders.append(get_link_to_form("Sketch Order", order_name)) if created_orders: Heavy Write in Loop Line 182 calls doc.save() inside the loop for each order detail row. This triggers full document validation, hooks, and controller logic for every iteration. If only creating records without complex validation, consider using frappe.get_doc().insert() with ignore_permissions=True or batch insert operations to avoid N controller executions. doc.save() return doc.name Inefficient Bulk Update Lines 48-53 use frappe.db.set_value with a list filter to update multiple Sketch Orders. While better than a loop, this still triggers one query per matched document internally. For large datasets, verify if this causes performance issues. Consider using frappe.db.sql with UPDATE...WHERE IN for true bulk updates if permission checks are not critical here. frappe.db.set_value( "Sketch Order", {"name": ["in", sketch_order_names]}, "update_delivery_date", self.updated_delivery_date, )

---

🛠️ Relevant configurations:

These are the relevant configurations for this tool:

[config]

enable_ai_metadata: False
model_reasoning: vertex_ai/gemini-2.5-pro
model: gpt-5.2-2025-12-11
model_turbo: anthropic/claude-haiku-4-5-20251001
fallback_models: ['anthropic/claude-sonnet-4-5-20250929', 'bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0']
second_model_for_exhaustive_mode: o4-mini
git_provider: github
publish_output: True
publish_output_no_suggestions: True
publish_output_progress: True
verbosity_level: 0
publish_logs: False
debug_mode: False
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
use_global_wiki_settings_file: False
disable_auto_feedback: False
ai_timeout: 150
response_language: en-US
clone_repo_instead_of_fetch: True
always_clone: False
add_repo_metadata: True
clone_repo_time_limit: 300
publish_inline_comments_fallback_batch_size: 5
publish_inline_comments_fallback_sleep_time: 2
max_model_tokens: 32000
custom_model_max_tokens: -1
patch_extension_skip_types: ['.md', '.txt']
extra_allowed_extensions: []
allow_dynamic_context: True
allow_forward_dynamic_context: True
max_extra_lines_before_dynamic_context: 12
patch_extra_lines_before: 5
patch_extra_lines_after: 1
ai_handler: litellm
cli_mode: False
TRIAL_GIT_ORG_MAX_INVOKES_PER_MONTH: 75
TRIAL_RATIO_CLOSE_TO_LIMIT: 0.8
invite_only_mode: False
enable_request_access_msg_on_new_pr: False
check_also_invites_field: False
calculate_context: True
disable_checkboxes: False
output_relevant_configurations: True
large_patch_policy: clip
seed: -1
temperature: 0.2
allow_dynamic_context_ab_testing: False
choose_dynamic_context_ab_testing_ratio: 0.5
ignore_pr_title: ['^\\[Auto\\]', '^Auto', '^\\[Bump\\]']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: ['do-not-merge', 'skip-qodo']
ignore_ticket_labels: []
allow_only_specific_folders: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
is_auto_command: False
is_new_pr: False
present_reasoning: True
max_tickets: 10
max_tickets_chars: 8000
prevent_any_approval: False
enable_comment_approval: False
enable_auto_approval: False
auto_approve_for_low_review_effort: -1
auto_approve_for_no_suggestions: False
ensure_ticket_compliance: False
new_diff_format: True
new_diff_format_add_external_references: True
enable_custom_labels: False

[pr_reviewer]

require_score_review: True
require_tests_review: True
require_estimate_effort_to_review: True
require_can_be_split_review: True
require_security_review: True
require_todo_scan: True
require_ticket_analysis_review: True
require_ticket_labels: False
require_no_ticket_labels: False
check_pr_additional_content: False
persistent_comment: True
extra_instructions: You are a Senior Technical Architect specializing in the Frappe Framework, Python, and MariaDB optimization.
Be concise but strict about performance and security. Provide code snippets (or pseudocode) for fixes.

========================
1) 🚨 FRAPPE PERFORMANCE KILLERS (High Priority)
========================
A) N+1 Query Detection (must flag):
- Flag ANY loop that contains `frappe.get_doc`, `frappe.db.get_value`, `frappe.db.get_all/get_list`, or `frappe.db.sql`.
- Explain the query multiplier (e.g., “1 + N queries”) and propose a bulk-fetch refactor:
  - pre-fetch with `frappe.get_all/get_list` (with fields + filters) or one SQL query with IN,
  - build a dict/map keyed by name, then iterate without DB calls.

B) Heavy Writes / Controller chain (must flag):
- If `doc.save()` is used to update just 1–2 fields, aggressively suggest replacing with:
  - `frappe.db.set_value(doctype, name, field, value)`
  - or `frappe.db.set_value(doctype, name, {field1: v1, field2: v2})`
- Explain it avoids triggering heavy validations / hooks / controller logic unnecessarily.
- Exception: if controller validations are required for correctness, require justification.

C) ORM abuse / wrong API choice:
- Warn against `frappe.db.sql()` if the same can be achieved by permission-safe ORM (`frappe.get_list/get_all`)
  unless it is a complex join/aggregation needing SQL.
- Also warn when `frappe.db.*` is used in user-facing endpoints where permission checks matter.

D) Redundant commits (must flag):
- `frappe.db.commit()` inside loops is forbidden unless there is a carefully justified transactional reason.
- Suggest moving commit outside loop, batching, or using background jobs.

E) Unbounded reads:
- Flag `get_all/get_list` without strong filters or without pagination on potentially large tables.
- Recommend adding filters, indexed ordering, `limit/page_length`, or background processing.

F) Hot hooks:
- Flag heavy work (bulk queries, file IO, network calls) inside validate/on_update/after_insert hooks.
- Recommend `frappe.enqueue` and async jobs.

========================
2) 🛡️ SECURITY & SQL SAFETY (must be strict)
========================
A) SQL injection:
- Strictly forbid f-strings / concatenation inside `frappe.db.sql()`.
- REQUIRE parameter passing: `frappe.db.sql("... %s", (val,))`.

B) Whitelisting:
- If functions are intended to be called from client (JS / REST), ensure `@frappe.whitelist()` exists.
- Confirm sensitive functions are NOT accidentally exposed.

C) Permission checks:
- For new `get_list/get_all`/SQL calls, ensure strict permissions are not bypassed.
- If `ignore_permissions=True` or `frappe.db.*` is used, REQUIRE explicit justification + risk assessment.

D) Unsafe patterns:
- Flag `eval/exec`, insecure deserialization, and unsafe file path usage.

========================
3) 🐍 PYTHONIC & LOGIC OPTIMIZATIONS
========================
A) Mutable defaults (must flag):
- `def f(x=[])` / `def f(x={})` etc. Require `None` + initialization.

B) Data structure choices:
- If code does `if item in big_list`, suggest `set(big_list)` for O(1) lookups.

C) Inefficient loops:
- Reduce nested loops; pull invariants out; memoize expensive computations.

D) List building:
- Suggest list comprehensions ONLY when readability remains good.
- Avoid creating large intermediate lists when generators/chunking is better.

E) String + datetime churn:
- Flag repeated parsing/formatting/conversions inside loops.

========================
4) 📉 MARIADB / DATABASE OPTIMIZATION CHECKS
========================
A) Wildcards / over-fetching:
- Warn against `SELECT *`. Require selecting specific columns.

B) Indexing:
- If a field is used in WHERE / JOIN / ORDER BY patterns frequently, ask if it needs an index.
- If appropriate, suggest composite index order aligned with query filters (e.g., company + docstatus + posting_date).

C) Sorting + limiting:
- Flag ORDER BY on large sets without indexed order; recommend index or reduce rows first.

D) Aggregations:
- If Python loops compute SUM/COUNT over many rows, suggest SQL aggregation where safe.

========================
5) OUTPUT REQUIREMENTS
========================
- Provide concrete refactor steps and code snippets.
- Quantify impact when possible: “reduces from N queries to 1”.
- If table sizes/indexes are unknown, recommend checking EXPLAIN and adding indexes if confirmed.

final_update_message: False
enable_review_labels_security: True
enable_review_labels_effort: True
enable_help_text: False
num_max_findings: 8

[DHINESH00]

/review

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Latest suggestions up to b87a7e8

Category	Suggestion	Impact
Security	Add permission check to endpoint Add a permission check using frappe.has_permission to the get_customer_orderType whitelisted method to ensure only authorized users can access customer data. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [246-254] @frappe.whitelist() def get_customer_orderType(customer_code): + frappe.has_permission("Customer", "read", throw=True) + OrderType = DocType("Order Type") return ( frappe.qb.from_(OrderType) .select(OrderType.order_type) .where(OrderType.parent == customer_code) .run(as_dict=True) ) Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a valid and important security suggestion to add a permission check to a whitelisted API endpoint, preventing potential unauthorized data access.	Medium
General	Validate delivery date before PO In create_purchase_order, add a check to ensure doc.delivery_date has a value before creating a new Purchase Order, and throw an error if it is missing. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [214-237] def create_purchase_order(doc): + if not doc.delivery_date: + frappe.throw(_("Delivery Date is required to create Purchase Order")) + total_qty = sum(row.qty for row in doc.order_details) po = frappe.new_doc("Purchase Order") po.update( { "supplier": doc.supplier, "company": doc.company, "branch": doc.branch, "project": doc.project, "custom_form": "Sketch Order Form", "custom_form_id": doc.name, "purchase_type": "Subcontracting", "custom_sketch_order_form": doc.name, "schedule_date": doc.delivery_date, } ) item = po.append("items", {}) item.item_code = "Design Expness" item.qty = total_qty item.schedule_date = doc.delivery_date po.save() Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a missing validation for doc.delivery_date, which is used to set required fields in the Purchase Order, thus preventing potential errors.	Medium
	Optimize list retrieval with pluck In cancel_linked_sketch_orders, use pluck="name" with frappe.db.get_list to directly fetch a list of names, which simplifies the code by removing the need for a list comprehension. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [55-65] def cancel_linked_sketch_orders(self): - sketch_orders = frappe.db.get_list( - "Sketch Order", filters={"sketch_order_form": self.name}, fields="name" + sketch_order_names = frappe.db.get_list( + "Sketch Order", filters={"sketch_order_form": self.name}, pluck="name" ) frappe.db.set_value( "Sketch Order", - {"name": ["in", [order["name"] for order in sketch_orders]]}, + {"name": ["in", sketch_order_names]}, "workflow_state", "Cancelled", ) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a code simplification and minor performance improvement by using pluck="name", which is a good practice in Frappe.	Low
Incremental [*]	Optimize bulk update query Optimize the bulk update in update_delivery_dates by replacing frappe.db.set_value with a more performant frappe.db.sql query. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [48-53] -frappe.db.set_value( - "Sketch Order", - {"name": ["in", sketch_order_names]}, - "update_delivery_date", - self.updated_delivery_date, -) +if sketch_order_names: + frappe.db.sql( + """UPDATE `tabSketch Order` + SET update_delivery_date = %s + WHERE name IN ({})""".format(", ".join(["%s"] * len(sketch_order_names))), + [self.updated_delivery_date] + sketch_order_names + ) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion proposes using a raw SQL query for a potential performance gain, which is a valid optimization, although frappe.db.set_value with an IN clause is the standard and often sufficient approach.	Low
	Reduce dictionary lookup operations Refactor the department shift lookup to use dict.get() to perform a single lookup instead of two. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [108-111] -if parent.department not in department_shifts: +shift_times = department_shifts.get(parent.department) +if not shift_times: return -shift_start, shift_end = department_shifts[parent.department] +shift_start, shift_end = shift_times Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion offers a valid micro-optimization by replacing two dictionary lookups (in and []) with a single .get() call, improving code conciseness and efficiency slightly.	Low
General	Validate quantity before order creation Add a validation check to ensure total_qty is greater than zero before creating a Purchase Order. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [214-237] def create_purchase_order(doc): total_qty = sum(row.qty for row in doc.order_details) + + if total_qty <= 0: + frappe.throw(_("Cannot create Purchase Order with zero or negative quantity")) po = frappe.new_doc("Purchase Order") po.update( { "supplier": doc.supplier, "company": doc.company, "branch": doc.branch, "project": doc.project, "custom_form": "Sketch Order Form", "custom_form_id": doc.name, "purchase_type": "Subcontracting", "custom_sketch_order_form": doc.name, "schedule_date": doc.delivery_date, } ) item = po.append("items", {}) item.item_code = "Design Expness" item.qty = total_qty item.schedule_date = doc.delivery_date po.save() Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion adds a useful validation to prevent the creation of purchase orders with zero quantity, which improves data integrity and prevents potential downstream errors.	Medium
	Skip empty list database operations Add a check to ensure sketch_order_names is not empty before attempting a database update to avoid unnecessary operations. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [42-53] def update_delivery_dates(self): if self.updated_delivery_date: sketch_order_names = frappe.get_all( "Sketch Order", filters={"sketch_order_form": self.name}, pluck="name" ) - frappe.db.set_value( - "Sketch Order", - {"name": ["in", sketch_order_names]}, - "update_delivery_date", - self.updated_delivery_date, - ) + if sketch_order_names: + frappe.db.set_value( + "Sketch Order", + {"name": ["in", sketch_order_names]}, + "update_delivery_date", + self.updated_delivery_date, + ) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that an empty list check can prevent a needless database call, which is a good practice for performance and robustness, although the impact is minor.	Low
Security	Validate input before database query Add validation for the customer_code parameter to sanitize the input before it is used in a database query. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [246-254] @frappe.whitelist() def get_customer_orderType(customer_code): + if not customer_code or not isinstance(customer_code, str): + frappe.throw(_("Invalid customer code")) + OrderType = DocType("Order Type") return ( frappe.qb.from_(OrderType) .select(OrderType.order_type) .where(OrderType.parent == customer_code) .run(as_dict=True) ) Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion adds a basic input validation check, which is good practice, but Frappe's query builder already provides sufficient protection against SQL injection in this context, making the change a minor improvement.	Low
Update

• Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

---

🛠️ Relevant configurations:

These are the relevant configurations for this tool:

[config]

enable_ai_metadata: False
model_reasoning: vertex_ai/gemini-2.5-pro
model: gpt-5.2-2025-12-11
model_turbo: anthropic/claude-haiku-4-5-20251001
fallback_models: ['anthropic/claude-sonnet-4-5-20250929', 'bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0']
second_model_for_exhaustive_mode: o4-mini
git_provider: github
publish_output: True
publish_output_no_suggestions: True
publish_output_progress: True
verbosity_level: 0
publish_logs: False
debug_mode: False
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
use_global_wiki_settings_file: False
disable_auto_feedback: False
ai_timeout: 150
response_language: en-US
clone_repo_instead_of_fetch: True
always_clone: False
add_repo_metadata: True
clone_repo_time_limit: 300
publish_inline_comments_fallback_batch_size: 5
publish_inline_comments_fallback_sleep_time: 2
max_model_tokens: 32000
custom_model_max_tokens: -1
patch_extension_skip_types: ['.md', '.txt']
extra_allowed_extensions: []
allow_dynamic_context: True
allow_forward_dynamic_context: True
max_extra_lines_before_dynamic_context: 12
patch_extra_lines_before: 5
patch_extra_lines_after: 1
ai_handler: litellm
cli_mode: False
TRIAL_GIT_ORG_MAX_INVOKES_PER_MONTH: 75
TRIAL_RATIO_CLOSE_TO_LIMIT: 0.8
invite_only_mode: False
enable_request_access_msg_on_new_pr: False
check_also_invites_field: False
calculate_context: True
disable_checkboxes: False
output_relevant_configurations: True
large_patch_policy: clip
seed: -1
temperature: 0.2
allow_dynamic_context_ab_testing: False
choose_dynamic_context_ab_testing_ratio: 0.5
ignore_pr_title: ['^\\[Auto\\]', '^Auto', '^\\[Bump\\]']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: ['do-not-merge', 'skip-qodo']
ignore_ticket_labels: []
allow_only_specific_folders: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
is_auto_command: False
is_new_pr: False
present_reasoning: True
max_tickets: 10
max_tickets_chars: 8000
prevent_any_approval: False
enable_comment_approval: False
enable_auto_approval: False
auto_approve_for_low_review_effort: -1
auto_approve_for_no_suggestions: False
ensure_ticket_compliance: False
new_diff_format: True
new_diff_format_add_external_references: True
enable_custom_labels: False
user_interaction_statistics: {'action_type': 'more_suggestions'}
disable_head_sha_cleanup: False
extra_statistics: {'suggestion_statistics': [{'score': 5, 'label': "incremental <sup><a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a></sup>"}, {'score': 4, 'label': "incremental <sup><a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a></sup>"}, {'score': 5, 'label': 'general'}, {'score': 7, 'label': 'general'}, {'score': 8, 'label': 'security'}]}

[pr_code_suggestions]

suggestions_depth: exhaustive
commitable_code_suggestions: False
decouple_hunks: False
dual_publishing_score_threshold: -1
focus_only_on_problems: True
allow_thumbs_up_down: False
enable_suggestion_type_reuse: False
enable_more_suggestions_checkbox: True
high_level_suggestions_enabled: True
extra_instructions: Focus suggestions on PERFORMANCE + SECURITY:
- Reduce DB roundtrips (bulk fetch, IN queries, joins/aggregates).
- Prefer set_value over save for small field updates unless validations are required.
- Ensure parameterized SQL only.
- Ensure endpoints are whitelisted intentionally and respect permissions.
- Prefer chunking/generators for big datasets.
Return copy-paste ready snippets when feasible.

enable_help_text: False
show_extra_context: False
persistent_comment: True
max_history_len: 5
apply_suggestions_checkbox: True
enable_chat_in_code_suggestions: True
apply_limit_scope: True
suggestions_score_threshold: 0
new_score_mechanism: True
new_score_mechanism_th_high: 9
new_score_mechanism_th_medium: 7
discard_unappliable_suggestions: False
num_code_suggestions_per_chunk: 6
num_best_practice_suggestions: 2
max_number_of_calls: 3
final_clip_factor: 0.8
demand_code_suggestions_self_review: True
code_suggestions_self_review_text: **Author self-review**: I have reviewed the PR code suggestions, and addressed the relevant ones.
approve_pr_on_self_review: False
fold_suggestions_on_self_review: True
publish_post_process_suggestion_impact: True
wiki_page_accepted_suggestions: True
enable_local_self_reflect_in_large_prs: False
simplify_response: True

---

Previous suggestions

✅ Suggestions up to commit 2580192

Category	Suggestion	Impact
General	✅ Avoid redundant database save operations Suggestion Impact: The suggestion's intent was implemented but through a different approach. Instead of conditionally saving in make_sketch_order, the commit refactored the code to eliminate the redundant save by moving the apply_parent_dates and apply_order_criteria_dates calls inside the set_missing_values callback (lines 108-109). This removes the need for the separate save operation that was happening in create_sketch_order (lines 50-53 removed), achieving the same performance improvement of avoiding redundant database writes. code diff: for row in doc.order_details: - order_name = make_sketch_order("Sketch Order Form Detail", row.name, doc) - - sketch_order = frappe.get_doc("Sketch Order", order_name) - - apply_parent_dates(doc, sketch_order) - apply_order_criteria_dates(doc, sketch_order, order_criteria) - - sketch_order.save(ignore_permissions=True) + order_name = make_sketch_order( + "Sketch Order Form Detail", row.name, doc, order_criteria, department_shifts + ) created_orders.append(get_link_to_form("Sketch Order", order_name)) - if created_orders: frappe.msgprint( _("The following {0} were created: {1}").format( @@ -97,26 +101,20 @@ sketch_order.delivery_date = get_datetime(parent.delivery_date) -def apply_order_criteria_dates(parent, sketch_order, order_criteria): +def apply_order_criteria_dates(parent, sketch_order, order_criteria, department_shifts): if not parent.order_date: return + if parent.department not in department_shifts: + return + + shift_start, shift_end = department_shifts[parent.department] + parent_date = getdate(parent.order_date) - - department_shifts = { - row.department: (get_time(row.shift_start_time), get_time(row.shift_end_time)) - for row in order_criteria.department_shift - if not row.disable - } for criteria in order_criteria.order: if criteria.disable: continue - - if parent.department not in department_shifts: - continue - - shift_start, shift_end = department_shifts[parent.department] sketch_delivery = calculate_sketch_delivery(parent_date, criteria) sketch_order.sketch_delivery_date = sketch_delivery @@ -162,12 +160,16 @@ return datetime.combine(next_day, shift_start) + timedelta(hours=extra_hours) -def make_sketch_order(doctype, source_name, parent_doc, target_doc=None): +def make_sketch_order( + doctype, source_name, parent_doc, order_criteria, department_shifts, target_doc=None +): def set_missing_values(source, target): target.sketch_order_form_detail = source.name target.sketch_order_form = source.parent target.sketch_order_form_index = source.idx copy_parent_fields(parent_doc, target) + apply_parent_dates(parent_doc, target) + apply_order_criteria_dates(parent_doc, target, order_criteria, department_shifts) In make_sketch_order, avoid a redundant database save by only calling doc.save() if a target_doc is not provided, allowing the caller to control the save operation. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [165-181] def make_sketch_order(doctype, source_name, parent_doc, target_doc=None): def set_missing_values(source, target): target.sketch_order_form_detail = source.name target.sketch_order_form = source.parent target.sketch_order_form_index = source.idx copy_parent_fields(parent_doc, target) doc = get_mapped_doc( doctype, source_name, {doctype: {"doctype": "Sketch Order"}}, target_doc, set_missing_values, ) - doc.save() + if not target_doc: + doc.save() return doc.name [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a redundant doc.save() call within make_sketch_order that causes an extra database write for each item in the loop in create_sketch_order. The proposed fix to conditionally save the document is accurate and significantly improves performance by halving the number of database writes.	Medium
	Optimize validation by reducing database queries Optimize validate_category_subcategory by fetching all parent categories in a single database query before the loop to resolve the N+1 problem. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [26-35] def validate_category_subcategory(self): + subcategories = [row.subcategory for row in self.order_details if row.subcategory] + if not subcategories: + return + + parent_map = { + d.name: d.parent_attribute_value + for d in frappe.get_all( + "Attribute Value", + filters={"name": ("in", list(set(subcategories)))}, + fields=["name", "parent_attribute_value"], + ) + } + for row in self.order_details: if row.subcategory: - parent_category = frappe.db.get_value( - "Attribute Value", row.subcategory, "parent_attribute_value" - ) + parent_category = parent_map.get(row.subcategory) if row.category != parent_category: frappe.throw( _("Category & Sub Category mismatched in row #{0}").format(row.idx) ) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an N+1 query problem in the validate_category_subcategory method and provides an effective solution to optimize it by using a single bulk query, which is a significant performance improvement.	Medium
	✅ Bulk update records to reduce database calls Suggestion Impact: The suggestion was directly implemented in the commit. The loop was removed and replaced with a single bulk update using the IN filter syntax. Additionally, the same optimization pattern was applied to the cancel_linked_sketch_orders method, which was not part of the original suggestion but follows the same principle. code diff: - for sketch_order_names in sketch_order_names: - frappe.db.set_value( - "Sketch Order", - sketch_order_names, - "update_delivery_date", - self.updated_delivery_date, - ) + frappe.db.set_value( + "Sketch Order", + {"name": ["in", sketch_order_names]}, + "update_delivery_date", + self.updated_delivery_date, + ) Optimize update_delivery_dates by using a single bulk frappe.db.set_value call with an IN filter to update all sketch orders at once, reducing database writes. jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [42-54] def update_delivery_dates(self): if self.updated_delivery_date: sketch_order_names = frappe.get_all( "Sketch Order", filters={"sketch_order_form": self.name}, pluck="name" ) - for sketch_order_names in sketch_order_names: + if sketch_order_names: frappe.db.set_value( "Sketch Order", - sketch_order_names, + {"name": ("in", sketch_order_names)}, "update_delivery_date", self.updated_delivery_date, ) [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies inefficient database updates within a loop in the update_delivery_dates method. The proposed change to use a single bulk update is a valid and significant performance optimization.	Medium
High-level	Move creation logic into class methods Move the create_sketch_order and create_purchase_order global functions into the SketchOrderForm class as instance methods. This will better encapsulate behavior related to the document and improve the object-oriented design. Examples: jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [37-41] def handle_submit(self): create_sketch_order(self) if self.supplier: create_purchase_order(self) jewellery_erpnext/gurukrupa_exports/doctype/sketch_order_form/sketch_order_form.py [67-87] def create_sketch_order(doc): order_criteria = frappe.get_single("Order Criteria") created_orders = [] for row in doc.order_details: order_name = make_sketch_order("Sketch Order Form Detail", row.name, doc) sketch_order = frappe.get_doc("Sketch Order", order_name) apply_parent_dates(doc, sketch_order) ... (clipped 11 lines) Solution Walkthrough: Before: class SketchOrderForm(Document): # ... def handle_submit(self): create_sketch_order(self) if self.supplier: create_purchase_order(self) # ... def create_sketch_order(doc): # ... logic to create sketch order ... # uses doc as the SketchOrderForm instance def create_purchase_order(doc): # ... logic to create purchase order ... # uses doc as the SketchOrderForm instance After: class SketchOrderForm(Document): # ... def handle_submit(self): self._create_sketch_order() if self.supplier: self._create_purchase_order() def _create_sketch_order(self): # ... logic to create sketch order ... # uses self as the SketchOrderForm instance def _create_purchase_order(self): # ... logic to create purchase order ... # uses self as the SketchOrderForm instance # ... Suggestion importance[1-10]: 6 __ Why: This is a valid design suggestion that improves encapsulation and object-oriented structure, but it's not a critical issue as the current refactoring already significantly improves code organization.	Low

[manikandan-s-18]

/review

[manikandan-s-18]

/review

[DHINESH00]

/review

[manikandan-s-18]

/review