Amjith's Odoo #3335
Amjith's Odoo #3335
Conversation
…/care into odoo-agent-create
Odoo agent create and set
* UserOdooAgent Model * Move User viewset changes to signals * Remove comment
* Odoo Requester for appointment * odoo dispense
…to vigneshhari/test-odoo
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| headers_str = " ".join([f"-H '{k}: {v}'" for k, v in headers.items()]) | ||
| data_str = f"-d '{json.dumps(data)}'" if data else "" | ||
| curl_command = f"curl -X POST {headers_str} {data_str} '{url}'" | ||
| logger.info("Equivalent curl command:\n%s", curl_command) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To address the issue, the logging of sensitive Authorization header values should be omitted, redacted, or masked. The best way is to construct the curl command without the actual credentials in the Authorization header—either by omitting the header entirely or by replacing its value with a placeholder such as <REDACTED>.
Specifically:
- In the block that constructs
headers_str, we should detect the Authorization header and replace its value with<REDACTED>or[REDACTED]. - The rest of the curl command construction remains unchanged so that debugging remains useful while sensitive information is protected.
- This change only concerns lines within the method shown, especially lines 48–51.
No changes to the rest of the functionality are required. No additional imports are necessary.
| @@ -45,7 +45,10 @@ | ||
|
|
||
| # Log curl equivalent for debugging | ||
| try: | ||
| headers_str = " ".join([f"-H '{k}: {v}'" for k, v in headers.items()]) | ||
| headers_str = " ".join([ | ||
| f"-H '{k}: {('[REDACTED]' if k.lower() == 'authorization' else v)}'" | ||
| for k, v in headers.items() | ||
| ]) | ||
| data_str = f"-d '{json.dumps(data)}'" if data else "" | ||
| curl_command = f"curl -X POST {headers_str} {data_str} '{url}'" | ||
| logger.info("Equivalent curl command:\n%s", curl_command) |
| return response_json | ||
| except requests.exceptions.RequestException as e: | ||
| logger.exception("Odoo API Error: %s", str(e)) | ||
| raise ValidationError(str(e)) from e |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix this problem, we should avoid exposing the raw content of exceptions from the requests library to end users, since these can contain sensitive information about system internals, URLs, network errors, etc. Instead, we will log the full details server-side for debugging, and raise a ValidationError with a fixed, generic error message (such as "Failed to contact Odoo server") to the API client.
Specifically, in odoo/connector/connector.py, inside the except requests.exceptions.RequestException as e: handler (lines 69-71), we should replace the raised ValidationError(str(e)) with a generic message, such as ValidationError("Failed to contact Odoo server. Please try again later."). The detailed exception is already logged with logger.exception, fulfilling the requirement to capture the stack trace for developers.
There are no new methods, imports, or definitions needed.
| @@ -68,4 +68,4 @@ | ||
| return response_json | ||
| except requests.exceptions.RequestException as e: | ||
| logger.exception("Odoo API Resonse Processing Error: %s", str(e)) | ||
| raise ValidationError(str(e)) from e | ||
| raise ValidationError("Failed to contact Odoo server. Please try again later.") from e |
…ith/odoo_module
…ith/odoo_module
…ith/odoo_module
📝 WalkthroughWalkthroughAdds an Odoo integration: new connector, Pydantic specs, resources to sync products/partners/users/invoices/payments/categories/delivery orders, signal handlers, AppConfig and model for user↔Odoo agent, ODOO_CONFIG setting, Pipfile addition, and transaction atomicity wrappers in multiple EMR viewsets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/api/viewsets/user.py (1)
109-116: Email failure should not rollback user creation.Raising
IntegrityErrorwhen password reset email fails is semantically incorrect (it's not a data integrity issue) and creates a poor user experience. If the email service is temporarily down, admins cannot create users at all. Additionally, the base class'stransaction.atomic()will catch this and rollback the entire user creation.Consider decoupling email sending from user creation:
- if not instance.has_usable_password(): - try: - mail_type = MailTypeChoices.create.value - send_password_reset_email(instance, mail_type) - except Exception as e: - raise IntegrityError( - "User creation failed due to email error." - ) from e # to fail the transaction + # Move outside transaction - call after super().perform_create() completes + if not instance.has_usable_password(): + try: + mail_type = MailTypeChoices.create.value + send_password_reset_email(instance, mail_type) + except Exception as e: + # Log error but don't fail user creation + logger.error( + f"Failed to send password reset email for user {instance.username}: {e}" + ) + # Optionally: Queue for retry via async taskAlternatively, use Celery to send the email asynchronously after user creation succeeds, ensuring resilience against transient email service failures.
♻️ Duplicate comments (2)
odoo/connector/connector.py (2)
46-53: Critical: Password logged in clear text.Line 51 logs the complete curl command including the Authorization header with the password. This is a critical security vulnerability as passwords will appear in application logs, potentially exposing credentials to anyone with log access.
Remove or redact the Authorization header before logging:
# Log curl equivalent for debugging try: + safe_headers = headers.copy() + safe_headers["Authorization"] = "Basic [REDACTED]" - headers_str = " ".join([f"-H '{k}: {v}'" for k, v in headers.items()]) + headers_str = " ".join([f"-H '{k}: {v}'" for k, v in safe_headers.items()]) data_str = f"-d '{json.dumps(data)}'" if data else "" curl_command = f"curl -X POST {headers_str} {data_str} '{url}'" logger.info("Equivalent curl command:\n%s", curl_command) except Exception as e: logger.info(e)
69-71: Exception details exposed to users.Line 71 includes
str(e)in theValidationError, which can expose stack traces and internal error details to API consumers. This is a security concern as it may leak implementation details.Provide a generic error message to users:
except requests.exceptions.RequestException as e: - logger.exception("Odoo API Resonse Processing Error: %s", str(e)) - raise ValidationError(str(e)) from e + logger.exception("Odoo API Response Processing Error") + raise ValidationError("Failed to communicate with Odoo service") from e
🧹 Nitpick comments (25)
odoo/TODO.md (1)
1-15: Consider moving TODO items to a dedicated issue tracker rather than committing to version control.While it's helpful to document these planning items, committing a TODO file to the repository is generally discouraged. These items would be better tracked in GitHub Issues, a project board, or a project management tool, where they can be assigned, prioritized, linked to PRs, and tracked over time.
That said, if these items must remain in the repository, consider the following improvements:
- Some items are phrased as open-ended questions ("Figure out...", "Checkout how...") rather than actionable tasks. Rephrase for clarity.
- Add priority levels (High/Medium/Low) or dependencies between items to clarify sequencing.
- Consider grouping related items (e.g., accounting-related tasks together).
- Add ownership or estimated effort where possible.
Example of a more structured approach:
# TODO Items ## Configuration & Setup - [ ] Make country, currency and other settings configurable (High priority) - [ ] Automate initial changes required as migrations if possible or as a script ## Data Mapping & Accounting - [ ] Store care id in invoice and use it when syncing - [ ] Understand the difference between product template and product variant - [ ] Map charge item accounts to Odoo accounts - [ ] Determine account mappings for discounts - [ ] Determine account mappings for taxes - [ ] Validate and apply rounding logic consistently ## Sync Operations - [ ] Implement invoice cancellation sync (keep both systems in sync) - [ ] Implement transaction handling in Odoo - [ ] Implement exception handling strategy - [ ] Create command/periodic task to sync invoices, products, and users ## Investigation & Documentation - [ ] Document accounting reporting and account mapping strategycare/emr/api/viewsets/inventory/product.py (2)
62-63: Redundant atomic block: base class already provides transaction safety.The
perform_createin the base class (EMRBaseViewSetviaEMRCreateMixin) already wraps operations intransaction.atomic()(see care/emr/api/viewsets/base.py lines 94-124). This nested atomic block is redundant—it simply becomes a savepoint with no additional benefit.Consider removing the explicit atomic wrapper here since the base class already guarantees atomicity:
- with transaction.atomic(): - super().perform_create(instance) + super().perform_create(instance)
66-67: Redundant atomic block: base class already provides transaction safety.The
perform_updatein the base class already wraps operations intransaction.atomic()(see care/emr/api/viewsets/base.py lines 176-195). This nested atomic block adds no value.Consider removing this wrapper:
- with transaction.atomic(): - super().perform_update(instance) + super().perform_update(instance)care/emr/api/viewsets/payment_reconciliation.py (2)
80-81: Redundant atomic block: base class already provides transaction safety.The base class's
perform_createalready executes within atransaction.atomic()block (care/emr/api/viewsets/base.py lines 94-124), making this wrapper unnecessary.- with transaction.atomic(): - super().perform_create(instance) + super().perform_create(instance)
93-94: Redundant atomic block: base class already provides transaction safety.The base class's
perform_updatealready wraps operations intransaction.atomic().- with transaction.atomic(): - super().perform_update(instance) + super().perform_update(instance)care/emr/api/viewsets/charge_item_definition.py (2)
91-92: Redundant atomic block: base class already provides transaction safety.The base class
perform_createalready wraps the save operation intransaction.atomic()(care/emr/api/viewsets/base.py lines 94-124).- with transaction.atomic(): - super().perform_create(instance) + super().perform_create(instance)
98-99: Redundant atomic block: base class already provides transaction safety.The base class
perform_updatealready ensures atomicity.- with transaction.atomic(): - super().perform_update(instance) + super().perform_update(instance)care/emr/api/viewsets/invoice.py (1)
134-172: Redundant atomic block: base class already provides transaction safety.While the
InvoiceLockcontext manager provides invoice-specific locking, the nestedtransaction.atomic()block is redundant since the base class'sperform_updatealready wraps operations atomically (care/emr/api/viewsets/base.py lines 176-195). The lock and atomic semantics are independent—you could keep the lock without the explicit atomic wrapper.Consider simplifying:
def perform_update(self, instance): with InvoiceLock(instance): - with transaction.atomic(): - old_invoice = Invoice.objects.get(id=instance.id) - if old_invoice.status != instance.status: - if instance.status in INVOICE_CANCELLED_STATUS: - raise ValidationError( - "Call the cancel invoice API to cancel the invoice" - ) - # ... rest of validation logic ... - super().perform_update(instance) + old_invoice = Invoice.objects.get(id=instance.id) + if old_invoice.status != instance.status: + if instance.status in INVOICE_CANCELLED_STATUS: + raise ValidationError( + "Call the cancel invoice API to cancel the invoice" + ) + # ... rest of validation logic ... + super().perform_update(instance) rebalance_account_task.delay(instance.account.id) return instanceodoo/models.py (1)
17-17: Consider adding a database index and uniqueness constraint.The
odoo_agent_idfield will likely be used for lookups when syncing data back from Odoo. Without a database index, these queries will perform full table scans as the table grows. Additionally, if each Odoo agent maps to exactly one Care user, consider adding a uniqueness constraint to prevent duplicate mappings.- odoo_agent_id = models.IntegerField(help_text="The ID of the agent in Odoo system") + odoo_agent_id = models.IntegerField( + help_text="The ID of the agent in Odoo system", + db_index=True, + unique=True, # Add only if one-to-one mapping is required on Odoo side + )If Odoo agents can map to multiple Care users, omit
unique=Truebut keepdb_index=True.care/emr/api/viewsets/user.py (1)
85-107: Redundant atomic block: base class already provides transaction safety.The base class's
perform_createalready wraps operations intransaction.atomic()(care/emr/api/viewsets/base.py lines 94-124), making this explicit wrapper unnecessary.def perform_create(self, instance): - with transaction.atomic(): - super().perform_create(instance) - # Get or create organization with the role - org_name = instance.user_type.capitalize() - org = Organization.objects.filter( - parent__isnull=True, - name=org_name, - org_type="role", - system_generated=True, - ).first() - if not org: - org = Organization.objects.create( - name=org_name, org_type="role", system_generated=True - ) - # Add User to organization with default role - OrganizationUser.objects.create( - organization=org, - user=instance, - role=RoleModel.objects.get( - is_system=True, - name=UserTypeRoleMapping[instance.user_type].value.name, - ), - ) - - if not instance.has_usable_password(): - try: - mail_type = MailTypeChoices.create.value - send_password_reset_email(instance, mail_type) - except Exception as e: - raise IntegrityError( - "User creation failed due to email error." - ) from e # to fail the transaction + super().perform_create(instance) + # Get or create organization with the role + org_name = instance.user_type.capitalize() + org = Organization.objects.filter( + parent__isnull=True, + name=org_name, + org_type="role", + system_generated=True, + ).first() + if not org: + org = Organization.objects.create( + name=org_name, org_type="role", system_generated=True + ) + # Add User to organization with default role + OrganizationUser.objects.create( + organization=org, + user=instance, + role=RoleModel.objects.get( + is_system=True, + name=UserTypeRoleMapping[instance.user_type].value.name, + ), + ) + + if not instance.has_usable_password(): + try: + mail_type = MailTypeChoices.create.value + send_password_reset_email(instance, mail_type) + except Exception as e: + raise IntegrityError( + "User creation failed due to email error." + ) from e # to fail the transactionThough honestly, the email error handling has deeper issues (see next comment).
care/emr/api/viewsets/resource_category.py (2)
95-96: Redundant atomic block: base class already provides transaction safety.The base class's
perform_createalready wraps operations intransaction.atomic()(care/emr/api/viewsets/base.py lines 94-124).- with transaction.atomic(): - super().perform_create(instance) + super().perform_create(instance)
102-103: Redundant atomic block: base class already provides transaction safety.The base class's
perform_updatealready ensures atomicity (care/emr/api/viewsets/base.py lines 176-195).- with transaction.atomic(): - super().perform_update(instance) + super().perform_update(instance)odoo/resource/product_category/spec.py (1)
4-7: Consider adding validation for UUID fields.The
parent_x_care_idandx_care_idfields appear to hold UUID values based on their usage inodoo/resource/product_category/category.py. Adding Pydantic field validation would help catch invalid data early. Also,parent_x_care_idis set to an empty string when there's no parent—perhapsOptional[str]would better express this intent.Example validation approach:
+from pydantic import BaseModel, Field +from typing import Optional class CategoryData(BaseModel): category_name: str - parent_x_care_id: str - x_care_id: str + parent_x_care_id: Optional[str] = Field(None, description="Parent category UUID or None") + x_care_id: str = Field(..., description="Category UUID")odoo/resource/res_user/spec.py (1)
13-20: Consider adding field validation for email and phone.The
phonefields lack format validation. Pydantic'sEmailStrtype could ensure email validity, and a regex pattern could validate phone numbers. This would catch data quality issues before sending to Odoo.-from pydantic import BaseModel +from pydantic import BaseModel, EmailStr, Field class UserData(BaseModel): name: str login: str - email: str + email: EmailStr user_type: UserType - phone: str + phone: str = Field(..., pattern=r"^\+?[0-9\s\-\(\)]+$") state: str partner_data: PartnerDataodoo/resource/res_partner/resource.py (1)
7-34: Missing error handling for API failures.The method doesn't handle exceptions from
OdooConnector.call_api, which can raiseValidationErrorperodoo/connector/connector.pylines 69-71. If the API call fails or returns an unexpected response structure, the sync silently returnsNone. Callers might not know whether the sync succeeded or failed.Consider logging errors and potentially re-raising them:
+import logging + +logger = logging.getLogger(__name__) + class OdooPartnerResource: def sync_partner_to_odoo_api(self, organization: Organization) -> int | None: """ Synchronize an organization to Odoo as a partner. Args: organization: Organization instance Returns: Odoo partner ID if successful, None otherwise """ # Extract contact information from metadata metadata = organization.metadata or {} email = metadata.get("email", "") phone = metadata.get("phone", "") state = metadata.get("state", "kerala") data = PartnerData( name=organization.name, x_care_id=str(organization.external_id), email=email, phone=phone, state=state, partner_type=PartnerType.company, agent=False, ).model_dump() - response = OdooConnector.call_api("api/add/partner", data) - return response.get("partner", {}).get("id") + try: + response = OdooConnector.call_api("api/add/partner", data) + partner_id = response.get("partner", {}).get("id") + if partner_id is None: + logger.warning("Odoo sync returned no partner ID for org %s", organization.external_id) + return partner_id + except Exception as e: + logger.exception("Failed to sync partner to Odoo for org %s", organization.external_id) + return Noneodoo/apps.py (1)
9-10: Specify the exact error code in the noqa directive.The blanket
# noqais flagged by Ruff. Since you're importing for side effects (signal registration), use# noqa: F401to specifically suppress the "unused import" warning. This makes the intent clearer.def ready(self): - import odoo.signals # noqa + import odoo.signals # noqa: F401odoo/resource/product_category/category.py (1)
6-25: Missing error handling for API failures.Like the partner resource, this method doesn't handle exceptions from
OdooConnector.call_api. Silent failures make debugging difficult and leave callers uncertain about sync status. Consider adding try-except blocks and logging as shown in the partner resource review.odoo/connector/connector.py (2)
52-53: Avoid catching bare Exception.Line 52 catches all exceptions with
Exception, which can hide unexpected errors. If the curl logging fails, it's probably acceptable to skip it, but catching specific exceptions likeTypeErrororValueErrorwould be more precise.- except Exception as e: - logger.info(e) + except (TypeError, ValueError) as e: + logger.debug("Failed to generate curl command: %s", e)
68-71: Follow try-else pattern for clarity.The
return response_jsonstatement (Line 68) should be in anelseblock to make the control flow explicit: successful responses return, errors raise. Also,logger.exceptionalready includes exception info, so passingstr(e)to the message is redundant (Line 70).try: response = requests.post(url, headers=headers, json=data, timeout=30) logger.info("Odoo API Response Status: %s", response.status_code) logger.info("Odoo API Raw Response: %s", response.text) response_json = response.json() logger.info("Odoo API Response JSON: %s", response_json) if not response.ok: error_msg = response_json.get("message", str(response.reason)) logger.exception("Odoo API Response Error: %s", error_msg) response.raise_for_status() # This will raise HTTPError with proper status code - - return response_json + else: + return response_json except requests.exceptions.RequestException as e: - logger.exception("Odoo API Resonse Processing Error: %s", str(e)) + logger.exception("Odoo API Response Processing Error") raise ValidationError("Failed to communicate with Odoo service") from eNote: Also fixed the typo "Resonse" → "Response" in the log message.
odoo/resource/res_user/resource.py (1)
13-46: Consider deriving state from user model and add error handling.A few observations that might warrant attention:
- The state is hardcoded to "kerala" (lines 29, 41), but the User model has state, district, and local_body fields that could provide the actual location.
- The API call lacks error handling, so Odoo failures would propagate as unhandled exceptions and potentially roll back the user save transaction.
- The method is called on every save via signals (not just creation), which could cause duplicate API calls.
Consider this approach:
def sync_user_to_odoo_api(self, user) -> int | None: """ Synchronize a user to Odoo. Args: user: User instance Returns: Odoo user ID if successful, None otherwise """ + try: + state = user.state.name.lower() if user.state else "kerala" + except AttributeError: + state = "kerala" + # Create partner data first partner_data = PartnerData( name=self.get_full_name(user), x_care_id=str(user.external_id), partner_type=PartnerType.person, phone=user.phone_number, - state="kerala", # Default to Kerala + state=state, email=user.email, agent=True, ) # Create user data data = UserData( name=self.get_full_name(user), login=user.username, email=user.email, user_type=UserType.internal, # Default to internal user phone=user.phone_number, - state="kerala", # Default to Kerala + state=state, partner_data=partner_data, ).model_dump() - response = OdooConnector.call_api("api/add/user", data) - return response.get("user", {}).get("id") + try: + response = OdooConnector.call_api("api/add/user", data) + return response.get("user", {}).get("id") + except Exception as e: + logger.exception("Failed to sync user %s to Odoo: %s", user.external_id, e) + return Noneodoo/signals.py (2)
72-79: Consider adding conditional logic to avoid unnecessary syncs.The handler fires on every ChargeItemDefinition save without checking if relevant fields changed. This could result in redundant Odoo API calls.
Consider tracking whether sync-relevant fields changed:
if created or (update_fields and any(field in update_fields for field in ['title', 'price_components', 'category'])): # sync to Odoo
117-124: Consider adding null check and transaction handling.The handler calls
sync_product_from_product_model, which returns None if the product lacks a charge_item_definition. Consider adding a conditional check here to avoid the function call entirely.@receiver(post_save, sender=Product) def sync_product_to_odoo(sender, instance, created, **kwargs): """ Signal handler to sync product to Odoo when it has a charge item definition. """ + if not instance.charge_item_definition: + return odoo_product = OdooProductProductResource() odoo_product.sync_product_from_product_model(instance)odoo/resource/product_product/resource.py (1)
86-106: LGTM with a minor suggestion.The early return for missing charge_item_definition is appropriate. Consider adding a similar null check for product_knowledge before accessing its alternate_identifier.
hsn = ( - product.product_knowledge.alternate_identifier - if product.product_knowledge - and product.product_knowledge.alternate_identifier + product.product_knowledge.alternate_identifier or "" + if product.product_knowledge else "" )odoo/resource/account_move/spec.py (2)
26-33: Consider type improvements for dates and reason field.A few observations on the API design:
invoice_dateanddue_dateare strings rather than date types, which means callers must manually format dates and lose type safety- The
reasonfield is required but both usages in the codebase pass an empty string"", suggesting it should be optional with a default valueConsider these refinements:
+from datetime import date + class AccountMoveApiRequest(BaseModel): x_care_id: str bill_type: BillType - invoice_date: str - due_date: str + invoice_date: date | str # Accept both for flexibility + due_date: date | str partner_data: PartnerData invoice_items: list[InvoiceItem] - reason: str + reason: str = "" # Optional with defaultThough if the Odoo API specifically expects string dates in "dd-mm-yyyy" format, the current approach is reasonable.
9-10: Remove the unusedAgentDataclass.The verification confirms
AgentDatais not referenced anywhere in the codebase. Sinceagent_idinInvoiceItemis already a plain string field, theAgentDataclass serves no purpose and should be removed from lines 9-10.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Pipfile(1 hunks)care/emr/api/viewsets/charge_item_definition.py(2 hunks)care/emr/api/viewsets/inventory/delivery_order.py(2 hunks)care/emr/api/viewsets/inventory/product.py(2 hunks)care/emr/api/viewsets/invoice.py(1 hunks)care/emr/api/viewsets/organization.py(1 hunks)care/emr/api/viewsets/payment_reconciliation.py(3 hunks)care/emr/api/viewsets/resource_category.py(2 hunks)care/emr/api/viewsets/user.py(1 hunks)config/settings/base.py(2 hunks)odoo/TODO.md(1 hunks)odoo/apps.py(1 hunks)odoo/connector/connector.py(1 hunks)odoo/models.py(1 hunks)odoo/resource/account_move/delivery_order.py(1 hunks)odoo/resource/account_move/invoice.py(1 hunks)odoo/resource/account_move/spec.py(1 hunks)odoo/resource/account_move_payment/payment.py(1 hunks)odoo/resource/account_move_payment/spec.py(1 hunks)odoo/resource/product_category/category.py(1 hunks)odoo/resource/product_category/spec.py(1 hunks)odoo/resource/product_product/resource.py(1 hunks)odoo/resource/product_product/spec.py(1 hunks)odoo/resource/res_partner/resource.py(1 hunks)odoo/resource/res_partner/spec.py(1 hunks)odoo/resource/res_user/resource.py(1 hunks)odoo/resource/res_user/spec.py(1 hunks)odoo/signals.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/charge_item_definition.pyodoo/apps.pyodoo/resource/res_partner/spec.pyconfig/settings/base.pyodoo/resource/res_user/spec.pyodoo/connector/connector.pyodoo/resource/account_move_payment/payment.pyodoo/resource/res_partner/resource.pyodoo/resource/product_product/spec.pycare/emr/api/viewsets/invoice.pyodoo/resource/product_product/resource.pycare/emr/api/viewsets/resource_category.pyodoo/resource/product_category/spec.pyodoo/signals.pycare/emr/api/viewsets/inventory/product.pyodoo/resource/product_category/category.pyodoo/resource/account_move/delivery_order.pycare/emr/api/viewsets/organization.pyodoo/resource/account_move_payment/spec.pyodoo/resource/account_move/invoice.pycare/emr/api/viewsets/user.pycare/emr/api/viewsets/payment_reconciliation.pycare/emr/api/viewsets/inventory/delivery_order.pyodoo/resource/res_user/resource.pyodoo/models.pyodoo/resource/account_move/spec.py
**/signals.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django signals to decouple error handling and logging from core business logic.
Files:
odoo/signals.py
**/{models,views,forms}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in user model and authentication framework for user management.
Files:
odoo/models.py
**/{forms,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django's validation framework to validate form and model data.
Files:
odoo/models.py
**/{views,models,tasks}.py
📄 CodeRabbit inference engine (.cursorrules)
Prefer try-except blocks for handling exceptions in business logic and views.
Files:
odoo/models.py
**/{models,forms,views}.py
📄 CodeRabbit inference engine (.cursorrules)
Keep business logic in models and forms; keep views light and focused on request handling.
Files:
odoo/models.py
**/{views,models}*.py
📄 CodeRabbit inference engine (.cursorrules)
**/{views,models}*.py: Leverage Django’s caching framework to optimize performance for frequently accessed data.
Use Django’s cache framework with backend support (e.g., Redis or Memcached) to reduce database load.
Files:
odoo/models.py
**/{views,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Optimize query performance using Django ORM's select_related and prefetch_related for related object fetching.
Files:
odoo/models.py
**/models.py
📄 CodeRabbit inference engine (.cursorrules)
Implement database indexing and query optimization techniques for better performance.
Files:
odoo/models.py
🧠 Learnings (2)
📚 Learning: 2025-07-18T15:17:40.483Z
Learnt from: CR
Repo: ohcnetwork/care PR: 0
File: .cursorrules:0-0
Timestamp: 2025-07-18T15:17:40.483Z
Learning: Applies to **/signals.py : Use Django signals to decouple error handling and logging from core business logic.
Applied to files:
odoo/signals.py
📚 Learning: 2024-11-28T06:13:52.067Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/bed.py:62-65
Timestamp: 2024-11-28T06:13:52.067Z
Learning: In `care/facility/models/bed.py`, when deleting a `Bed` instance in the `delete` method, if we're only adding validation and performing a single update, adding an atomic transaction is not necessary.
Applied to files:
care/emr/api/viewsets/inventory/delivery_order.py
🧬 Code graph analysis (20)
care/emr/api/viewsets/charge_item_definition.py (5)
care/emr/api/viewsets/inventory/product.py (2)
perform_create(49-63)perform_update(65-67)care/emr/api/viewsets/resource_category.py (2)
perform_create(90-96)perform_update(98-103)care/emr/api/viewsets/base.py (2)
perform_create(95-125)perform_update(177-196)care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/base.py (1)
calculate_slug_from_facility(39-40)
odoo/resource/res_user/spec.py (1)
odoo/resource/res_partner/spec.py (1)
PartnerData(11-19)
odoo/resource/account_move_payment/payment.py (5)
care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/resources/payment_reconciliation/spec.py (1)
PaymentReconciliationPaymentMethodOptions(48-55)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move_payment/spec.py (6)
AccountMovePaymentApiRequest(29-38)AccountPaymentCancelApiRequest(41-43)BillCounterData(23-26)CustomerType(18-20)JournalType(8-10)PaymentMode(13-15)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
odoo/resource/res_partner/resource.py (3)
care/emr/models/organization.py (1)
Organization(106-107)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
odoo/resource/product_product/spec.py (1)
odoo/resource/product_category/spec.py (1)
CategoryData(4-7)
care/emr/api/viewsets/invoice.py (5)
care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/resources/invoice/spec.py (1)
InvoiceStatusOptions(15-20)care/emr/models/charge_item.py (1)
ChargeItem(7-47)care/emr/resources/charge_item/spec.py (1)
ChargeItemStatusOptions(20-27)care/emr/api/viewsets/payment_reconciliation.py (1)
perform_update(84-95)
odoo/resource/product_product/resource.py (6)
care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/product.py (1)
Product(6-17)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (2)
ProductData(11-18)TaxData(6-8)
care/emr/api/viewsets/resource_category.py (4)
care/emr/api/viewsets/charge_item_definition.py (2)
perform_create(86-92)perform_update(94-99)care/emr/api/viewsets/base.py (2)
perform_create(95-125)perform_update(177-196)care/emr/models/resource_category.py (1)
ResourceCategory(10-86)care/emr/models/base.py (1)
calculate_slug_from_facility(39-40)
odoo/signals.py (19)
care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/models/product.py (1)
Product(6-17)care/emr/models/resource_category.py (1)
ResourceCategory(10-86)care/emr/models/supply_delivery.py (1)
DeliveryOrder(29-48)care/emr/resources/inventory/supply_delivery/delivery_order.py (1)
SupplyDeliveryOrderStatusOptions(18-24)care/emr/resources/invoice/spec.py (1)
InvoiceStatusOptions(15-20)care/emr/resources/organization/spec.py (1)
OrganizationTypeChoices(11-15)care/emr/resources/payment_reconciliation/spec.py (1)
PaymentReconciliationStatusOptions(22-26)care/emr/resources/resource_category/spec.py (1)
ResourceCategoryResourceTypeOptions(10-13)care/users/models.py (1)
User(196-436)odoo/resource/account_move/delivery_order.py (2)
OdooDeliveryOrderResource(21-148)sync_delivery_order_to_odoo_api(46-148)odoo/resource/account_move/invoice.py (3)
OdooInvoiceResource(24-180)sync_invoice_to_odoo_api(51-157)sync_invoice_return_to_odoo_api(159-180)odoo/resource/account_move_payment/payment.py (3)
OdooPaymentResource(21-95)sync_payment_to_odoo_api(22-73)sync_payment_cancel_to_odoo_api(75-95)odoo/resource/product_category/category.py (2)
OdooCategoryResource(5-25)sync_category_to_odoo_api(6-25)odoo/resource/product_product/resource.py (3)
OdooProductProductResource(9-106)sync_product_to_odoo_api(43-84)sync_product_from_product_model(86-106)odoo/resource/res_partner/resource.py (2)
OdooPartnerResource(6-34)sync_partner_to_odoo_api(7-34)odoo/resource/res_user/resource.py (2)
OdooUserResource(7-46)sync_user_to_odoo_api(13-46)
care/emr/api/viewsets/inventory/product.py (3)
care/emr/api/viewsets/charge_item_definition.py (2)
perform_create(86-92)perform_update(94-99)care/emr/api/viewsets/inventory/delivery_order.py (2)
perform_create(86-95)perform_update(97-99)care/emr/api/viewsets/base.py (2)
perform_create(95-125)perform_update(177-196)
odoo/resource/product_category/category.py (2)
odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)
odoo/resource/account_move/delivery_order.py (8)
care/emr/models/supply_delivery.py (2)
DeliveryOrder(29-48)SupplyDelivery(7-26)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)care/emr/resources/inventory/supply_delivery/spec.py (1)
SupplyDeliveryStatusOptions(18-22)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move/spec.py (3)
AccountMoveApiRequest(26-33)BillType(21-23)InvoiceItem(13-18)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (1)
ProductData(11-18)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
care/emr/api/viewsets/organization.py (2)
care/emr/api/viewsets/user.py (1)
perform_create(84-116)care/emr/api/viewsets/base.py (2)
perform_create(95-125)perform_update(177-196)
odoo/resource/account_move_payment/spec.py (1)
odoo/resource/res_partner/spec.py (1)
PartnerData(11-19)
odoo/resource/account_move/invoice.py (12)
care/emr/models/charge_item.py (1)
ChargeItem(7-47)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/medication_dispense.py (1)
MedicationDispense(8-34)care/emr/models/scheduling/booking.py (1)
TokenBooking(22-50)care/emr/models/service_request.py (1)
ServiceRequest(7-46)care/emr/resources/charge_item/spec.py (1)
ChargeItemResourceOptions(37-41)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move/spec.py (4)
AccountMoveApiRequest(26-33)AccountMoveReturnApiRequest(36-43)BillType(21-23)InvoiceItem(13-18)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (1)
ProductData(11-18)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
care/emr/api/viewsets/payment_reconciliation.py (3)
care/emr/api/viewsets/charge_item_definition.py (2)
perform_create(86-92)perform_update(94-99)care/emr/api/viewsets/inventory/product.py (2)
perform_create(49-63)perform_update(65-67)care/emr/api/viewsets/invoice.py (2)
perform_create(91-114)perform_update(132-174)
care/emr/api/viewsets/inventory/delivery_order.py (5)
care/emr/api/viewsets/charge_item_definition.py (2)
perform_create(86-92)perform_update(94-99)care/emr/api/viewsets/inventory/product.py (2)
perform_create(49-63)perform_update(65-67)care/emr/api/viewsets/inventory/request_order.py (1)
perform_create(89-97)care/emr/api/viewsets/inventory/supply_delivery.py (2)
perform_create(101-111)perform_update(122-138)care/emr/api/viewsets/medication_dispense.py (2)
perform_create(82-110)perform_update(151-174)
odoo/resource/res_user/resource.py (4)
care/users/models.py (1)
User(196-436)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)odoo/resource/res_user/spec.py (2)
UserData(13-20)UserType(8-10)
odoo/models.py (1)
odoo/resource/res_user/resource.py (1)
get_full_name(8-11)
odoo/resource/account_move/spec.py (2)
odoo/resource/product_product/spec.py (1)
ProductData(11-18)odoo/resource/res_partner/spec.py (1)
PartnerData(11-19)
🪛 GitHub Check: CodeQL
odoo/connector/connector.py
[failure] 51-51: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
[warning] 71-71: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.3)
odoo/apps.py
10-10: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
odoo/connector/connector.py
52-52: Do not catch blind exception: Exception
(BLE001)
68-68: Consider moving this statement to an else block
(TRY300)
70-70: Redundant exception object included in logging.exception call
(TRY401)
care/emr/api/viewsets/invoice.py
138-140: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
157-159: Avoid specifying long messages outside the exception class
(TRY003)
odoo/resource/product_product/resource.py
14-14: Create your own exception
(TRY002)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
odoo/signals.py
36-36: Unused function argument: sender
(ARG001)
36-36: Unused function argument: created
(ARG001)
36-36: Unused function argument: kwargs
(ARG001)
45-45: Unused function argument: sender
(ARG001)
45-45: Unused function argument: raw
(ARG001)
45-45: Unused function argument: using
(ARG001)
45-45: Unused function argument: update_fields
(ARG001)
45-45: Unused function argument: kwargs
(ARG001)
57-57: Unused function argument: sender
(ARG001)
57-57: Unused function argument: created
(ARG001)
57-57: Unused function argument: kwargs
(ARG001)
73-73: Unused function argument: sender
(ARG001)
73-73: Unused function argument: created
(ARG001)
73-73: Unused function argument: kwargs
(ARG001)
82-82: Unused function argument: sender
(ARG001)
82-82: Unused function argument: created
(ARG001)
82-82: Unused function argument: kwargs
(ARG001)
95-95: Unused function argument: sender
(ARG001)
95-95: Unused function argument: created
(ARG001)
95-95: Unused function argument: kwargs
(ARG001)
105-105: Unused function argument: sender
(ARG001)
105-105: Unused function argument: created
(ARG001)
105-105: Unused function argument: kwargs
(ARG001)
118-118: Unused function argument: sender
(ARG001)
118-118: Unused function argument: created
(ARG001)
118-118: Unused function argument: kwargs
(ARG001)
odoo/resource/account_move/invoice.py
29-29: Create your own exception
(TRY002)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (16)
care/emr/api/viewsets/payment_reconciliation.py (1)
82-82: Good: Async task triggered after transaction.The
rebalance_account_task.delay()call is correctly placed outside the atomic block, ensuring the task runs only after the transaction commits.care/emr/api/viewsets/invoice.py (1)
173-173: Good: Async task triggered after transaction.The
rebalance_account_task.delay()call is correctly placed outside the atomic block.odoo/models.py (1)
11-16: Verify CASCADE deletion behavior.Using
on_delete=models.CASCADEmeans deleting a user will also delete their Odoo agent mapping. This might be the intended behavior, but consider whether you need to preserve the mapping for audit trails or if you should useon_delete=models.SET_NULLwith a nullable foreign key to retain historical records.Confirm that deleting users should also remove their Odoo agent mappings, or if soft-deletion/preservation is needed.
Pipfile (1)
52-52: No action needed—version is current and secure.The pinned version
0.10.1is already the latest stable release on PyPI with no known security vulnerabilities, so the codebase is in good shape.care/emr/api/viewsets/inventory/delivery_order.py (1)
94-99: Verify if nested transaction wrapping is intentional.Similar to
OrganizationViewSet, wrapping these methods intransaction.atomic()might be redundant if the base class already provides transactional safety. Review the base class implementation to confirm whether this additional wrapping is necessary for Odoo sync signal coordination.Based on learnings: In simpler cases with single updates, atomic transactions may not be necessary, though here you're calling
super()which might perform multiple operations.odoo/resource/res_partner/spec.py (1)
1-19: LGTM!The data models are clean and straightforward. The use of Pydantic for validation and the enum for partner type are appropriate choices for external API integration.
config/settings/base.py (1)
142-142: LGTM!The Odoo app is correctly added to LOCAL_APPS.
odoo/resource/res_user/resource.py (1)
8-11: LGTM!The full name construction correctly handles optional name components and provides sensible fallbacks.
odoo/resource/product_product/spec.py (1)
1-18: LGTM!The data models are well-structured and appropriately reference CategoryData. The optional fields for taxes and HSN code provide flexibility for different product types.
odoo/signals.py (3)
81-92: LGTM with transaction caveat.The conditional check for charge_item_definition resource type is appropriate. However, the same transaction boundary concern applies here as with other handlers.
94-102: LGTM with transaction caveat.The conditional check for product_supplier org type is appropriate. However, consider applying the transaction.on_commit pattern here as well.
104-115: LGTM with transaction caveat.The conditional logic checking both status and origin is appropriate for avoiding unnecessary syncs. The same transaction boundary concern applies.
odoo/resource/account_move_payment/spec.py (1)
1-43: LGTM!The data models are well-structured with appropriate enums and type annotations. The composition of PartnerData and BillCounterData within AccountMovePaymentApiRequest is clean.
odoo/resource/account_move/delivery_order.py (1)
22-30: LGTM on the helper method.The logic correctly extracts the base price from price components with appropriate fallback handling.
odoo/resource/account_move/spec.py (2)
13-18: String types for quantity and sale_price align with API expectations.While it might seem unusual to use strings for numeric fields, this matches how the values are prepared in the invoice and delivery_order resources (using
str()conversions everywhere). This appears to be an intentional design choice for Odoo API compatibility.
36-43: LGTM on the return request structure.The design appropriately makes most fields optional since a return/cancellation primarily needs just the care ID and reason. This provides flexibility while keeping the common case simple.
| def perform_create(self, instance): | ||
| with transaction.atomic(): | ||
| super().perform_create(instance) | ||
|
|
||
| def perform_update(self, instance): | ||
| with transaction.atomic(): | ||
| super().perform_update(instance) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if nested transaction wrapping is intentional.
The base class EMRModelViewSet already wraps perform_create and perform_update in transaction.atomic() blocks (see care/emr/api/viewsets/base.py lines 94-124 and 176-195). Adding another transaction.atomic() here creates nested transactions, which Django handles as savepoints. While not harmful, it's likely redundant unless specifically intended for Odoo signal handler isolation.
Run the following script to confirm the base class transaction wrapping:
🏁 Script executed:
#!/bin/bash
# Verify base class transaction handling
ast-grep --pattern $'class EMRModelViewSet($$$) {
$$$
def perform_create($$$) {
$$$
with transaction.atomic():
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for perform_create and perform_update in base viewset with context
rg -n "def perform_create|def perform_update" care/emr/api/viewsets/base.py -A 5 -B 2Length of output: 740
🏁 Script executed:
#!/bin/bash
# Verify the actual organization.py implementation
cat care/emr/api/viewsets/organization.py | sed -n '150,170p'Length of output: 688
🏁 Script executed:
#!/bin/bash
# Check if transaction.atomic is imported in both files
rg -n "from django.db import transaction|import transaction" care/emr/api/viewsets/organization.py care/emr/api/viewsets/base.pyLength of output: 139
Remove redundant transaction wrapping in perform_create and perform_update.
The base class methods already wrap operations in transaction.atomic() (base.py lines 98 and 180). Wrapping the super() calls again creates unnecessary nested transactions—Django converts these to savepoints, which adds no atomicity benefit. Simply call super().perform_create(instance) and super().perform_update(instance) directly without the additional with transaction.atomic(): block.
Note: perform_destroy can retain its transaction.atomic() wrapper since it contains custom logic beyond a simple super() call.
🤖 Prompt for AI Agents
In care/emr/api/viewsets/organization.py around lines 156 to 162, the
perform_create and perform_update methods are redundantly wrapping super() calls
in transaction.atomic() even though the base class already provides atomic
coverage; remove the with transaction.atomic(): blocks and call
super().perform_create(instance) and super().perform_update(instance) directly
so you avoid unnecessary nested transactions/savepoints while keeping
perform_destroy unchanged.
| ODOO_CONFIG = { | ||
| "host": env("ODOO_HOST", default="host.docker.internal"), # Odoo instance URL | ||
| "port": env("ODOO_PORT", default=8069), | ||
| "protocol": env("ODOO_PROTOCOL", default="jsonrpc"), | ||
| "database": env("ODOO_DATABASE", default="ohc"), # Odoo database name | ||
| "username": env("ODOO_USERNAME", default="vignesh@ohc.network"), # Odoo username | ||
| "password": env( | ||
| "ODOO_PASSWORD", default="9fe380242cf3a95d23b4169ef79167fd4c6640c6" | ||
| ), | ||
| } |
There was a problem hiding this comment.
Remove hardcoded credentials from default values.
The default password value appears to be an actual API token. Even for development environments, hardcoded credentials in source code pose a security risk and could be accidentally committed or exposed.
Apply this diff to remove the sensitive default:
ODOO_CONFIG = {
"host": env("ODOO_HOST", default="host.docker.internal"), # Odoo instance URL
"port": env("ODOO_PORT", default=8069),
"protocol": env("ODOO_PROTOCOL", default="jsonrpc"),
"database": env("ODOO_DATABASE", default="ohc"), # Odoo database name
- "username": env("ODOO_USERNAME", default="vignesh@ohc.network"), # Odoo username
- "password": env(
- "ODOO_PASSWORD", default="9fe380242cf3a95d23b4169ef79167fd4c6640c6"
- ),
+ "username": env("ODOO_USERNAME", default=""), # Odoo username
+ "password": env("ODOO_PASSWORD", default=""),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ODOO_CONFIG = { | |
| "host": env("ODOO_HOST", default="host.docker.internal"), # Odoo instance URL | |
| "port": env("ODOO_PORT", default=8069), | |
| "protocol": env("ODOO_PROTOCOL", default="jsonrpc"), | |
| "database": env("ODOO_DATABASE", default="ohc"), # Odoo database name | |
| "username": env("ODOO_USERNAME", default="vignesh@ohc.network"), # Odoo username | |
| "password": env( | |
| "ODOO_PASSWORD", default="9fe380242cf3a95d23b4169ef79167fd4c6640c6" | |
| ), | |
| } | |
| ODOO_CONFIG = { | |
| "host": env("ODOO_HOST", default="host.docker.internal"), # Odoo instance URL | |
| "port": env("ODOO_PORT", default=8069), | |
| "protocol": env("ODOO_PROTOCOL", default="jsonrpc"), | |
| "database": env("ODOO_DATABASE", default="ohc"), # Odoo database name | |
| "username": env("ODOO_USERNAME", default=""), # Odoo username | |
| "password": env("ODOO_PASSWORD", default=""), | |
| } |
🤖 Prompt for AI Agents
In config/settings/base.py around lines 755 to 764, the ODOO_CONFIG contains a
hardcoded password default which is sensitive; remove the hardcoded default
value for "password" (do not insert any secrets into source), change
env("ODOO_PASSWORD", default="...") to either env("ODOO_PASSWORD") so it becomes
required or env("ODOO_PASSWORD", default="") to default to an empty/placeholder
value, and update any related code or docs to handle missing credentials (fail
fast with a clear error or instruct to set the env var in dev/prod).
| def sync_payment_to_odoo_api(self, payment_id: str) -> int | None: | ||
| """ | ||
| Synchronize a Django payment reconciliation to Odoo using the custom addon API. | ||
|
|
||
| Args: | ||
| payment_id: External ID of the Django payment reconciliation | ||
|
|
||
| Returns: | ||
| Odoo payment ID if successful, None otherwise | ||
| """ | ||
| payment = PaymentReconciliation.objects.select_related( | ||
| "facility", "account", "target_invoice" | ||
| ).get(external_id=payment_id) | ||
|
|
||
| # Prepare partner data | ||
| partner_data = PartnerData( | ||
| name=payment.account.patient.name, | ||
| x_care_id=str(payment.account.patient.external_id), | ||
| partner_type=PartnerType.person, | ||
| phone=payment.account.patient.phone_number, | ||
| state=payment.facility.state or "kerala", | ||
| email="", | ||
| agent=False, | ||
| ) | ||
|
|
||
| # Prepare payment data | ||
| data = AccountMovePaymentApiRequest( | ||
| journal_x_care_id=str( | ||
| payment.target_invoice.external_id if payment.target_invoice else "" | ||
| ), | ||
| x_care_id=str(payment.external_id), | ||
| amount=float(payment.amount), | ||
| journal_input=JournalType.cash | ||
| if payment.method == PaymentReconciliationPaymentMethodOptions.cash.value | ||
| else JournalType.bank, | ||
| payment_date=payment.payment_datetime.strftime("%Y-%m-%d"), | ||
| payment_mode=PaymentMode.send | ||
| if payment.is_credit_note | ||
| else PaymentMode.receive, | ||
| partner_data=partner_data, | ||
| customer_type=CustomerType.customer, | ||
| counter_data=BillCounterData( | ||
| x_care_id=str(payment.location.external_id), | ||
| cashier_id=str(payment.created_by.external_id), | ||
| counter_name=payment.location.name, | ||
| ), | ||
| ).model_dump() | ||
|
|
||
| logger.info("Odoo Payment Data: %s", data) | ||
|
|
||
| response = OdooConnector.call_api("api/account/move/payment", data) | ||
| return response["payment"]["id"] |
There was a problem hiding this comment.
Add null checks for location and created_by.
Several fields accessed without null checks:
- Line 64:
payment.location.external_id- location is a nullable ForeignKey, so this will raise AttributeError if None. - Line 65:
payment.created_by.external_id- though EMRBaseModel likely sets this, it should be verified. - Line 73: Direct dictionary access
response["payment"]["id"]could raise KeyError if the response structure differs.
Apply this diff:
def sync_payment_to_odoo_api(self, payment_id: str) -> int | None:
"""
Synchronize a Django payment reconciliation to Odoo using the custom addon API.
Args:
payment_id: External ID of the Django payment reconciliation
Returns:
Odoo payment ID if successful, None otherwise
"""
payment = PaymentReconciliation.objects.select_related(
"facility", "account", "target_invoice"
).get(external_id=payment_id)
+ if not payment.location or not payment.created_by:
+ logger.warning(
+ "Cannot sync payment %s to Odoo: missing location or creator",
+ payment_id
+ )
+ return None
+
# Prepare partner data
partner_data = PartnerData(
name=payment.account.patient.name,
x_care_id=str(payment.account.patient.external_id),
partner_type=PartnerType.person,
phone=payment.account.patient.phone_number,
state=payment.facility.state or "kerala",
email="",
agent=False,
)
# Prepare payment data
data = AccountMovePaymentApiRequest(
journal_x_care_id=str(
payment.target_invoice.external_id if payment.target_invoice else ""
),
x_care_id=str(payment.external_id),
amount=float(payment.amount),
journal_input=JournalType.cash
if payment.method == PaymentReconciliationPaymentMethodOptions.cash.value
else JournalType.bank,
payment_date=payment.payment_datetime.strftime("%Y-%m-%d"),
payment_mode=PaymentMode.send
if payment.is_credit_note
else PaymentMode.receive,
partner_data=partner_data,
customer_type=CustomerType.customer,
counter_data=BillCounterData(
x_care_id=str(payment.location.external_id),
cashier_id=str(payment.created_by.external_id),
counter_name=payment.location.name,
),
).model_dump()
logger.info("Odoo Payment Data: %s", data)
- response = OdooConnector.call_api("api/account/move/payment", data)
- return response["payment"]["id"]
+ try:
+ response = OdooConnector.call_api("api/account/move/payment", data)
+ return response.get("payment", {}).get("id")
+ except Exception as e:
+ logger.exception("Failed to sync payment %s to Odoo: %s", payment_id, e)
+ return None🤖 Prompt for AI Agents
In odoo/resource/account_move_payment/payment.py around lines 22 to 73, the code
assumes payment.location and payment.created_by are always present and directly
indexes response["payment"]["id"]; add null checks and safe fallbacks: when
building BillCounterData use conditional values (e.g.,
str(payment.location.external_id) if payment.location else "" and
str(payment.created_by.external_id) if payment.created_by else "") and guard
counter_name with payment.location.name if present; after calling
OdooConnector.call_api validate that response is a dict and contains "payment"
and "id" (return None or raise a descriptive error if keys are missing) instead
of unguarded direct indexing.
| def sync_payment_cancel_to_odoo_api(self, payment_id: str) -> int | None: | ||
| """ | ||
| Synchronize a cancelled Django payment reconciliation to Odoo using the custom addon API. | ||
|
|
||
| Args: | ||
| payment_id: External ID of the Django payment reconciliation | ||
|
|
||
| Returns: | ||
| Odoo payment ID if successful, None otherwise | ||
| """ | ||
| payment = PaymentReconciliation.objects.get(external_id=payment_id) | ||
|
|
||
| data = AccountPaymentCancelApiRequest( | ||
| x_care_id=str(payment.external_id), | ||
| reason=payment.status, | ||
| ).model_dump() | ||
|
|
||
| logger.info("Odoo Payment Cancel Data: %s", data) | ||
|
|
||
| response = OdooConnector.call_api("api/account/move/payment/cancel", data) | ||
| return response["payment"]["id"] |
There was a problem hiding this comment.
Add error handling and use safe dictionary access.
Similar issues as the previous method:
- Line 95: Direct dictionary access could raise KeyError.
- No error handling around the API call.
Apply this diff:
def sync_payment_cancel_to_odoo_api(self, payment_id: str) -> int | None:
"""
Synchronize a cancelled Django payment reconciliation to Odoo using the custom addon API.
Args:
payment_id: External ID of the Django payment reconciliation
Returns:
Odoo payment ID if successful, None otherwise
"""
payment = PaymentReconciliation.objects.get(external_id=payment_id)
data = AccountPaymentCancelApiRequest(
x_care_id=str(payment.external_id),
reason=payment.status,
).model_dump()
logger.info("Odoo Payment Cancel Data: %s", data)
- response = OdooConnector.call_api("api/account/move/payment/cancel", data)
- return response["payment"]["id"]
+ try:
+ response = OdooConnector.call_api("api/account/move/payment/cancel", data)
+ return response.get("payment", {}).get("id")
+ except Exception as e:
+ logger.exception("Failed to cancel payment %s in Odoo: %s", payment_id, e)
+ return None🤖 Prompt for AI Agents
In odoo/resource/account_move_payment/payment.py around lines 75 to 95, the
method sync_payment_cancel_to_odoo_api lacks error handling and uses direct dict
indexing which can raise KeyError; update it to wrap the DB lookup and the
OdooConnector.call_api call in try/except blocks (catch
PaymentReconciliation.DoesNotExist and generic Exception as appropriate), log
the exception details, and return None on error, and replace
response["payment"]["id"] with safe access using .get (e.g.,
response.get("payment", {}).get("id")) or validate the structure before
returning to avoid KeyError.
| def get_product_purchase_price(self, product): | ||
| """Get purchase price from charge item definition price components""" | ||
| if not product.charge_item_definition: | ||
| return None | ||
|
|
||
| for item in product.charge_item_definition.price_components: | ||
| if ( | ||
| item["monetary_component_type"] | ||
| == MonetaryComponentType.informational.value | ||
| and item["code"]["code"] == "purchase_price" | ||
| ): | ||
| return item["amount"] | ||
| return None |
There was a problem hiding this comment.
Add defensive checks for nested dictionary access.
Line 41 accesses item["code"]["code"] without verifying that the "code" key exists in the item dictionary, which could raise a KeyError if the data structure doesn't match expectations.
Consider adding a safety check:
def get_product_purchase_price(self, product):
"""Get purchase price from charge item definition price components"""
if not product.charge_item_definition:
return None
for item in product.charge_item_definition.price_components:
if (
item["monetary_component_type"]
== MonetaryComponentType.informational.value
- and item["code"]["code"] == "purchase_price"
+ and item.get("code", {}).get("code") == "purchase_price"
):
return item["amount"]
return None🤖 Prompt for AI Agents
In odoo/resource/account_move/delivery_order.py around lines 32 to 44, the loop
accesses item["code"]["code"] (and item["amount"]) without verifying the "code"
key or nested structure; add defensive checks to avoid KeyError by first
ensuring item is a dict, that "code" exists and is a dict (or has a string
"code"), and that "amount" exists before comparing values or returning; update
the conditional to safely access nested keys (e.g., use .get and type checks)
and only return item["amount"] when present and valid.
| def sync_product_to_odoo_api( | ||
| self, charge_item_definition, hsn: str = "" | ||
| ) -> int | None: | ||
| """ | ||
| Synchronize a charge item definition to Odoo as a product. | ||
|
|
||
| Args: | ||
| charge_item_definition: ChargeItemDefinition instance | ||
|
|
||
| Returns: | ||
| Odoo product ID if successful, None otherwise | ||
| """ | ||
| base_price = self.get_charge_item_base_price(charge_item_definition) | ||
| mrp = self.get_charge_item_mrp(charge_item_definition) | ||
| purchase_price = self.get_charge_item_purchase_price(charge_item_definition) | ||
|
|
||
| taxes = [] | ||
| for tax in self.get_taxes(charge_item_definition): | ||
| taxes.append( | ||
| TaxData( | ||
| tax_name=tax["code"]["display"], | ||
| tax_percentage=float(tax["factor"]), | ||
| ) | ||
| ) | ||
| data = ProductData( | ||
| product_name=f"CARE: {charge_item_definition.title}", | ||
| x_care_id=str(charge_item_definition.external_id), | ||
| mrp=float(base_price or "0"), | ||
| cost=float(purchase_price or mrp or "0"), | ||
| category=CategoryData( | ||
| category_name=charge_item_definition.category.title, | ||
| parent_x_care_id=str(charge_item_definition.category.parent.external_id) | ||
| if charge_item_definition.category.parent | ||
| else "", | ||
| x_care_id=str(charge_item_definition.category.external_id), | ||
| ), | ||
| taxes=taxes, | ||
| hsn=hsn, | ||
| ).model_dump() | ||
|
|
||
| response = OdooConnector.call_api("api/add/product", data) | ||
| return response.get("product", {}).get("id") |
There was a problem hiding this comment.
Add null check for category and error handling.
Two concerns here:
- Line 73:
charge_item_definition.category.titlewill raise AttributeError if category is None (it's a nullable ForeignKey per the model definition). - The API call lacks error handling, so failures propagate.
Apply this diff:
def sync_product_to_odoo_api(
self, charge_item_definition, hsn: str = ""
) -> int | None:
"""
Synchronize a charge item definition to Odoo as a product.
Args:
charge_item_definition: ChargeItemDefinition instance
Returns:
Odoo product ID if successful, None otherwise
"""
+ if not charge_item_definition.category:
+ logger.warning(
+ "Cannot sync charge item %s to Odoo: missing category",
+ charge_item_definition.external_id
+ )
+ return None
+
base_price = self.get_charge_item_base_price(charge_item_definition)
mrp = self.get_charge_item_mrp(charge_item_definition)
purchase_price = self.get_charge_item_purchase_price(charge_item_definition)
taxes = []
for tax in self.get_taxes(charge_item_definition):
taxes.append(
TaxData(
tax_name=tax["code"]["display"],
tax_percentage=float(tax["factor"]),
)
)
data = ProductData(
product_name=f"CARE: {charge_item_definition.title}",
x_care_id=str(charge_item_definition.external_id),
mrp=float(base_price or "0"),
cost=float(purchase_price or mrp or "0"),
category=CategoryData(
category_name=charge_item_definition.category.title,
parent_x_care_id=str(charge_item_definition.category.parent.external_id)
if charge_item_definition.category.parent
else "",
x_care_id=str(charge_item_definition.category.external_id),
),
taxes=taxes,
hsn=hsn,
).model_dump()
- response = OdooConnector.call_api("api/add/product", data)
- return response.get("product", {}).get("id")
+ try:
+ response = OdooConnector.call_api("api/add/product", data)
+ return response.get("product", {}).get("id")
+ except Exception as e:
+ logger.exception(
+ "Failed to sync product %s to Odoo: %s",
+ charge_item_definition.external_id,
+ e
+ )
+ return NoneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In odoo/resource/product_product/resource.py around lines 43–84, avoid
AttributeError by null-checking charge_item_definition.category before accessing
.title, .parent, or .external_id: when category is None set CategoryData fields
(category_name, parent_x_care_id, x_care_id) to empty strings (or sensible
defaults) instead of dereferencing; and add error handling around the
OdooConnector.call_api call (wrap it in try/except Exception as e), log the
exception and return None on failure, and validate the response contains a
product id before returning it so API failures don’t propagate.
| metadata = organization.metadata or {} | ||
| email = metadata.get("email", "") | ||
| phone = metadata.get("phone", "") | ||
| state = metadata.get("state", "kerala") |
There was a problem hiding this comment.
Hard-coded default state value lacks context.
The hard-coded "kerala" default for state (Line 21) appears to be location-specific. If organizations can exist outside Kerala, this default could send incorrect data to Odoo. Consider making this configurable or requiring the state field in metadata.
🤖 Prompt for AI Agents
In odoo/resource/res_partner/resource.py around lines 18 to 21, the code uses a
hard-coded state default "kerala" which is location-specific; replace this by
reading a configurable default (e.g., from settings/config or environment) and
use that fallback, or validate and require that 'state' be present in
organization.metadata and raise/return an error if missing; update the codepaths
and tests to retrieve the configured default (or enforce the required metadata)
so organizations outside Kerala do not get incorrect state values.
| @receiver(post_save, sender=User) | ||
| def sync_user_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync user to Odoo when created or updated. | ||
| """ | ||
| odoo_user = OdooUserResource() | ||
| odoo_user.sync_user_to_odoo_api(instance) | ||
|
|
There was a problem hiding this comment.
Add conditional check and async task processing.
The signal handler has a couple of concerns:
- It runs on every User save (create or update), but doesn't check the
createdflag. This means updating any user field triggers an Odoo sync, which could create duplicates or unnecessary API calls. - The synchronous API call blocks the request thread and runs within the database transaction, meaning Odoo failures will roll back the user save.
Consider using a Celery task for async processing:
+from django.db import transaction
+
@receiver(post_save, sender=User)
def sync_user_to_odoo(sender, instance, created, **kwargs):
"""
Signal handler to sync user to Odoo when created or updated.
"""
- odoo_user = OdooUserResource()
- odoo_user.sync_user_to_odoo_api(instance)
+ if created:
+ transaction.on_commit(lambda: sync_user_to_odoo_task.delay(str(instance.external_id)))And define the task:
from celery import shared_task
@shared_task
def sync_user_to_odoo_task(user_id: str):
try:
from care.users.models import User
user = User.objects.get(external_id=user_id)
odoo_user = OdooUserResource()
odoo_user.sync_user_to_odoo_api(user)
except Exception as e:
logger.exception("Failed to sync user %s to Odoo", user_id)🧰 Tools
🪛 Ruff (0.14.3)
36-36: Unused function argument: sender
(ARG001)
36-36: Unused function argument: created
(ARG001)
36-36: Unused function argument: kwargs
(ARG001)
| @receiver(post_save, sender=Invoice) | ||
| def save_fields_before_update(sender, instance, raw, using, update_fields, **kwargs): | ||
| if instance.status in [ | ||
| InvoiceStatusOptions.issued.value, | ||
| ]: | ||
| odoo_integration = OdooInvoiceResource() | ||
| odoo_integration.sync_invoice_to_odoo_api(instance.external_id) | ||
| elif instance.status in INVOICE_CANCELLED_STATUS: | ||
| odoo_integration = OdooInvoiceResource() | ||
| odoo_integration.sync_invoice_return_to_odoo_api(instance.external_id) | ||
|
|
There was a problem hiding this comment.
Move Odoo API call outside transaction boundary.
The invoice sync occurs within the database transaction. If the Odoo API fails or times out, it will roll back the invoice status change, which may not be the desired behavior.
Consider deferring the API call until after the transaction commits:
+from django.db import transaction
+
@receiver(post_save, sender=Invoice)
def save_fields_before_update(sender, instance, raw, using, update_fields, **kwargs):
if instance.status in [
InvoiceStatusOptions.issued.value,
]:
- odoo_integration = OdooInvoiceResource()
- odoo_integration.sync_invoice_to_odoo_api(instance.external_id)
+ invoice_id = instance.external_id
+ transaction.on_commit(lambda: sync_invoice_task.delay(str(invoice_id), "issued"))
elif instance.status in INVOICE_CANCELLED_STATUS:
- odoo_integration = OdooInvoiceResource()
- odoo_integration.sync_invoice_return_to_odoo_api(instance.external_id)
+ invoice_id = instance.external_id
+ transaction.on_commit(lambda: sync_invoice_task.delay(str(invoice_id), "cancelled"))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
45-45: Unused function argument: sender
(ARG001)
45-45: Unused function argument: raw
(ARG001)
45-45: Unused function argument: using
(ARG001)
45-45: Unused function argument: update_fields
(ARG001)
45-45: Unused function argument: kwargs
(ARG001)
| @receiver(post_save, sender=PaymentReconciliation) | ||
| def sync_payment_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync payment reconciliation to Odoo when created. | ||
| """ | ||
| if instance.status == PaymentReconciliationStatusOptions.active.value: | ||
| odoo_payment = OdooPaymentResource() | ||
| odoo_payment.sync_payment_to_odoo_api(instance.external_id) | ||
| elif instance.status in [ | ||
| PaymentReconciliationStatusOptions.cancelled.value, | ||
| PaymentReconciliationStatusOptions.entered_in_error.value, | ||
| ]: | ||
| odoo_payment = OdooPaymentResource() | ||
| odoo_payment.sync_payment_cancel_to_odoo_api(instance.external_id) | ||
|
|
There was a problem hiding this comment.
Apply transaction.on_commit pattern for reliability.
Similar to other handlers, the synchronous Odoo API call runs within the transaction, creating a coupling between payment persistence and external API availability.
Consider the same async pattern suggested for other handlers.
🧰 Tools
🪛 Ruff (0.14.3)
57-57: Unused function argument: sender
(ARG001)
57-57: Unused function argument: created
(ARG001)
57-57: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In odoo/signals.py around lines 56 to 70, the handler currently calls Odoo API
synchronously inside the DB transaction; change it to use Django's
transaction.on_commit so the external API call runs only after the DB commit
succeeds — for created/active and for cancelled/entered_in_error, register a
callback with transaction.on_commit that instantiates OdooPaymentResource and
invokes the appropriate sync method (or dispatches the existing async task used
elsewhere) passing instance.external_id; import transaction from django.db and
ensure the callback captures only the external_id (not the full model instance)
to avoid stale data/lifecycle issues.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (10)
odoo/resource/account_move/invoice.py (6)
110-139: Batch-load related resources to avoid the N+1 avalanche.We’re still issuing individual
ServiceRequest/TokenBooking/MedicationDispenselookups per charge item. On a busy invoice this is dozens of round-trips. Please prefetch by type and reuse in-memory dictionaries.
29-29: Raise something more specific thanException.Same story as before: callers can’t react intelligently when you throw a plain
Exception. Give us a dedicated error type (or ValueError) that callers can catch.
31-48: Use safe lookups forcode["code"].Both helper methods still assume every price component has a
{"code": {"code": ...}}structure. One malformed entry triggers a KeyError. Please switch to.getchecks (and guardunit_price_componentswhen it’s None).
84-101: Handle missing categories before buildingCategoryData.
charge_item_definition.categoryis optional; when it’s absent the ternary touches.title/.parentand crashes. Mirror the fallback used elsewhere (log/return default CategoryData) before accessing those attributes.
157-158: Defend against malformed API responses.
response["invoice"]["id"]blindly indexes the payload; wrap it in safe access (and raise/log a descriptive error when missing) so we don’t crash with a KeyError mid-sync.
179-181: Same for the return flow.
response["reverse_invoice"]["id"]has the identical KeyError problem—harden it the same way.odoo/resource/product_product/resource.py (2)
14-14: Stop raising bareException.We already asked for this: callers can’t distinguish a missing base-price component when you throw a generic
Exception. Give us a dedicated error (or at least a ValueError) so the sync flow can react sanely.
68-78: Guard against missing categories before we dereference them.
charge_item_definition.categoryis nullable; hitting this code with an uncategorized item blows up on Line 73. Please short-circuit (log and return/skip) when the category is absent before touching.titleor.parent.odoo/resource/account_move/delivery_order.py (2)
37-43: Defensive access forpurchase_price, please.We’re still indexing
item["code"]["code"]blindly. One malformed component and the sync dies with a KeyError. Use.get(and maybe anisinstancecheck) so we only return amounts when the nested structure is actually there.
148-149: Don’t assume the API payload survived intact.
response["invoice"]["id"]will KeyError the moment Odoo gives us a partial or error payload. Use safe access (and log/fail gracefully) instead of indexing directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
odoo/resource/account_move/delivery_order.py(1 hunks)odoo/resource/account_move/invoice.py(1 hunks)odoo/resource/product_product/resource.py(1 hunks)odoo/resource/product_product/spec.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
odoo/resource/account_move/invoice.pyodoo/resource/product_product/spec.pyodoo/resource/account_move/delivery_order.pyodoo/resource/product_product/resource.py
🧬 Code graph analysis (4)
odoo/resource/account_move/invoice.py (12)
care/emr/models/charge_item.py (1)
ChargeItem(7-47)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/medication_dispense.py (1)
MedicationDispense(8-34)care/emr/models/scheduling/booking.py (1)
TokenBooking(22-50)care/emr/models/service_request.py (1)
ServiceRequest(7-46)care/emr/resources/charge_item/spec.py (1)
ChargeItemResourceOptions(37-41)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move/spec.py (4)
AccountMoveApiRequest(26-33)AccountMoveReturnApiRequest(36-43)BillType(21-23)InvoiceItem(13-18)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (1)
ProductData(19-27)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
odoo/resource/product_product/spec.py (1)
odoo/resource/product_category/spec.py (1)
CategoryData(4-7)
odoo/resource/account_move/delivery_order.py (8)
care/emr/models/supply_delivery.py (2)
DeliveryOrder(29-48)SupplyDelivery(7-26)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)care/emr/resources/inventory/supply_delivery/spec.py (1)
SupplyDeliveryStatusOptions(18-22)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move/spec.py (3)
AccountMoveApiRequest(26-33)BillType(21-23)InvoiceItem(13-18)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (1)
ProductData(19-27)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)
odoo/resource/product_product/resource.py (6)
care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/product.py (1)
Product(6-17)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (2)
ProductData(19-27)TaxData(8-10)
🪛 Ruff (0.14.4)
odoo/resource/account_move/invoice.py
29-29: Create your own exception
(TRY002)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
odoo/resource/product_product/resource.py
14-14: Create your own exception
(TRY002)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
| supplier_metadata = delivery_order.supplier.metadata or {} | ||
| partner_data = PartnerData( | ||
| name=delivery_order.supplier.name, | ||
| x_care_id=str(delivery_order.supplier.external_id), | ||
| partner_type=PartnerType.company, | ||
| phone=supplier_metadata.get("phone", ""), | ||
| state=supplier_metadata.get("state", "kerala"), | ||
| email=supplier_metadata.get("email", ""), | ||
| agent=False, | ||
| ) |
There was a problem hiding this comment.
Handle missing suppliers before we touch metadata.
supplier on DeliveryOrder is nullable; when it’s absent this block instantly raises. Bail out (log/return) when delivery_order.supplier is falsy before reading metadata.
🤖 Prompt for AI Agents
In odoo/resource/account_move/delivery_order.py around lines 61 to 70, the code
assumes delivery_order.supplier is always present and directly accesses
supplier.metadata, which raises when supplier is None; add an early guard that
checks if not delivery_order.supplier and then either log a clear message and
return/skip processing (or raise a specific exception depending on surrounding
flow) before touching supplier metadata, otherwise proceed to build PartnerData
as before.
| for supply_delivery in supply_deliveries: | ||
| if supply_delivery.supplied_item: | ||
| product = supply_delivery.supplied_item | ||
| base_price = self.get_product_base_price(product) | ||
| purchase_price = self.get_product_purchase_price(product) | ||
|
|
||
| # Get category data if charge item definition exists | ||
| if ( | ||
| product.charge_item_definition | ||
| and product.charge_item_definition.category | ||
| ): | ||
| category_data = CategoryData( | ||
| category_name=product.charge_item_definition.category.title, | ||
| parent_x_care_id=str( | ||
| product.charge_item_definition.category.parent.external_id | ||
| ) | ||
| if product.charge_item_definition.category.parent | ||
| else "", | ||
| x_care_id=str( | ||
| product.charge_item_definition.category.external_id | ||
| ), | ||
| ) | ||
| else: | ||
| category_data = CategoryData( | ||
| category_name="Uncategorized", | ||
| parent_x_care_id="", | ||
| x_care_id="", | ||
| ) | ||
|
|
||
| product_data = ProductData( | ||
| product_name=product.product_knowledge.name | ||
| if product.product_knowledge | ||
| else "Unknown Product", | ||
| x_care_id=str(product.external_id), | ||
| mrp=float(base_price or "0"), | ||
| cost=float(purchase_price or base_price or "0"), | ||
| category=category_data, | ||
| status=product.charge_item_definition.status, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Skip products without charge-item definitions.
Inside the loop we happily do product.charge_item_definition.status even when the product has no definition, leading to an AttributeError. Short-circuit (continue) when the definition is missing before calculating prices or building ProductData.
🤖 Prompt for AI Agents
In odoo/resource/account_move/delivery_order.py around lines 84 to 123, the loop
builds ProductData using product.charge_item_definition.status even when
charge_item_definition is None which raises AttributeError; modify the loop to
check if product.charge_item_definition is present at the start and immediately
continue (skip) if missing, before computing base_price/purchase_price or
constructing CategoryData/ProductData, so only products with a
charge_item_definition proceed.
| product_data = ProductData( | ||
| product_name=charge_item.charge_item_definition.title, | ||
| x_care_id=str(charge_item.charge_item_definition.external_id), | ||
| mrp=float(base_price or "0"), | ||
| cost=float(purchase_price or base_price or "0"), | ||
| category=CategoryData( | ||
| category_name=charge_item.charge_item_definition.category.title, | ||
| parent_x_care_id=str( | ||
| charge_item.charge_item_definition.category.parent.external_id | ||
| ) | ||
| if charge_item.charge_item_definition.category.parent | ||
| else "", | ||
| x_care_id=str( | ||
| charge_item.charge_item_definition.category.external_id | ||
| ), | ||
| ), | ||
| status=charge_item.charge_item_definition.status, | ||
| ) |
There was a problem hiding this comment.
Map status into the enum we defined.
Just like the product sync, feeding the raw charge_item_definition.status into ProductData will explode if the value isn’t one of the Enum literals. Normalize (and default) before constructing the payload.
| mrp=float(base_price or "0"), | ||
| cost=float(purchase_price or mrp or "0"), | ||
| category=CategoryData( |
There was a problem hiding this comment.
Use the actual MRP you just fetched.
You compute mrp = self.get_charge_item_mrp(...) and then promptly ignore it, writing the base price into the MRP slot. That mis-prices every product lacking a base/ MRP match. Cache the resolved mrp/cost values before building ProductData and feed the right ones in.
🤖 Prompt for AI Agents
In odoo/resource/product_product/resource.py around lines 70 to 72, you are
currently writing base_price into the mrp field instead of the computed mrp from
self.get_charge_item_mrp(...). Capture the resolved mrp in a local variable (as
you already compute it), compute cost = float(purchase_price or mrp or "0"), and
use mrp = float(mrp or base_price or "0") (or similar order you intend) and then
pass those cached float values into ProductData (mrp=..., cost=...). Ensure you
convert to float once and reuse those variables when constructing
CategoryData/ProductData so the correct MRP and derived cost are saved.
| status=charge_item_definition.status, | ||
| ).model_dump() |
There was a problem hiding this comment.
Translate status into the expected enum.
ProductData.status wants a ProductStatus; handing it the raw charge_item_definition.status string will trigger a validation error as soon as the value drifts outside {"active","retired","draft"}. Map/normalize it (with a sensible fallback) before instantiating ProductData.
🤖 Prompt for AI Agents
In odoo/resource/product_product/resource.py around lines 81 to 82, the code
passes the raw string charge_item_definition.status into ProductData, but
ProductData.status expects a ProductStatus enum; convert/normalize the incoming
string to the corresponding ProductStatus value (e.g., map "active" ->
ProductStatus.active, "retired" -> ProductStatus.retired, "draft" ->
ProductStatus.draft), handle case variations and unknown values by falling back
to a safe default (e.g., ProductStatus.draft or ProductStatus.retired as per
domain rules), and pass the enum value into ProductData before calling
.model_dump().
| class ProductStatus(str, Enum): | ||
| active = "active" | ||
| retired = "retired" | ||
| draft = "draft" | ||
|
|
||
|
|
||
| class ProductData(BaseModel): | ||
| product_name: str | ||
| x_care_id: str | ||
| cost: float | ||
| mrp: float | ||
| category: CategoryData | ||
| taxes: list[TaxData] | None = None | ||
| hsn: str | None = None | ||
| status: ProductStatus |
There was a problem hiding this comment.
Loosen the ProductStatus contract before this explodes in production.
ChargeItemDefinition.status (and Product.status) are free-form strings in our EMR models; constraining ProductData.status to ProductStatus(active|retired|draft) guarantees a ValidationError the moment a record carries something like "inactive" or "entered_in_error". Either broaden the enum to cover every stored value or just accept a plain string for now. Otherwise the sync dies before reaching Odoo.
🤖 Prompt for AI Agents
In odoo/resource/product_product/spec.py around lines 13 to 27,
ProductData.status is currently typed as a restrictive ProductStatus enum which
will raise ValidationError for any status values outside {active, retired,
draft}; change the declaration to accept a plain string instead (i.e., type
status: str) so the model accepts all EMR free-form status values during sync,
or alternatively expand ProductStatus to include every stored status if strict
enumeration is required — adjust any tests or downstream code that relied on the
enum accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
odoo/resource/account_move/invoice.py (1)
170-203: N+1 query issue already identified.This code segment issues individual queries inside the loop for
ServiceRequest,TokenBooking, andMedicationDispense. The previous review already covered this performance concern in detail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odoo/resource/account_move/invoice.py(1 hunks)odoo/resource/account_move/spec.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
odoo/resource/account_move/invoice.pyodoo/resource/account_move/spec.py
🧬 Code graph analysis (2)
odoo/resource/account_move/invoice.py (13)
care/emr/models/charge_item.py (1)
ChargeItem(7-47)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/medication_dispense.py (1)
MedicationDispense(8-34)care/emr/models/scheduling/booking.py (1)
TokenBooking(22-50)care/emr/models/service_request.py (1)
ServiceRequest(7-46)care/emr/resources/charge_item/spec.py (1)
ChargeItemResourceOptions(37-41)care/emr/resources/common/monetary_component.py (1)
MonetaryComponentType(9-14)odoo/connector/connector.py (2)
OdooConnector(12-71)call_api(14-71)odoo/resource/account_move/spec.py (7)
AccountMoveApiRequest(45-52)AccountMoveReturnApiRequest(55-62)BillType(40-42)DiscountGroup(9-11)DiscountType(14-16)InvoiceDiscounts(19-24)InvoiceItem(31-37)odoo/resource/product_category/spec.py (1)
CategoryData(4-7)odoo/resource/product_product/spec.py (1)
ProductData(19-27)odoo/resource/res_partner/spec.py (2)
PartnerData(11-19)PartnerType(6-8)care/facility/models/patient_icmr.py (1)
odoo/resource/account_move/spec.py (2)
odoo/resource/product_product/spec.py (1)
ProductData(19-27)odoo/resource/res_partner/spec.py (1)
PartnerData(11-19)
🪛 Ruff (0.14.5)
odoo/resource/account_move/invoice.py
32-32: Create your own exception
(TRY002)
32-32: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (3)
odoo/resource/account_move/invoice.py (2)
54-105: LGTM: Defensive programming done right.The
get_first_discountmethod properly uses.get()with safe defaults throughout, avoiding the KeyError pitfalls present in the other helper methods.
143-144: Verify MRP and cost fallback logic.Line 143 uses
base_priceas a fallback formrp, and line 144 usesbase_priceas a second fallback forcost. Ensure that treating the base price as both MRP and purchase cost is semantically correct for your business logic—typically these represent different pricing tiers.odoo/resource/account_move/spec.py (1)
1-62: Well-structured Pydantic models.The specifications are cleanly defined with proper typing, sensible defaults, and correct use of optional fields. The model hierarchy (discount models, invoice items, and request payloads) is logical and maintainable.
| x_care_id=str(invoice.patient.external_id), | ||
| partner_type=PartnerType.person, | ||
| phone=invoice.patient.phone_number, | ||
| state=invoice.facility.state or "kerala", |
There was a problem hiding this comment.
Hardcoded state fallback seems arbitrary.
Defaulting to "kerala" when invoice.facility.state is None appears to be an India-specific assumption that may not hold for all facilities. Consider using a more generic default or requiring facilities to have a state configured.
- state=invoice.facility.state or "kerala",
+ state=invoice.facility.state or "unknown",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state=invoice.facility.state or "kerala", | |
| state=invoice.facility.state or "unknown", |
🤖 Prompt for AI Agents
In odoo/resource/account_move/invoice.py around line 127, the code falls back to
the hardcoded string "kerala" when invoice.facility.state is falsy; remove this
India-specific default and instead use a neutral, deterministic approach —
either use a contextual fallback such as invoice.company_state or None, or
raise/validate earlier to require facility.state. Replace the hardcoded literal
with invoice.facility.state or invoice.company_state (or None) and add a
validation/constraint on the facility model or invoice create/update path to
surface missing state values rather than silently defaulting to "kerala".
| data = AccountMoveReturnApiRequest( | ||
| x_care_id=str(invoice.external_id), | ||
| reason=invoice.status, | ||
| ).model_dump() |
There was a problem hiding this comment.
Use cancelled_reason instead of status for the return reason.
Line 236 maps invoice.status to the return reason, but the Invoice model has a dedicated cancelled_reason field (line 16 in care/emr/models/invoice.py) that should be used here for clarity and correctness.
data = AccountMoveReturnApiRequest(
x_care_id=str(invoice.external_id),
- reason=invoice.status,
+ reason=invoice.cancelled_reason or invoice.status,
).model_dump()🤖 Prompt for AI Agents
In odoo/resource/account_move/invoice.py around lines 234 to 237, the return
payload uses invoice.status for the reason but the Invoice model exposes a
dedicated cancelled_reason field; change the mapping to use
invoice.cancelled_reason instead of invoice.status (convert to string or handle
None consistently with other callers), so x_care_id remains
str(invoice.external_id) and reason becomes invoice.cancelled_reason (or
str(invoice.cancelled_reason) if you need a string).
… into amjith/odoo_module
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
odoo/signals.py (3)
30-37: Still lackscreatedcheck and runs synchronously within transaction.The handler continues to trigger on every User save (create or update) and makes a blocking Odoo API call within the database transaction, which couples external API availability to user persistence.
39-49: Odoo API call still occurs within transaction boundary.The invoice sync continues to run within the database transaction. If Odoo fails or times out, the invoice status change will roll back.
51-65: Transaction.on_commit pattern still not applied.The handler remains coupled to the database transaction, making synchronous Odoo API calls that can block the request thread and roll back payment persistence on external failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
odoo/signals.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
odoo/signals.py
**/signals.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django signals to decouple error handling and logging from core business logic.
Files:
odoo/signals.py
🧠 Learnings (1)
📚 Learning: 2025-07-18T15:17:40.483Z
Learnt from: CR
Repo: ohcnetwork/care PR: 0
File: .cursorrules:0-0
Timestamp: 2025-07-18T15:17:40.483Z
Learning: Applies to **/signals.py : Use Django signals to decouple error handling and logging from core business logic.
Applied to files:
odoo/signals.py
🧬 Code graph analysis (1)
odoo/signals.py (13)
care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/organization.py (1)
Organization(106-107)care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/models/product.py (1)
Product(6-17)care/emr/models/resource_category.py (1)
ResourceCategory(10-86)care/users/models.py (1)
User(196-436)odoo/resource/account_move/invoice.py (3)
OdooInvoiceResource(27-241)sync_invoice_to_odoo_api(107-218)sync_invoice_return_to_odoo_api(220-241)odoo/resource/account_move_payment/payment.py (3)
OdooPaymentResource(21-95)sync_payment_to_odoo_api(22-73)sync_payment_cancel_to_odoo_api(75-95)odoo/resource/product_category/category.py (2)
OdooCategoryResource(5-25)sync_category_to_odoo_api(6-25)odoo/resource/product_product/resource.py (3)
OdooProductProductResource(9-107)sync_product_to_odoo_api(43-85)sync_product_from_product_model(87-107)odoo/resource/res_partner/resource.py (2)
OdooPartnerResource(6-34)sync_partner_to_odoo_api(7-34)odoo/resource/res_user/resource.py (2)
OdooUserResource(7-46)sync_user_to_odoo_api(13-46)
🪛 Ruff (0.14.5)
odoo/signals.py
31-31: Unused function argument: sender
(ARG001)
31-31: Unused function argument: created
(ARG001)
31-31: Unused function argument: kwargs
(ARG001)
40-40: Unused function argument: sender
(ARG001)
40-40: Unused function argument: raw
(ARG001)
40-40: Unused function argument: using
(ARG001)
40-40: Unused function argument: update_fields
(ARG001)
40-40: Unused function argument: kwargs
(ARG001)
52-52: Unused function argument: sender
(ARG001)
52-52: Unused function argument: created
(ARG001)
52-52: Unused function argument: kwargs
(ARG001)
68-68: Unused function argument: sender
(ARG001)
68-68: Unused function argument: created
(ARG001)
68-68: Unused function argument: kwargs
(ARG001)
77-77: Unused function argument: sender
(ARG001)
77-77: Unused function argument: created
(ARG001)
77-77: Unused function argument: kwargs
(ARG001)
90-90: Unused function argument: sender
(ARG001)
90-90: Unused function argument: created
(ARG001)
90-90: Unused function argument: kwargs
(ARG001)
113-113: Unused function argument: sender
(ARG001)
113-113: Unused function argument: created
(ARG001)
113-113: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
| @receiver(post_save, sender=ChargeItemDefinition) | ||
| def sync_charge_item_definition_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync charge item definition to Odoo as a product when created or updated. | ||
| """ | ||
| odoo_product = OdooProductProductResource() | ||
| odoo_product.sync_product_to_odoo_api(instance) | ||
|
|
There was a problem hiding this comment.
Synchronous Odoo sync on every ChargeItemDefinition save without guards.
This handler fires on every save (create and update) without checking the created flag, and the synchronous API call runs within the database transaction, blocking the request thread and causing rollbacks if Odoo is unavailable.
Consider deferring the sync with transaction.on_commit and checking the created flag:
+from django.db import transaction
+
@receiver(post_save, sender=ChargeItemDefinition)
def sync_charge_item_definition_to_odoo(sender, instance, created, **kwargs):
"""
Signal handler to sync charge item definition to Odoo as a product when created or updated.
"""
- odoo_product = OdooProductProductResource()
- odoo_product.sync_product_to_odoo_api(instance)
+ if created:
+ charge_item_id = str(instance.external_id)
+ transaction.on_commit(lambda: sync_charge_item_to_odoo_task(charge_item_id))And implement an async task (Celery or similar) to perform the actual sync outside the transaction boundary.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
68-68: Unused function argument: sender
(ARG001)
68-68: Unused function argument: created
(ARG001)
68-68: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In odoo/signals.py around lines 67 to 74, the current post_save handler
synchronously calls OdooProductProductResource.sync_product_to_odoo_api on every
save (create and update) inside the DB transaction; change it to only trigger on
creation (check the created flag) and defer the actual sync until after the
transaction commits via transaction.on_commit, and instead of making the API
call inline schedule an asynchronous background task (e.g., Celery, RQ or Django
background task) that accepts the instance PK and performs the sync outside the
request/transaction, with proper error handling and logging in the task so Odoo
downtime does not cause request blocking or rollbacks.
| @receiver(post_save, sender=ResourceCategory) | ||
| def sync_resource_category_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync resource category to Odoo when created or updated. | ||
| """ | ||
| if ( | ||
| instance.resource_type | ||
| == ResourceCategoryResourceTypeOptions.charge_item_definition.value | ||
| ): | ||
| odoo_category = OdooCategoryResource() | ||
| odoo_category.sync_category_to_odoo_api(instance) | ||
|
|
There was a problem hiding this comment.
Synchronous API call within transaction for category sync.
While the resource_type check is sensible, the handler still makes a blocking Odoo API call within the database transaction on every save, which can roll back the category save if Odoo is unavailable.
Apply transaction.on_commit to decouple the external API call:
+from django.db import transaction
+
@receiver(post_save, sender=ResourceCategory)
def sync_resource_category_to_odoo(sender, instance, created, **kwargs):
"""
Signal handler to sync resource category to Odoo when created or updated.
"""
if (
instance.resource_type
== ResourceCategoryResourceTypeOptions.charge_item_definition.value
):
- odoo_category = OdooCategoryResource()
- odoo_category.sync_category_to_odoo_api(instance)
+ category_id = str(instance.external_id)
+ transaction.on_commit(lambda: sync_category_to_odoo_task(category_id))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
77-77: Unused function argument: sender
(ARG001)
77-77: Unused function argument: created
(ARG001)
77-77: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In odoo/signals.py around lines 76 to 87, the signal handler is making a
synchronous Odoo API call inside the DB transaction; move that external call
into a transaction.on_commit callback so the API call runs only after the save
successfully commits. Capture the instance.pk (or serialize the minimal data
needed) before registering the callback, and inside the on_commit callback
re-fetch the instance or reconstruct the payload and call
OdooCategoryResource().sync_category_to_odoo_api(...) so the external request
does not block or cause transactions to roll back if Odoo is unavailable.
| @receiver(post_save, sender=Organization) | ||
| def sync_organization_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync organization to Odoo as a partner when org_type is product_supplier. | ||
| """ | ||
| if instance.org_type == OrganizationTypeChoices.product_supplier.value: | ||
| odoo_partner = OdooPartnerResource() | ||
| odoo_partner.sync_partner_to_odoo_api(instance) | ||
|
|
There was a problem hiding this comment.
Partner sync runs synchronously within the transaction.
The org_type check is appropriate, but the handler makes a blocking Odoo API call within the database transaction, which can cause the organization save to roll back if Odoo is unreachable.
Defer the API call with transaction.on_commit:
+from django.db import transaction
+
@receiver(post_save, sender=Organization)
def sync_organization_to_odoo(sender, instance, created, **kwargs):
"""
Signal handler to sync organization to Odoo as a partner when org_type is product_supplier.
"""
if instance.org_type == OrganizationTypeChoices.product_supplier.value:
- odoo_partner = OdooPartnerResource()
- odoo_partner.sync_partner_to_odoo_api(instance)
+ org_id = str(instance.external_id)
+ transaction.on_commit(lambda: sync_organization_to_odoo_task(org_id))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
90-90: Unused function argument: sender
(ARG001)
90-90: Unused function argument: created
(ARG001)
90-90: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In odoo/signals.py around lines 89 to 97, the signal handler currently makes a
blocking Odoo API call inside the DB transaction; change it to defer the sync
using transaction.on_commit so the API call runs only after the save commits.
Import django.db.transaction, capture any minimal data needed (e.g., instance.pk
or serialized fields) and inside transaction.on_commit schedule a callable
(lambda or function) that creates OdooPartnerResource and calls
sync_partner_to_odoo_api using the captured data; this avoids holding the
transaction open and prevents the save from rolling back if Odoo is unreachable.
| @receiver(post_save, sender=Product) | ||
| def sync_product_to_odoo(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal handler to sync product to Odoo when it has a charge item definition. | ||
| """ | ||
| odoo_product = OdooProductProductResource() | ||
| odoo_product.sync_product_from_product_model(instance) |
There was a problem hiding this comment.
Product sync fires on every save without transaction safety.
This handler triggers on every Product save without checking the created flag, and makes a synchronous API call within the database transaction, coupling product persistence to Odoo availability.
Use transaction.on_commit and check the created flag:
+from django.db import transaction
+
@receiver(post_save, sender=Product)
def sync_product_to_odoo(sender, instance, created, **kwargs):
"""
Signal handler to sync product to Odoo when it has a charge item definition.
"""
- odoo_product = OdooProductProductResource()
- odoo_product.sync_product_from_product_model(instance)
+ if created:
+ product_id = str(instance.external_id)
+ transaction.on_commit(lambda: sync_product_to_odoo_task(product_id))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
113-113: Unused function argument: sender
(ARG001)
113-113: Unused function argument: created
(ARG001)
113-113: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In odoo/signals.py around lines 112 to 118, the post_save handler currently runs
on every save and calls Odoo synchronously inside the DB transaction; change it
to first check the created flag and whether the Product has a charge item
definition, then schedule the sync call inside transaction.on_commit so it runs
after commit (and preferably dispatch to a background task/worker rather than
performing a blocking API call in-process). Ensure the on_commit callback
invokes OdooProductProductResource().sync_product_from_product_model(instance)
(or a Celery/RQ task wrapper) so the sync is executed only after successful
commit and not during the DB transaction.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
odoo/signals.py (8)
35-41: User sync runs synchronously on every save, coupled to DB transaction.Right now every
User.save()(create or any update) triggers a blocking Odoo call, and likely inside the DB transaction. That means:
- Any Odoo timeout/error can roll back user creation/update.
- All innocuous updates (profile tweaks, last_login changes, etc.) still hit Odoo, which is probably not what you want with an
api/add/userendpoint.At minimum I’d suggest:
- Run the sync in a
transaction.on_commitcallback so DB state is final before you touch Odoo.- Decide whether you truly want this on every save vs. only on creation or on specific field changes.
For example, a minimal on-commit pattern:
+from django.db import transaction + @receiver(post_save, sender=User) def sync_user_to_odoo(sender, instance, created, **kwargs): - """ - Signal handler to sync user to Odoo when created or updated. - """ - odoo_user = OdooUserResource() - odoo_user.sync_user_to_odoo_api(instance) + """ + Signal handler to sync user to Odoo after commit. + """ + user_external_id = str(instance.external_id) + + def _sync(): + user = User.objects.get(external_id=user_external_id) + odoo_user = OdooUserResource() + odoo_user.sync_user_to_odoo_api(user) + + transaction.on_commit(_sync)If you already have Celery/RQ in this project, pushing
_syncinto a background task from insideon_commitwould avoid blocking the request thread altogether.
44-53: Invoice sync/cancel still happens inline in the transaction.
save_fields_before_updatedirectly callsOdooInvoiceResourceinside the handler whenever status isissuedor inINVOICE_CANCELLED_STATUS. Given invoice creation/updates generally run insideatomicblocks, an Odoo timeout here can roll back billing changes and hold open DB transactions longer than necessary.To reduce coupling:
+from django.db import transaction + @receiver(post_save, sender=Invoice) def save_fields_before_update(sender, instance, raw, using, update_fields, **kwargs): - if instance.status in [ - InvoiceStatusOptions.issued.value, - ]: - odoo_integration = OdooInvoiceResource() - odoo_integration.sync_invoice_to_odoo_api(instance.external_id) - elif instance.status in INVOICE_CANCELLED_STATUS: - odoo_integration = OdooInvoiceResource() - odoo_integration.sync_invoice_return_to_odoo_api(instance.external_id) + invoice_id = str(instance.external_id) + + if instance.status == InvoiceStatusOptions.issued.value: + def _sync_issued(): + odoo_integration = OdooInvoiceResource() + odoo_integration.sync_invoice_to_odoo_api(invoice_id) + transaction.on_commit(_sync_issued) + elif instance.status in INVOICE_CANCELLED_STATUS: + def _sync_cancel(): + odoo_integration = OdooInvoiceResource() + odoo_integration.sync_invoice_return_to_odoo_api(invoice_id) + transaction.on_commit(_sync_cancel)You can swap
_sync_*for async tasks if you don’t want invoice posting to block the request thread.
56-69: Payment reconciliation sync/cancel also runs synchronously inside the save path.The status-based branching looks fine, but both active and cancel flows still call Odoo directly from the signal. That means payment posting or cancellation is effectively “all or nothing” with Odoo, and any remote error bubbles back into the DB transaction.
Pattern wise, this should follow the same
transaction.on_commitapproach as invoices:+from django.db import transaction + @receiver(post_save, sender=PaymentReconciliation) def sync_payment_to_odoo(sender, instance, created, **kwargs): @@ - if instance.status == PaymentReconciliationStatusOptions.active.value: - odoo_payment = OdooPaymentResource() - odoo_payment.sync_payment_to_odoo_api(instance.external_id) - elif instance.status in [ - PaymentReconciliationStatusOptions.cancelled.value, - PaymentReconciliationStatusOptions.entered_in_error.value, - ]: - odoo_payment = OdooPaymentResource() - odoo_payment.sync_payment_cancel_to_odoo_api(instance.external_id) + payment_id = str(instance.external_id) + + if instance.status == PaymentReconciliationStatusOptions.active.value: + def _sync_active(): + odoo_payment = OdooPaymentResource() + odoo_payment.sync_payment_to_odoo_api(payment_id) + transaction.on_commit(_sync_active) + elif instance.status in [ + PaymentReconciliationStatusOptions.cancelled.value, + PaymentReconciliationStatusOptions.entered_in_error.value, + ]: + def _sync_cancel(): + odoo_payment = OdooPaymentResource() + odoo_payment.sync_payment_cancel_to_odoo_api(payment_id) + transaction.on_commit(_sync_cancel)This keeps “when to sync” semantics unchanged but moves “how” out of the transaction.
72-78: ChargeItemDefinition sync fires on every save and blocks on Odoo.Every save of a
ChargeItemDefinitiongoes straight through toOdooProductProductResource.sync_product_to_odoo_apisynchronously. Depending on how often these are edited and howapi/add/productbehaves, this can be:
- Lots of redundant Odoo calls, and
- A source of failed saves if Odoo has issues.
At least moving the call to
transaction.on_commitwould de-risk persistence; you might also decide to only sync on initial creation or on particular status transitions if the Odoo endpoint is not idempotent.Example:
+from django.db import transaction + @receiver(post_save, sender=ChargeItemDefinition) def sync_charge_item_definition_to_odoo(sender, instance, created, **kwargs): @@ - odoo_product = OdooProductProductResource() - odoo_product.sync_product_to_odoo_api(instance) + def _sync(): + odoo_product = OdooProductProductResource() + odoo_product.sync_product_to_odoo_api(instance) + + transaction.on_commit(_sync)You can later tighten the trigger condition (e.g.
if createdor based oninstance.status) once you’ve confirmed the desired behaviour with the Odoo side.
81-91: ResourceCategory sync does external I/O in the signal despite type guard.The
resource_typecheck is good, but once it passes you still call Odoo synchronously and, again, likely while the DB transaction is open. That’s the same failure/coupling pattern as the other handlers.A small restructuring would align it with the recommended pattern:
+from django.db import transaction + @receiver(post_save, sender=ResourceCategory) def sync_resource_category_to_odoo(sender, instance, created, **kwargs): @@ - if ( - instance.resource_type - == ResourceCategoryResourceTypeOptions.charge_item_definition.value - ): - odoo_category = OdooCategoryResource() - odoo_category.sync_category_to_odoo_api(instance) + if ( + instance.resource_type + == ResourceCategoryResourceTypeOptions.charge_item_definition.value + ): + category_id = str(instance.external_id) + + def _sync(): + category = ResourceCategory.objects.get(external_id=category_id) + odoo_category = OdooCategoryResource() + odoo_category.sync_category_to_odoo_api(category) + + transaction.on_commit(_sync)
94-101: Organization→partner sync is still directly bound to Odoo availability.For
Organizationwithorg_type == product_supplier, the signal immediately calls Odoo. If Odoo is down, that will cause supplier save/update to fail, which is usually not what you want for core master data.Same story: push the external call into an
on_commitcallback (and optionally into a background worker):+from django.db import transaction + @receiver(post_save, sender=Organization) def sync_organization_to_odoo(sender, instance, created, **kwargs): @@ - if instance.org_type == OrganizationTypeChoices.product_supplier.value: - odoo_partner = OdooPartnerResource() - odoo_partner.sync_partner_to_odoo_api(instance) + if instance.org_type == OrganizationTypeChoices.product_supplier.value: + org_id = str(instance.external_id) + + def _sync(): + org = Organization.objects.get(external_id=org_id) + odoo_partner = OdooPartnerResource() + odoo_partner.sync_partner_to_odoo_api(org) + + transaction.on_commit(_sync)
104-114: DeliveryOrder→vendor bill sync is a heavy call inside the signal.For completed delivery orders without an origin, you post a vendor bill to Odoo synchronously on every qualifying save. Given the amount of data involved and external latency, this is a prime candidate for
transaction.on_commit+ async.A minimal on-commit refactor:
+from django.db import transaction + @receiver(post_save, sender=DeliveryOrder) def sync_delivery_order_to_odoo(sender, instance, created, **kwargs): @@ - if ( - instance.status == SupplyDeliveryOrderStatusOptions.completed.value - and not instance.origin - ): - odoo_delivery_order = OdooDeliveryOrderResource() - odoo_delivery_order.sync_delivery_order_to_odoo_api(instance.external_id) + if ( + instance.status == SupplyDeliveryOrderStatusOptions.completed.value + and not instance.origin + ): + delivery_order_id = str(instance.external_id) + + def _sync(): + odoo_delivery_order = OdooDeliveryOrderResource() + odoo_delivery_order.sync_delivery_order_to_odoo_api(delivery_order_id) + + transaction.on_commit(_sync)If you already have a worker infrastructure, wrapping
_syncin a task would be an easy next step.
117-123: Product sync is synchronous and unconditional; rely onon_commitand existing guard.
sync_product_to_odoocurrently callssync_product_from_product_modeldirectly for every Product save. The actual guard oncharge_item_definitionlives insideOdooProductProductResource.sync_product_from_product_model, which is fine, but this is still a synchronous external call in the signal.You can keep the existing semantics and just move it out of the transaction:
+from django.db import transaction + @receiver(post_save, sender=Product) def sync_product_to_odoo(sender, instance, created, **kwargs): @@ - odoo_product = OdooProductProductResource() - odoo_product.sync_product_from_product_model(instance) + product_id = instance.pk + + def _sync(): + product = Product.objects.get(pk=product_id) + odoo_product = OdooProductProductResource() + odoo_product.sync_product_from_product_model(product) + + transaction.on_commit(_sync)That keeps the “only if it has a
charge_item_definition” logic centralised in the resource class, but no longer risks rolling back a Product save due to Odoo.
🧹 Nitpick comments (1)
odoo/signals.py (1)
35-123: Ruff ARG001 warnings for unused signal arguments.Ruff is flagging all the unused
sender,created,raw,using,update_fields, andkwargsparameters. Django’s signal API does pass these (by name), so you can’t safely rename them to_sender/_createdwithout changing the call signature, but you also don’t have to pretend to use them everywhere either.Two low-friction options:
- Add
# noqa: ARG001on the function definitions where you intentionally ignore the extra args, e.g.:@receiver(post_save, sender=User) def sync_user_to_odoo(sender, instance, created, **kwargs): # noqa: ARG001 ...
- Or, if you really want to avoid
noqa, explicitly consume them:def sync_user_to_odoo(sender, instance, created, **kwargs): _ = (sender, created, kwargs) ...I’d lean toward a
noqahere so the intent stays clear and the signature matches Django’s docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
odoo/signals.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
odoo/signals.py
**/signals.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django signals to decouple error handling and logging from core business logic.
Files:
odoo/signals.py
🧠 Learnings (1)
📚 Learning: 2025-07-18T15:17:40.483Z
Learnt from: CR
Repo: ohcnetwork/care PR: 0
File: .cursorrules:0-0
Timestamp: 2025-07-18T15:17:40.483Z
Learning: Applies to **/signals.py : Use Django signals to decouple error handling and logging from core business logic.
Applied to files:
odoo/signals.py
🧬 Code graph analysis (1)
odoo/signals.py (19)
care/emr/models/charge_item_definition.py (1)
ChargeItemDefinition(6-24)care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/models/product.py (1)
Product(6-17)care/emr/models/resource_category.py (1)
ResourceCategory(10-86)care/emr/models/supply_delivery.py (1)
DeliveryOrder(29-48)care/emr/resources/inventory/supply_delivery/delivery_order.py (1)
SupplyDeliveryOrderStatusOptions(18-24)care/emr/resources/invoice/spec.py (1)
InvoiceStatusOptions(15-20)care/emr/resources/organization/spec.py (1)
OrganizationTypeChoices(11-15)care/emr/resources/payment_reconciliation/spec.py (1)
PaymentReconciliationStatusOptions(22-26)care/emr/resources/resource_category/spec.py (1)
ResourceCategoryResourceTypeOptions(10-13)care/users/models.py (1)
User(196-436)odoo/resource/account_move/delivery_order.py (2)
OdooDeliveryOrderResource(21-149)sync_delivery_order_to_odoo_api(46-149)odoo/resource/account_move/invoice.py (3)
OdooInvoiceResource(27-241)sync_invoice_to_odoo_api(107-218)sync_invoice_return_to_odoo_api(220-241)odoo/resource/account_move_payment/payment.py (3)
OdooPaymentResource(21-95)sync_payment_to_odoo_api(22-73)sync_payment_cancel_to_odoo_api(75-95)odoo/resource/product_category/category.py (2)
OdooCategoryResource(5-25)sync_category_to_odoo_api(6-25)odoo/resource/product_product/resource.py (3)
OdooProductProductResource(9-107)sync_product_to_odoo_api(43-85)sync_product_from_product_model(87-107)odoo/resource/res_partner/resource.py (2)
OdooPartnerResource(6-34)sync_partner_to_odoo_api(7-34)odoo/resource/res_user/resource.py (2)
OdooUserResource(7-46)sync_user_to_odoo_api(13-46)
🪛 Ruff (0.14.5)
odoo/signals.py
36-36: Unused function argument: sender
(ARG001)
36-36: Unused function argument: created
(ARG001)
36-36: Unused function argument: kwargs
(ARG001)
45-45: Unused function argument: sender
(ARG001)
45-45: Unused function argument: raw
(ARG001)
45-45: Unused function argument: using
(ARG001)
45-45: Unused function argument: update_fields
(ARG001)
45-45: Unused function argument: kwargs
(ARG001)
57-57: Unused function argument: sender
(ARG001)
57-57: Unused function argument: created
(ARG001)
57-57: Unused function argument: kwargs
(ARG001)
73-73: Unused function argument: sender
(ARG001)
73-73: Unused function argument: created
(ARG001)
73-73: Unused function argument: kwargs
(ARG001)
82-82: Unused function argument: sender
(ARG001)
82-82: Unused function argument: created
(ARG001)
82-82: Unused function argument: kwargs
(ARG001)
95-95: Unused function argument: sender
(ARG001)
95-95: Unused function argument: created
(ARG001)
95-95: Unused function argument: kwargs
(ARG001)
105-105: Unused function argument: sender
(ARG001)
105-105: Unused function argument: created
(ARG001)
105-105: Unused function argument: kwargs
(ARG001)
118-118: Unused function argument: sender
(ARG001)
118-118: Unused function argument: created
(ARG001)
118-118: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (1)
odoo/signals.py (1)
1-32: Confirmed: all handlers sync to Odoo inline without transaction.on_commit().The analysis is spot-on. All 8 signal handlers execute synchronous Odoo API calls directly—no transaction deferral pattern anywhere. Each handler (lines 36–118) instantiates an Odoo resource class and calls its sync method immediately, without wrapping in
transaction.on_commit(). The transaction module isn't imported, and there's zero use ofon_commitin the file.This tight coupling between external API and database transaction is a real architectural risk: if Odoo is unavailable or slow, you're rolling back core EMR persistence or stalling request threads. The recommendation to import
transactionand consistently defer these calls until after commit is a sound mitigation and aligns with Django best practices.⛔ Skipped due to learnings
Learnt from: CR Repo: ohcnetwork/care PR: 0 File: .cursorrules:0-0 Timestamp: 2025-07-18T15:17:40.483Z Learning: Applies to **/signals.py : Use Django signals to decouple error handling and logging from core business logic.
Odoo
Summary by CodeRabbit
New Features
Improvements
Chore