refactor: create integration request in new transaction; code smell;

[karm1000]

Summary by CodeRabbit

• New Features

  • Integration requests now capture richer details (headers, payload, response output, and errors) for improved visibility and auditing.

• Refactor

  • Integration request submission streamlined into a single, isolated transactional path to improve reliability and avoid partial writes.
  • Added an internal helper to run integration operations in a fresh transaction context for safer, more predictable behavior.

[coderabbitai]

Walkthrough

Replaced calls to enqueue_integration_request with create_integration_request in the parser; removed enqueue_integration_request; added execute_in_new_transaction decorator in transaction_parser.transaction_parser.utils and applied it to create_integration_request; updated related imports.

Changes

Cohort / File(s)	Summary
Parser integration request call switch transaction_parser/transaction_parser/ai_integration/parser.py	Import changed to create_integration_request from transaction_parser.transaction_parser.utils.integration_request; finally-block now calls create_integration_request(**log) instead of enqueue_integration_request(**log).
Utilities: transactional decorator transaction_parser/transaction_parser/utils/__init__.py	Added execute_in_new_transaction(fn) decorator (uses functools.wraps) that runs a function in a fresh DB connection/transaction (non-admin), ensuring commit/close and restoration of previous DB context. Exposed at module level.
Integration request refactor transaction_parser/transaction_parser/utils/integration_request.py	Removed enqueue_integration_request; updated imports to use execute_in_new_transaction and pretty_json from transaction_parser.transaction_parser.utils; decorated create_integration_request with @execute_in_new_transaction.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Parser as Parser
  participant IR as create_integration_request
  participant DB as Database

  Parser->>IR: create_integration_request(..., request_headers, data, output?, error?, ...)
  activate IR
  note right of IR #e6f7ff: Wrapped by execute_in_new_transaction
  IR->>DB: Open new DB connection / begin transaction (non-admin)
  DB-->>IR: Connection ready
  IR->>DB: Insert/update integration request record(s)
  alt success
    DB-->>IR: Commit
  else error
    DB-->>IR: Rollback
  end
  IR-->>Parser: ack/return
  deactivate IR

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hop through logs with nimble might,
New transactions stitched in soft moonlight.
No queue to wait — I write and seal,
Commit or roll, the burrow's real.
Headers, data, tidy and bright — hop! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title combines multiple clauses with semicolons and includes a vague “code smell” remark, which adds noise rather than clarifying the main change; it should instead be a concise, single sentence that clearly highlights the primary refactor to run the integration request in a new transaction.	Rename the title to focus on the core change, for example: “refactor: run create_integration_request in a new transaction” to remove extraneous wording and improve clarity.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ir-in-new-transaction

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5478b9c and 1995b67.

📒 Files selected for processing (1)

• transaction_parser/transaction_parser/utils/__init__.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• transaction_parser/transaction_parser/utils/init.py

⏰ 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: Python Unit Tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56f73ac and 4854a77.

📒 Files selected for processing (3)

• transaction_parser/transaction_parser/ai_integration/parser.py (2 hunks)
• transaction_parser/transaction_parser/utils/__init__.py (2 hunks)
• transaction_parser/transaction_parser/utils/integration_request.py (1 hunks)

🔇 Additional comments (4)

transaction_parser/transaction_parser/utils/__init__.py (1)

2-2: LGTM!

The import of wraps from functools is necessary for the new decorator and correctly preserves function metadata.

transaction_parser/transaction_parser/utils/integration_request.py (2)

6-9: LGTM!

The updated imports correctly bring in the new execute_in_new_transaction decorator and align with the refactoring objective.

---

14-14: Verify transaction safety and idempotency.

Applying @execute_in_new_transaction changes the execution model. Please confirm:

1. Idempotency: Does create_integration_request have safeguards (e.g., unique constraints, duplicate checks) to prevent creating duplicate integration requests if retried?
2. Error handling: Does the decorator properly roll back the transaction on errors, or does it only commit in the finally block?
3. Privilege requirements: The decorator "connects without elevating admin privileges" - verify this function doesn't require admin access.
4. Concurrency: Is the function safe for concurrent calls, or could race conditions occur?
5. Semantic change: Replacing enqueue_integration_request (async) with create_integration_request (sync in new transaction) may impact performance and error propagation to callers.

Run the following script to examine the implementation:

transaction_parser/transaction_parser/ai_integration/parser.py (1)

14-16: LGTM!

The import change correctly reflects the refactoring to use create_integration_request with the new transactional decorator.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Wrap in try-except to prevent exception masking.

Creating the integration request in a separate transaction is good for observability. However, if create_integration_request raises an exception in the finally block, it will replace any exception raised earlier (line 79), making debugging difficult.

Apply this diff to prevent exception masking:

         finally:
-            create_integration_request(**log)
+            try:
+                create_integration_request(**log)
+            except Exception:
+                frappe.log_error("Failed to create integration request")

📝 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.

Suggested change

	finally:
	enqueue_integration_request(**log)
	create_integration_request(**log)
	finally:
	try:
	create_integration_request(**log)
	except Exception:
	frappe.log_error("Failed to create integration request")

🤖 Prompt for AI Agents

In transaction_parser/transaction_parser/ai_integration/parser.py around lines
81-82, the finally block calls create_integration_request which can raise and
mask an earlier exception; wrap the create_integration_request call in a
try/except that catches and logs the secondary exception (including traceback
and context) but does not overwrite the original error — either suppress the
secondary exception after logging or attach it via chaining, and ensure the
original exception (if present) is re-raised so the root cause is preserved.

[Abdeali099]

@karm1000