review of the brainwise_branding.py file

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)"""

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):
 	"""

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():
 	"""

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,13 +79,23 @@ 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
         precision = frappe.get_cached_value("System Settings", None, "currency_precision") or 3
         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)