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_credit_control #268

Merged
merged 100 commits into from
May 29, 2023

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Feb 28, 2023

Continue from #242

@astirpe astirpe force-pushed the 16_mig_account_credit_control branch 2 times, most recently from 7c6023d to 63d001d Compare February 28, 2023 15:40
@astirpe astirpe marked this pull request as ready for review February 28, 2023 15:43
Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

There's an error when you try to get into the partner (no matter if it's a customer or vendor).
The error code:

`Traceback (most recent call last):
File "/opt/odoo/odoo/http.py", line 1584, in _serve_db
return service_model.retrying(self._serve_ir_http, self.env)
File "/opt/odoo/odoo/service/model.py", line 134, in retrying
result = func()
File "/opt/odoo/odoo/http.py", line 1613, in _serve_ir_http
response = self.dispatcher.dispatch(rule.endpoint, args)
File "/opt/odoo/odoo/http.py", line 1810, in dispatch
result = self.request.registry['ir.http']._dispatch(endpoint)
File "/opt/odoo/odoo/addons/base/models/ir_http.py", line 149, in _dispatch
result = endpoint(**request.params)
File "/opt/odoo/odoo/http.py", line 699, in route_wrapper
result = endpoint(self, *args, **params_ok)
File "/opt/odoo/addons/web/controllers/dataset.py", line 42, in call_kw
return self._call_kw(model, method, args, kwargs)
File "/opt/odoo/addons/web/controllers/dataset.py", line 33, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/opt/odoo/odoo/api.py", line 457, in call_kw
result = _call_kw_model(method, model, args, kwargs)
File "/opt/odoo/odoo/api.py", line 430, in _call_kw_model
result = method(recs, *args, **kwargs)
File "/opt/odoo/odoo/models.py", line 1605, in name_search
ids = self._name_search(name, args, operator, limit=limit)
File "/opt/odoo/odoo/models.py", line 1624, in _name_search
return self._search(args, limit=limit, access_rights_uid=name_get_uid)
File "/opt/odoo/odoo/models.py", line 4624, in _search
query = self._where_calc(domain)
File "/opt/odoo/odoo/models.py", line 4392, in _where_calc
return expression.expression(domain, self).query
File "/opt/odoo/odoo/osv/expression.py", line 447, in init
self.parse()
File "/opt/odoo/odoo/osv/expression.py", line 849, in parse
ids2 = comodel._name_search(right, domain or [], op2, limit=None)
File "/opt/odoo/addons/account/models/account_account.py", line 580, in _name_search
return self._search(expression.AND([domain, args]), limit=limit, access_rights_uid=name_get_uid)
File "/opt/odoo/odoo/models.py", line 4624, in _search
query = self._where_calc(domain)
File "/opt/odoo/odoo/models.py", line 4392, in _where_calc
return expression.expression(domain, self).query
File "/opt/odoo/odoo/osv/expression.py", line 447, in init
self.parse()
File "/opt/odoo/odoo/osv/expression.py", line 1024, in parse
expr, params = self.__leaf_to_sql(leaf, model, alias)
File "/opt/odoo/odoo/osv/expression.py", line 1100, in __leaf_to_sql
raise ValueError("Invalid domain term %r" % (leaf,))
ValueError: Invalid domain term ('name', 'in', 'property_account_receivable_id')

The above server error caused the following client error:
null`

@astirpe astirpe force-pushed the 16_mig_account_credit_control branch 4 times, most recently from 71ff07b to 11a8a60 Compare March 7, 2023 09:27
@astirpe
Copy link
Member Author

astirpe commented Mar 7, 2023

Thank you @Bearnard21, should be working now

@astirpe astirpe force-pushed the 16_mig_account_credit_control branch from 11a8a60 to 4032416 Compare March 7, 2023 09:33
Copy link

@nguyenminhchien nguyenminhchien left a comment

Choose a reason for hiding this comment

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

Hi @astirpe, i just added some comments, feel free to discuss with me bout them.
Thank you,

account_credit_control/README.rst Show resolved Hide resolved
account_credit_control/README.rst Show resolved Hide resolved
account_credit_control/views/credit_control_line.xml Outdated Show resolved Hide resolved
to_add, to_remove = self._get_partner_related_lines(credit_control_run)
lines = (lines | to_add) - to_remove
to_add, to_remove = self._get_invoice_related_lines(credit_control_run)
lines = (lines | to_add) - to_remove

Choose a reason for hiding this comment

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

Because all invoice lines must be for a particular company. I cannot find the case where (lines | to_add) != lines.
I mean that we don't need to_add in both cases _get_partner_related_lines and _get_invoice_related_lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to agree with you, but I don't know why this part was implemented this way. Maybe to allow _get_partner_related_lines and _get_invoice_related_lines to be extended by extra modules?

Choose a reason for hiding this comment

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

Yes, maybe. So, just let to_add = self.env["account.move.line"] in def _move_lines_subset()
Extended modules will update it then

</field>
</record>
<!-- for button -->
<record id="action_wizard_credit_policy_changer" model="ir.actions.act_window">

Choose a reason for hiding this comment

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

cannot find a menu call this action

Copy link
Member Author

Choose a reason for hiding this comment

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

Me neither, I'm trying to understand how was this originally supposed to work

Choose a reason for hiding this comment

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

You should add a new menu to call this action, or delete it.

@ and others added 18 commits March 27, 2023 16:19
* Modify scenarios to be compatible with python scenarios
* Voucher/statement related steps
* Deprecated step implementation
* Print report generation
* All tests green
* Mail + print of reminder
* Broken translations
* Fix failure to confirm invoice when both account_credit_control  and account_constraints are installed
* use except_orm instead of except_osv
* reindent code
* Cleanup
* use of deprecated field user_email causes permission error in multicompany setting fixed by using the correct 'email' field in the template
* Address in mako template to make sure address is displayed
* Remove 'more info here' from report
* Add some css to reminder report and set texts taking the whole width of the document
* Add a subject in reminder report
* Report use precise mode by default
* date_entry fields on line model to be used by report or in next MP on filter group by
* reporting layer, + add hook function to get contact address
* credit control mail are not in plain text but send as attachement
* policy level name is now translatable as it is use in report and mail
* permission on invoices because onetomany widget load data even if hidden ...
* translation files + lang source
* french translation
* Translation + report invoice address layout
* Label Force credit control policy + french translation
* pep8 (space after comma, reduced length of lines)
* does not need to check if there is ids before calling unlink, it will return early
* remove unused import
* 'raise a new invoice' is a British English expression for 'issue a new invoice'
* scenarios: We fix year of scenario on 2013 to have reproducable setup, and ensure test maintnability. The credit control scenarios are based on base_finance setup scenario provided in the project OpenERP Scenarios. This setup provide a financial setup for 2012 2013. We also fix some value as precision computation has been improved in OpenERP.
* bug 1287072 Level calculation error if previous credit line is ignored
* unifing wizard views to respect UI guide lines
* credit control policy changer. Add a wizard on Invoice that will add a credit line and deprecate exisiting one on the invoice
* test to ensure that wizard run on customer invoice
* Scenario that test coherence of credit run after manually altering an invoice level
* multicurrency in communication
* ...
@astirpe astirpe force-pushed the 16_mig_account_credit_control branch from 9994273 to 0d1e8c4 Compare March 30, 2023 08:22
@astirpe
Copy link
Member Author

astirpe commented Mar 30, 2023

@nguyenminhchien regarding the two points left, I understand your proposal but I'm not yet fully convinced I want to change that part of code. Maybe the authors of that part could join the discussion, but for the time being I would leave it as it is, since this PR was meant to be a pure module migration. Do you mind to let the discussion continue on a separate PR once this one is merged?

Copy link

@nguyenminhchien nguyenminhchien left a comment

Choose a reason for hiding this comment

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

@astirpe: it's ok

Copy link
Member

@GabbasovDinar GabbasovDinar left a comment

Choose a reason for hiding this comment

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

Code LGTM

@victoralmau
Copy link
Member

Please, cherry-pick #279 to commit history

@astirpe
Copy link
Member Author

astirpe commented Apr 20, 2023

@victoralmau

Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Credit control policy level form check

  1. A check is performed after all policy levels were added to the credit control policy form. In case you've missed the custom mail message field in any of the credit control policy levels, you will get a warning on saving the whole policy. If there are several levels that were added, it's pretty inconvenient to check all the policy levels to find the missing field, so it would be nice if a check will be performed right on the policy level form.
  2. In case you've chosen phone call action, you still have to fill mandatory e-mail template field.
  3. If you're making changes in created policy level (eg. changing the channel from phone to e-mail), checking on the custom mail message field is not performed.

@astirpe
Copy link
Member Author

astirpe commented May 8, 2023

@Bearnard21 I see.

Point 1 and 3 are something related to a common issue of HTML fields, so I don't think there's a quick way to fix it within this PR. I found an old discussion where this issue is explained more in details: https://www.odoo.com/forum/help-1/how-to-make-a-html-field-mandatory-in-the-form-161104

For point 2 I agree that it's something should be fixed, but is a problem already existing in previous versions of this module, so I think it's better to open a dedicated issue for that. What do you think?

@Bearnard21
Copy link

Sorry for replying late,
Points 1 and 3
You can override the "New" method and recall function that sets up constrain for HTML to perform a check of the new policy right on a form view. Just keep in mind that the empty HTML field contains 12 symbols.
Talking about point 2
Asking me, I think that improvement could be done in this PR.

@astirpe
Copy link
Member Author

astirpe commented May 22, 2023

@Bearnard21 please go ahead and open a PR to fix your points on previous versions. I will cherry pick your fixes to include them in this PR. Thanks!

Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Changes requested to be commited in next PR

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@andreampiovesana
Copy link

merge?

@pedrobaeza
Copy link
Member

/ocabot migration account_credit_control
/ocabot merge nobump

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone May 29, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request May 29, 2023
12 tasks
@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-268-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 64dfbb3 into OCA:16.0 May 29, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at cc55cd4. Thanks a lot for contributing to OCA. ❤️

Comment on lines +197 to +199
action_name = "account_credit_control.credit_control_line_action"
action = self.env.ref(action_name)
action = action.read()[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will raise AccessError for anyone not member of Settings/Administration group. Proper way to do it is:

action_name = "account_credit_control.credit_control_line_action"
action = self.env["ir.actions.act_window"]._for_xml_id(action_name)

That seems to be missing in the migration guide actually 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I opened #287

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

Successfully merging this pull request may close these issues.