Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][mig] account_banking_ach_base #128

Open
wants to merge 20 commits into
base: 16.0
Choose a base branch
from

Conversation

JordiBForgeFlow
Copy link
Member

Superseeds #97

@rrebollo
Copy link

@JordiBForgeFlow @JasminSForgeFlow would U need help with failing checks? Is this ready for code review? Is there anything I can do to help?

@JasminSForgeFlow
Copy link

@JordiBForgeFlow @JasminSForgeFlow would U need help with failing checks? Is this ready for code review? Is there anything I can do to help?

Hi,
It's ready for review.
Thanks

Copy link

@rrebollo rrebollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. The code review LGTM; however, I suggest considering some of my comments to improve overall quality and ensure compliance with OCA's guidelines.

"website": "https://github.com/OCA/l10n-usa",
"category": "Banking addons",
"depends": [
"account_payment_order",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a dependency in another oca repository I'll suggest the use of oca_dependencies.txt

"category": "Banking addons",
"depends": [
"account_payment_order",
"account_banking_mandate",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

"l10n_us_partner_legal_number",
],
"data": [
"views/account_banking_mandate.xml",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Odoo's guidelines which are the base for ours this file's name should be suffixed with "_views". The renaming should be done with "git move" command.

],
"data": [
"views/account_banking_mandate.xml",
"views/account_move.xml",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

"data": [
"views/account_banking_mandate.xml",
"views/account_move.xml",
"views/res_bank.xml",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

"views/account_banking_mandate.xml",
"views/account_move.xml",
"views/res_bank.xml",
"views/res_company.xml",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

"views/res_bank.xml",
"views/res_company.xml",
],
"external_dependencies": {"python": ["python-stdnum", "ach"]},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCA's Contributing guidelines suggest put installation instructions for these as part of the INSTALLATION section of README

def set_payment_modes_on_partner(self):
"""
Set the payment modes on the Partner if they don't already exist.
"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""
"""
self.ensure_one()

@lauradiaz22
Copy link

Good morning,

During the functional review, I have detected an error related to traceability. This problem occurs when creating a customer invoice with the payment mode configured as ACH Customers and with a bank mandate set up.

When trying to add the line item to a debit mandate, the system threw the following traceability error:

Traceback (most recent call last):
File "/opt/odoo/custom/src/odoo/odoo/http.py", line 1652, in _serve_db
return service_model.retrying(self._serve_ir_http, self.env)
File "/opt/odoo/custom/src/odoo/odoo/service/model.py", line 133, in retrying
result = func()
File "/opt/odoo/custom/src/odoo/odoo/http.py", line 1679, in _serve_ir_http
response = self.dispatcher.dispatch(rule.endpoint, args)
File "/opt/odoo/custom/src/odoo/odoo/http.py", line 1883, in dispatch
result = self.request.registry['ir.http']._dispatch(endpoint)
File "/opt/odoo/custom/src/odoo/odoo/addons/base/models/ir_http.py", line 154, in _dispatch
result = endpoint(**request.params)
File "/opt/odoo/custom/src/odoo/odoo/http.py", line 734, in route_wrapper
result = endpoint(self, *args, **params_ok)
File "/opt/odoo/auto/addons/web/controllers/dataset.py", line 46, in call_button
action = self._call_kw(model, method, args, kwargs)
File "/opt/odoo/auto/addons/web/controllers/dataset.py", line 33, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/opt/odoo/custom/src/odoo/odoo/api.py", line 468, in call_kw
result = _call_kw_multi(method, model, args, kwargs)
File "/opt/odoo/custom/src/odoo/odoo/api.py", line 453, in _call_kw_multi
result = method(recs, *args, **kwargs)
File "/opt/odoo/auto/addons/account_banking_ach_base/models/account_move.py", line 19, in create_account_payment_line
raise UserError(
TypeError: UserError.init() got an unexpected keyword argument 'name'

The above server error caused the following client error:
RPC_ERROR: Odoo Server Error
RPC_ERROR
at makeErrorFromResponse (http://10.10.10.72:16069/web/assets/459-123235e/web.assets_backend.min.js:997:163)
at XMLHttpRequest. (http://10.10.10.72:16069/web/assets/459-123235e/web.assets_backend.min.js:1005:13)

I remain attentive to any additional questions or clarifications you may need to resolve this issue.

Thank you very much in advance

Copy link

@rrebollo rrebollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We missed these before. Please test my suggestions locally because I have no linter or pre-commit formatting in github.

_(
"To satisfy payment mandate, cannot add "
"invoice %(name)s to Debit Order until %(delay_expired)s!"
),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
),
,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analizing the traceback from @lauradiaz22 I think this should solve the issue.

"invoice %(name)s to Debit Order until %(delay_expired)s!"
),
name=invoice.name,
delay_expired=delay_expired.strftime("%Y-%m-%d"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
delay_expired=delay_expired.strftime("%Y-%m-%d"),
delay_expired=delay_expired.strftime("%Y-%m-%d"))

@rrebollo
Copy link

rrebollo commented Dec 24, 2024

@lauradiaz22, thank you for your input. While I’m not the author of the PR, I am serving as a technical reviewer. The issue you identified is valid; however, I believe there’s a potential workaround. Based on the section of code triggering the issue, it seems likely that the problem lies with the bank mandate configuration. If the code were functioning as intended, you would have seen a UserError popup with a message similar to the following:

To satisfy payment mandate, cannot add invoice (an invoice number) to Debit Order until (a date computed from invoice date + mandate's delay days)!

Unless that was exactly what you were trying to accomplish. I have requested some changes to @JordiBForgeFlow to fix the issue you just detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.