diff --git a/pos_next/api/branding.py b/pos_next/api/branding.py index a942ab58..6b15d2da 100644 --- a/pos_next/api/branding.py +++ b/pos_next/api/branding.py @@ -12,7 +12,18 @@ import base64 import hashlib - +# REVIEW: GIG QUESTION: Why do we use @frappe.whitelist??!! + +# REVIEW: This function has too many responsibilities: +# - DB existence check +# - configuration retrieval +# - obfuscation +# - formatting UI config +# Suggest refactoring into smaller helpers: +# fetch_branding_doc() +# obfuscate_branding_data() +# build_branding_payload() +# Also consider caching via frappe.cache() since branding rarely changes. @frappe.whitelist(allow_guest=False) def get_branding_config(): """ @@ -57,6 +68,10 @@ def get_branding_config(): return get_default_config() +# REVIEW: Static default config is ok, +# but returns nearly same structure as get_branding_config(). +# Consider making a constant DEFAULT_BRANDING dict or reading defaults from System Settings or Branding DocType schema. +# Also avoid computing base64 repeatedly — generate once, reuse. def get_default_config(): """Return default branding configuration""" return { @@ -128,6 +143,10 @@ def validate_branding(client_signature=None, brand_name=None, brand_url=None): return {"valid": False, "error": str(e)} +# REVIEW: This API is overly permissive: +# Users can POST arbitrary events. Consider logging only for authenticated roles. +# Also `event_type` should be enumerated centrally to avoid typos. +# Suggest moving event mapping into a constant DICT instead of if/elif chains. @frappe.whitelist(allow_guest=False) def log_client_event(event_type=None, details=None): """ @@ -197,6 +216,9 @@ def log_tampering_attempt(doc, details): frappe.log_error(f"Error logging tampering: {str(e)}", "BrainWise Branding") +# REVIEW: Direct role check is acceptable, but use frappe.has_permission() +# or a permission rule instead of manual role checking. +# Also consider caching the statistics if this is called frequently. @frappe.whitelist(allow_guest=False) def get_tampering_stats(): """Get tampering statistics (admin only)""" diff --git a/pos_next/api/credit_sales.py b/pos_next/api/credit_sales.py index 2f23a758..0974e70c 100644 --- a/pos_next/api/credit_sales.py +++ b/pos_next/api/credit_sales.py @@ -14,6 +14,17 @@ from frappe.utils import flt, nowdate, today, cint, get_datetime +# REVIEW: GIG QUESTION: Why do we use @frappe.whitelist??!! + +# REVIEW: This function mixes multiple concerns: +# 1. Calculating outstanding invoices +# 2. Calculating negative credits +# 3. Calculating unallocated advances +# RECOMMENDATIONS: breaking into helper functions like: +# - _get_customer_outstanding() +# - _get_customer_credits() +# - _get_customer_advances() +# Also, it's not good to write raw SQL queries. @frappe.whitelist() def get_customer_balance(customer, company=None): """ @@ -129,6 +140,11 @@ def check_credit_sale_enabled(pos_profile): return bool(pos_settings) +# REVIEW: +# Suggestions: +# - Break into _get_negative_invoices() and _get_unallocated_advances() +# - DRY: negative outstanding conversion occurs multiple times. +# - Consider performance with large datasets (pagination or filters) @frappe.whitelist() def get_available_credit(customer, company, pos_profile=None): """ @@ -216,6 +232,11 @@ def get_available_credit(customer, company, pos_profile=None): return total_credit +# REVIEW: +# Suggestions: +# - Validation of input dict should be stricter. +# - Transactional safety: consider wrapping all allocations in a single transaction. +# - Could extract allocation logic to a CreditAllocationService class for scalability. @frappe.whitelist() def redeem_customer_credit(invoice_name, customer_credit_dict): """ @@ -279,6 +300,11 @@ def redeem_customer_credit(invoice_name, customer_credit_dict): return created_journal_entries +# REVIEW: +# - Directly submits JE, which can fail mid-process — consider transaction/atomic commit. +# - Consider extracting debit/credit creation into a helper for DRY. +# - Cost center retrieval should default more safely if not set. +# - Could add logging/debug info for audit. def _create_credit_allocation_journal_entry(invoice_doc, original_invoice_name, amount): """ Create Journal Entry to allocate credit from one invoice to another. @@ -494,6 +520,10 @@ def get_credit_sale_summary(pos_profile): } +# REVIEW: Retrieves credit invoices. +# - Access control is manual; consider central permission check for POS Profiles. +# - Could refactor SQL query into a helper for reuse. +# - It's not good to write raw SQL queries. @frappe.whitelist() def get_credit_invoices(pos_profile, limit=100): """ diff --git a/pos_next/pos_next/doctype/brainwise_branding/brainwise_branding.py b/pos_next/pos_next/doctype/brainwise_branding/brainwise_branding.py index d5ab654c..e9b9099e 100644 --- a/pos_next/pos_next/doctype/brainwise_branding/brainwise_branding.py +++ b/pos_next/pos_next/doctype/brainwise_branding/brainwise_branding.py @@ -10,6 +10,7 @@ from datetime import datetime import secrets +# REVIEW: We should not have the hashes in the code. # MASTER KEY HASH - Only the person with the original key can disable branding # This hash was created from: secrets.token_urlsafe(32) @@ -19,11 +20,20 @@ # Secondary protection - requires both master key AND this phrase PROTECTION_PHRASE_HASH = "3ddb5c12a034095ff81a85bbd06623a60e81252c296b747cf9c127dc57e013a8" - +# REVIEW: We need to respect the order of frappe hooks like validate, before_save, etc. +# And respect the order of the helper methods. python files should be read from top to bottom. class BrainWiseBranding(Document): # Protected fields that require master key to modify PROTECTED_FIELDS = ['enabled', 'brand_text', 'brand_name', 'brand_url', 'check_interval'] + # REVIEW: The validate() method is too large and mixes responsibilities: + # - permission verification + # - logging + # - enforcing state logic + # RECOMMENDATION: Split into smaller helper methods: + # _verify_protected_field_permissions() + # _enforce_disable_protection() + # _log_master_key_usage() def validate(self): """Validate before saving - enforce master key requirement""" if not self.is_new(): @@ -63,6 +73,9 @@ def validate(self): frappe.PermissionError ) + # REVIEW: + # Suggest caching DB values or batching pull requests to reduce DB calls, + # especially if PROTECTED_FIELDS grows longer. def _check_protected_fields_changed(self): """Check if any protected fields have been modified""" if self.is_new(): @@ -79,6 +92,9 @@ def _check_protected_fields_changed(self): return changed_fields + # REVIEW: It's not good to have the code directly in the before_save method. + # We should have a helper method for this. + # RECOMMENDATION: clear master key using: self.set("master_key_provided", None) def before_save(self): """Generate encrypted signature and enforce protections""" # Always enforce enabled state unless master key provided @@ -92,6 +108,17 @@ def before_save(self): if self.master_key_provided: self.master_key_provided = None + # REVIEW: This function does multiple things: + # 1) input parsing + # 2) hashing + # 3) logging + # 4) validation decision + # Recommend splitting into: + # _parse_master_key() + # _hash_keys() + # _log_key_attempt(success=True/False) + # Also — security note: + # Too much detail about failure logging could help attackers. def _validate_master_key(self): """Internal method to validate master key""" if not self.master_key_provided: @@ -143,6 +170,7 @@ def _validate_master_key(self): frappe.log_error(f"Master key validation error: {str(e)}", "BrainWise Branding") return False + # REVIEW: Suggest moving encryption logic to a utility module. def generate_signature(self): """Generate an encrypted signature for branding validation""" # Create a signature from branding data @@ -175,6 +203,10 @@ def generate_signature(self): }).encode() ).decode() + # REVIEW: validate_signature() currently only checks brand_name / brand_url. + # It does NOT actually validate HMAC integrity. + # Meaning: any attacker can replay the existing signature successfully. + # SUGGESTION: Recalculate HMAC from received data & compare! def validate_signature(self, client_data): """Validate client-side data against server signature""" if not self.encrypted_signature: @@ -194,6 +226,10 @@ def validate_signature(self, client_data): frappe.log_error(f"Branding validation error: {str(e)}", "BrainWise Branding") return False + # REVIEW: Method makes DB writes inside a validation / logging scenario. + # This could cause recursive save loops. + # Suggest: use frappe.db.set_value instead of self.save() + # Also tampering_attempts should likely be limited or rate-tracked. def log_tampering(self, details): """Log tampering attempts""" if not self.log_tampering_attempts: @@ -210,7 +246,12 @@ def log_tampering(self, details): message=json.dumps(details, indent=2, default=str) ) +# REVIEW: It's better to have these apis in a separate file in the api folder. +# REVIEW: API function does too many things: +# Suggest splitting into: +# _serialize_branding_config() +# _self_heal_branding_state() @frappe.whitelist(allow_guest=False) def get_branding_config(): """API endpoint to get branding configuration""" @@ -248,7 +289,10 @@ def get_branding_config(): "_e": 1 } - +# REVIEW: validate_branding calls doc.save() on every validation. +# If this is called frequently by the client (e.g., every 10 seconds), +# this will cause unnecessary constant writes. +# Suggest: move last_validation into cache or redis instead of Doc field. @frappe.whitelist(allow_guest=False) def validate_branding(client_signature=None, brand_name=None, brand_url=None): """Validate branding integrity from client""" @@ -325,7 +369,8 @@ def log_client_event(event_type=None, details=None): frappe.log_error(f"Error logging client event: {str(e)}", "BrainWise Branding") return {"logged": False, "error": str(e)} - +# REVIEW: Good — but mixes responsibilities again: parsing, logging, validation. +# Consider moving key verification logic into _validate_master_key_shared() @frappe.whitelist() def verify_master_key(master_key_input): """ @@ -376,7 +421,8 @@ def verify_master_key(master_key_input): "error": str(e) } - +# REVIEW: The system allows regeneration of master key but requires manual code editing. +# Suggest storing hashes in Site Config instead of hard-coded python constants. @frappe.whitelist() def generate_new_master_key(): """ diff --git a/pos_next/pos_next/doctype/pos_closing_shift/pos_closing_shift.py b/pos_next/pos_next/doctype/pos_closing_shift/pos_closing_shift.py index 704d5489..cac4f491 100644 --- a/pos_next/pos_next/doctype/pos_closing_shift/pos_closing_shift.py +++ b/pos_next/pos_next/doctype/pos_closing_shift/pos_closing_shift.py @@ -5,6 +5,7 @@ from collections import defaultdict import frappe +# REVIEW: We import the consolidate_pos_invoices but we don't use it. from erpnext.accounts.doctype.pos_invoice_merge_log.pos_invoice_merge_log import ( consolidate_pos_invoices, ) @@ -13,6 +14,11 @@ from frappe.utils import flt +# REVIEW: +# - The conversion rate detection is duplicated in many places. +# Should be extracted to a shared method. +# - Consider caching conversion rate to avoid multiple doc.get() calls. +# - Should explicitly type input args for clarity. def get_base_value(doc, fieldname, base_fieldname=None, conversion_rate=None): """Return the value for a field in company currency.""" @@ -39,6 +45,12 @@ def get_base_value(doc, fieldname, base_fieldname=None, conversion_rate=None): class POSClosingShift(Document): + # REVIEW: + # Issues: + # - DB calls and business logic mixed. + # - Could be split into: + # _validate_user_shift_conflict() + # _validate_opening_shift_status() def validate(self): user = frappe.get_all( "POS Closing Shift", @@ -67,6 +79,9 @@ def validate(self): ) self.update_payment_reconciliation() + # REVIEW: + # But name is misleading — it is computing diffs not updating reconciliation + # Should be: compute_payment_differences() def update_payment_reconciliation(self): # update the difference values in Payment Reconciliation child table # get default precision for site @@ -74,6 +89,13 @@ def update_payment_reconciliation(self): for d in self.payment_reconciliation: d.difference = +flt(d.closing_amount, precision) - flt(d.expected_amount, precision) + # REVIEW: + # But function is doing 4 things: + # - retrieve shift + # - link shift to opening + # - delete invoices + # - link invoices + # Should be split for readability and testability. def on_submit(self): opening_entry = frappe.get_doc("POS Opening Shift", self.pos_opening_shift) opening_entry.pos_closing_shift = self.name @@ -83,6 +105,8 @@ def on_submit(self): # link invoices with this closing shift so ERPNext can block edits self._set_closing_entry_invoices() + # REVIEW: + # It's better to have a helper method for this. def on_cancel(self): if frappe.db.exists("POS Opening Shift", self.pos_opening_shift): opening_entry = frappe.get_doc("POS Opening Shift", self.pos_opening_shift) @@ -103,6 +127,17 @@ def _set_closing_entry_invoices(self): if frappe.db.has_column(doctype, "pos_closing_entry"): frappe.db.set_value(doctype, invoice, "pos_closing_entry", self.name) + # REVIEW: + # _clear_closing_entry_invoices is large and mixes responsibilities: + # - unlink pos_invoice -> pos_closing_entry + # - cancel and delete POS Invoice Merge Log entries + # - clear consolidated_invoice pointers + # - set POS Invoice status + # - collect consolidated sales invoices and cancel them + # RECOMMENDATIONS: + # - Break into: _unlink_pos_invoice(), _handle_merge_logs_for_pos_invoice(), _accumulate_consolidated_si() + # - Use batched queries when collecting merge logs to reduce query overhead. + # - Add defensive error handling and logging per invoice to avoid stopping midway. def _clear_closing_entry_invoices(self): """Clear closing shift links, cancel merge logs and cancel consolidated sales invoices.""" consolidated_sales_invoices = set() @@ -167,6 +202,9 @@ def _is_consolidated_sales_invoice(self, sales_invoice): ) ) + # REVIEW: + # It's not good to write raw SQL queries. + # We should use frappe.db.get_all() instead. def delete_draft_invoices(self): if frappe.get_value("POS Profile", self.pos_profile, "posa_allow_delete"): doctype = "Sales Invoice" @@ -186,6 +224,16 @@ def delete_draft_invoices(self): for invoice in data: frappe.delete_doc(doctype, invoice.name, force=1) + # REVIEW: + # get_payment_reconciliation_details assembles a template context and renders HTML. + # This method does: + # - iterates invoices and payment entries (many DB interactions) + # - builds breakdowns and mode summaries + # - renders with frappe.render_template + # RECOMMENDATIONS: + # - Extract data-aggregation into a service returning a serializable dict. + # - Rendering should be the final step of a small function that accepts aggregated data. + # - Use batch fetching and caching for invoices to reduce many get_doc calls. @frappe.whitelist() def get_payment_reconciliation_details(self): company_currency = frappe.get_cached_value( @@ -360,6 +408,8 @@ def get_cashiers(doctype, txt, searchfield, start, page_len, filters): return result +# REVIEW: +# It's not good to write raw SQL queries. @frappe.whitelist() def get_pos_invoices(pos_opening_shift, doctype=None): if not doctype: @@ -408,6 +458,17 @@ def get_payments_entries(pos_opening_shift): ) +# REVIEW: +# make_closing_shift_from_opening: +# - deserializes opening_shift from JSON expecting a string. +# - lots of logic building the closing_shift: aggregation, conversion, taxes, payments. +# SUGGESTIONS: +# - Validate that opening_shift is dict before json.loads to avoid unexpected exceptions. +# - Split into smaller functions: +# _build_transactions() +# _aggregate_taxes() +# _aggregate_payments() +# - Use batch retrieval of invoices to avoid repeated DB calls. @frappe.whitelist() def make_closing_shift_from_opening(opening_shift): opening_shift = json.loads(opening_shift) diff --git a/pos_next/pos_next/doctype/pos_coupon/pos_coupon.py b/pos_next/pos_next/doctype/pos_coupon/pos_coupon.py index e57b832b..0d264566 100644 --- a/pos_next/pos_next/doctype/pos_coupon/pos_coupon.py +++ b/pos_next/pos_next/doctype/pos_coupon/pos_coupon.py @@ -20,6 +20,16 @@ def autoname(self): elif self.coupon_type == "Gift Card": self.coupon_code = frappe.generate_hash()[:10].upper() + # REVIEW: + # - validate() function is too large and handles multiple validation responsibilities. + # - Should be broken into smaller functions like: + # validate_gift_card(), + # validate_discount_configuration(), + # validate_amount_rules(), + # validate_dates() + # - Improves readability, organization, debugging, and unit‐testability. + # - Consider moving static messages to a constants file for localization consistency. + # - Should not mutate maximum_use inside validation — better in before_insert or separate initializer. def validate(self): # Gift Card validations if self.coupon_type == "Gift Card": @@ -57,6 +67,12 @@ def validate(self): +# REVIEW: +# - This function is doing too many things: lookup, date validation, company validation, usage count checking. +# - Should be split into smaller logical units for readability and scalability. +# - Return value should be standardized (either return objects or standardized response schema). +# - coupon_code.upper() is repeated — normalize once at the top. +# - Database lookups could be optimized using frappe.db.get_value instead of get_doc when not needed. def check_coupon_code(coupon_code, customer=None, company=None): """Validate and return coupon details""" res = {"coupon": None} @@ -118,6 +134,11 @@ def check_coupon_code(coupon_code, customer=None, company=None): return res +# REVIEW: +# - Function contains both calculation logic and business rule enforcement. +# - Consider moving business rules to configuration settings or database for flexibility. +# - Should support future extensibility — e.g. buy-one-get-one, item-specific discounts. +# - Recommend returning a strongly typed object instead of a free-form dict (dataclass or standardized dict). def apply_coupon_discount(coupon, cart_total, net_total=None): """Calculate discount amount based on coupon configuration""" from frappe.utils import flt @@ -157,6 +178,11 @@ def apply_coupon_discount(coupon, cart_total, net_total=None): } +# REVIEW: +# - Should NOT call frappe.db.commit() inside utility function. +# - This breaks transaction atomicity and makes rollback impossible. +# - Instead allow commit at the request controller level. +# - Should perform update using frappe.db.set_value instead of loading document for performance. def increment_coupon_usage(coupon_code): """Increment the usage counter for a coupon""" try: @@ -171,6 +197,11 @@ def increment_coupon_usage(coupon_code): ) +# REVIEW: +# - Same commit issue as above — commit should not be internal. +# - Should gracefully handle case where coupon does not exist. +# - Should avoid loading full document just to decrement integer. +# - Consider storing usage as tracked metrics rather than mutating document. def decrement_coupon_usage(coupon_code): """Decrement the usage counter for a coupon (for cancelled invoices)""" try: diff --git a/pos_next/pos_next/doctype/pos_settings/pos_settings.py b/pos_next/pos_next/doctype/pos_settings/pos_settings.py index abad105e..6c0d4e3e 100644 --- a/pos_next/pos_next/doctype/pos_settings/pos_settings.py +++ b/pos_next/pos_next/doctype/pos_settings/pos_settings.py @@ -7,6 +7,10 @@ class POSSettings(Document): + # REVIEW: + # RECOMMENDATIONS: + # - Extract discount validation & search_limit validation into private helper methods for SRP. + # - Add explicit type hints for maintainability. def validate(self): """Validate POS Settings""" # Guard against None values and validate discount percentage @@ -24,6 +28,10 @@ def on_update(self): """Sync allow_negative_stock with Stock Settings""" self.sync_negative_stock_setting() + # REVIEW: + # RECOMMENDATIONS: + # - Use caching if many transactions call this repeatedly. + # - Potential scalability issue: frappe.db.count on every change (consider caching or optimizing). def sync_negative_stock_setting(self): """ Synchronize allow_negative_stock with Stock Settings. @@ -69,6 +77,11 @@ def sync_negative_stock_setting(self): ) +# REVIEW: +# - This function does authentication + DB fetch + side-effect mutation (inject field). +# Consider splitting responsibilities for cleaner architecture. +# - Input parameter should be validated for type (string). +# - Return schema should be documented for API stability. @frappe.whitelist() def get_pos_settings(pos_profile): """ @@ -122,6 +135,14 @@ def create_default_settings(pos_profile): return doc.as_dict() +# REVIEW: +# - Handles JSON input + validation + DB update. +# - RECOMMENDATIONS: +# - Split actions into separate helper functions: +# _check_access(), _update_existing_settings(), _create_new_settings() +# - Returning full doc.as_dict() may leak unnecessary internal data. +# - Security suggestion: +# - Validate the fields being saved to avoid unauthorized update of restricted properties. @frappe.whitelist() def update_pos_settings(pos_profile, settings): """Update POS Settings for a POS Profile""" diff --git a/pos_next/pos_next/doctype/referral_code/referral_code.py b/pos_next/pos_next/doctype/referral_code/referral_code.py index 465bada1..ff1c1e8e 100644 --- a/pos_next/pos_next/doctype/referral_code/referral_code.py +++ b/pos_next/pos_next/doctype/referral_code/referral_code.py @@ -19,6 +19,11 @@ def autoname(self): if not self.referral_code: self.referral_code = frappe.generate_hash()[:10].upper() + # REVIEW: This validation method is long and includes duplicated logic for referrer & referee. + # Consider refactoring into smaller helper functions like: + # validate_referrer_discounts() and validate_referee_discounts() + # RECOMMENDATIONS: + # - Improves readability & testability. def validate(self): # Validate Referrer (Primary Customer) rewards if not self.referrer_discount_type: @@ -51,6 +56,11 @@ def validate(self): frappe.throw(_("Referee Discount Amount must be greater than 0")) +# REVIEW: This factory method creates referral codes successfully, +# but should validate input more strictly at the beginning. +# Consider using `frappe.new_doc().update({...})` as a more concise approach. +# The commit() inside this function makes it unsafe for batch operations — +# consider removing commit() and let caller handle transaction boundaries. def create_referral_code(company, customer, referrer_discount_type, referrer_discount_percentage=None, referrer_discount_amount=None, referee_discount_type="Percentage", referee_discount_percentage=None, referee_discount_amount=None, campaign=None): @@ -88,6 +98,19 @@ def create_referral_code(company, customer, referrer_discount_type, referrer_dis return doc +# REVIEW: Good business logic, but function is too large and mixes responsibilities: +# - validation +# - coupon generation +# - DB writes +# RECOMMENDATIONS: +# Split responsibility into: +# validate_referral_usage() +# create_referrer_coupon() +# create_referee_coupon() +# update_referral_statistics() +# +# Also — manual commit() inside can break transactional integrity +# if called inside another transaction context. def apply_referral_code(referral_code, referee_customer): """ Apply a referral code - generates coupons for both referrer and referee @@ -197,6 +220,10 @@ def generate_referrer_coupon(referral): return coupon +# REVIEW: Similar to generate_referrer_coupon — duplicated structure. +# Suggest merging both into a single generic coupon generator: +# generate_coupon_from_referral(referral, recipient, type) +# That removes duplicated code & centralizes discount assignment logic. def generate_referee_coupon(referral, referee_customer): """Generate a Promotional coupon for the referee (new customer)""" coupon = frappe.new_doc("POS Coupon")