refactor: optimize sketch order workflow#4
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🛠️ Relevant configurations:
[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
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_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
|
PR Code Suggestions ✨Explore these optional code suggestions:
🛠️ Relevant configurations:
[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
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
extra_statistics: {'suggestion_statistics': [{'score': 5, 'label': 'general'}, {'score': 6, 'label': 'general'}, {'score': 7, 'label': 'general'}]}
[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
|
||||||||||||||||
PR Type
Enhancement
Description
Refactored sketch order workflow with improved code formatting and structure
Removed unused functions (
create_item_from_sketch_order,create_system_notification,update_item_variant)Optimized database queries by consolidating multiple
frappe.db.get_value()callsSimplified item creation logic with cleaner post-processing using
target.update()Removed extensive commented-out code and dead code paths
Diagram Walkthrough
File Walkthrough
sketch_order.py
Code cleanup and database query optimizationjewellery_erpnext/gurukrupa_exports/doctype/sketch_order/sketch_order.py
create_item_from_sketch_order()function and related itemvariant creation logic
create_system_notification()function entirelyupdate_item_variant()functioncreate_item_template_from_sketch_order()by consolidatingtwo separate
frappe.db.get_value()calls into onecreate_item_for_po()post-processing usingtarget.update()dictionary approach
updatet_item_template()toupdate_item_template()(typo fix)populate_child_table()🛠️ Relevant configurations:
These are the relevant configurations for this tool:
[config]
[pr_description]