since migrating to 14 rejected payment order are not treated#272
since migrating to 14 rejected payment order are not treated#272davidwul wants to merge 3 commits intoCompassionCH:14.0from
Conversation
Summary of ChangesHello @davidwul, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a critical issue where rejected payment orders were not being properly processed after a migration to Odoo 14. It enhances the payment return mechanism to correctly identify and handle rejected transactions, ensuring that associated payment lines are freed and states are updated appropriately. Additionally, the PR includes a minor correction in the recurring contract module and a substantial cleanup of the project's Odoo addon packaging setup, streamlining the build process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where rejected payment orders are not processed correctly, which seems to be a problem since migrating to Odoo 14. The main changes are in account_ebics_payment_return to handle the 'uploaded' state for payment orders and to correctly identify payment lines for rejected transactions. While the intent is clear, I've found a few issues. There's a critical bug in the logic for finding payment lines that could lead to application crashes. I've also suggested improvements for error handling and code style to enhance robustness and maintainability. The change in recurring_contract and the removal of setup files appear to be correct.
| endtoend_id=t.find("./ns:OrgnlEndToEndId", namespaces={"ns": ns}).text | ||
| payment_id = payment_order.payment_ids.filtered( | ||
| lambda l: l.move_id.id == int(endtoend_id)) | ||
| payment_line_ids = payment_order.payment_line_ids.filtered(lambda l: l.payment_ids.id ==payment_id.id) | ||
| _logger.info(f"PAIN002 payment_line_ids: {payment_id.name} with endtoend_id: {endtoend_id}", ) |
There was a problem hiding this comment.
The logic for finding payment_line_ids has several critical issues that could lead to runtime errors and incorrect behavior:
int(endtoend_id)will raise aValueErrorifendtoend_idfrom the XML is not a string representing an integer. This will crash the import process and should be handled gracefully.- The result of
filtered,payment_id, is a recordset. If no payment is found, it will be empty. If multiple are found, it will contain multiple records. Accessing attributes like.idor.namewill fail ifpayment_idis not a singleton recordset. This needs to be handled, for example by usingensure_one(). - The expression
l.payment_ids.id == payment_id.idis incorrect.l.payment_idsis aMany2manyfield, so it's a recordset and doesn't have an.idattribute. You should check for membership, e.g.,payment in l.payment_ids. - The log message is confusing as it says
payment_line_idsbut logspayment_id.name.
endtoend_id = t.find("./ns:OrgnlEndToEndId", namespaces={"ns": ns}).text
try:
move_id = int(endtoend_id)
except (ValueError, TypeError):
_logger.warning(f"PAIN002: Invalid OrgnlEndToEndId '{endtoend_id}'.")
continue
payment = payment_order.payment_ids.filtered(
lambda p: p.move_id.id == move_id
)
if not payment:
_logger.warning(f"PAIN002: No payment found for move_id {move_id}.")
continue
payment.ensure_one()
payment_line_ids = payment_order.payment_line_ids.filtered(
lambda l: payment in l.payment_ids
)
_logger.info(
f"PAIN002 found payment lines {payment_line_ids.ids} for payment {payment.name} with endtoend_id: {endtoend_id}"
)| ) | ||
| _logger.info("PAIN002 payment_order: %s", payment_order) | ||
| if payment_order.state == "generated": | ||
| if payment_order.state in ("generated","uploaded"): |
There was a problem hiding this comment.
For better readability and adherence to PEP 8 style guidelines, it's recommended to add a space after the comma within the tuple.
| if payment_order.state in ("generated","uploaded"): | |
| if payment_order.state in ("generated", "uploaded"): |
References
- PEP 8 suggests adding a space after commas in tuples, dicts, and lists to improve readability. (link)
| for b in payment_line_ids: | ||
| try: | ||
| b.free_line(rsn_txt) | ||
| _logger.info(f"PAIN002 line free: {rsn_txt}" ,) | ||
| except Exception as e: | ||
| _logger.info(f"error line free: {e}" ,) |
There was a problem hiding this comment.
The try...except block is a good addition for robustness. However, it can be improved:
- Catching the generic
Exceptionis too broad. Thefree_linemethod raisesodoo.exceptions.UserError. It's better to catch this specific exception to avoid hiding other unexpected errors. - When an error occurs, logging it with
_logger.infois not ideal for an error condition._logger.warningor_logger.errorwould be more appropriate to highlight the problem. The log message should also include more context, like which payment line failed. - The trailing comma in the logger calls is unnecessary and can be removed for cleaner code.
for b in payment_line_ids:
try:
b.free_line(rsn_txt)
_logger.info(f"PAIN002 line free: {rsn_txt}")
except UserError as e:
_logger.error(f"Error freeing line {b.id}: {e}")1 participant
No description provided.