[18.0][FIX] account_credit_control: correctly update control lines status

[len-foss]

This PR is a forward port of #447, and addresses the comment https://github.com/OCA/credit-control/pull/430/files#r1963626501

The state of the control lines should be changed after sending the email, but this wasn't done because there was a check on the subtype.

Another bug that affected this lack of state change was: #426
Because related values weren't flushed, the lines weren't linked to the communication, and so the postprocess would never update their state.

[pedrobaeza]

It seems you should squash the first 2 commits together, and why do you need the so called "flexibility" in the tests? You are not explaining the reasons in the commit.

[len-foss]

@pedrobaeza honestly I'm not very sure.
If you look at

credit-control/account_credit_control/models/credit_control_policy.py

Lines 229 to 230 in c573021

	report = _(
	'Policy "<b>%(name)s</b>" has generated <b>'

, there is no root tag for the test. The regex assumed a <p>, and I got a <span>. I don't think this was relevant to the test (I could add this to the commit message indeed).
This problem wasn't triggered on the runboat, only on my local run (hence why I wrote it's more flexible).

I would not squash the 2 first commits because the second one should be cherry-picked for 17.0. I've done it in #468

[pedrobaeza]

Yes, please include in the commit message.

[len-foss]

@pedrobaeza
There's a way worse issue with the #430, that is explained in details below.

Commit:
odoo/odoo@543a3ad

Contains the comment that

# If the value can't be parsed to an integer then it's computed in an automated way to
# half the size of db_maxconn because while most requests won't borrow cursors concurrently
# there are some exceptions where some controllers might allocate two or more cursors.

So a thread is generally assumed to take at most 2 cursors.
Because of this, it is essentially a very bad idea to open an unlimited number of cursors.
(On a decently sized DB, about 400 lines would be processed in parallel...)

Because things are launched in their own cursor, this doesn't impact the main request,
so the run is marked as done and all lines are marked as queued,
despite the emails never being scheduled.

There is already a way to manage this kind of cases in the OCA that is called queue_job.
Therefore this is the module that should handle this.

If for some reason one would still want the threaded code nonetheless,
it should be put in a custom project module, as it relies on some environment hypothesis
that can't be tested from within the code.

Note that one test is split into two; although it is better to avoid code duplication,
the test was cheating the setup, and therefore not representative of the real user flow is
when users click on the interface buttons.

[pedrobaeza]

@sergio-teruel can you check @len-foss' last comment?

[qgroulard]

Looks good to me (code review).
Much welcome simplification and improvements.

It would be great to have insight from @sergio-teruel though.

[yvaucher]

@pedrobaeza This is approved and ready to be merged if you or @sergio-teruel want to have a look before merge please say so.

[OCA-git-bot]

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). 🤖

[pedrobaeza]

This seems to remove the threaded printing, so @sergio-teruel should check

[yvaucher]

@pedrobaeza would it be ok for you if we merge this with a major version? To me the queue job option in a separate module is way more elegent.

[pedrobaeza]

@sergio-teruel should tell.

[len-foss]

@pedrobaeza done

[pedrobaeza]

I still see too many commits. For example, there's no sense IMO in having 2 commits about account_credit_control_queue_job while you are adding such module here...

[len-foss]

How expensive are commits? I left because it also adds another dependency.
Granted it's part of the same module and everything, but more lines mean more bugs. So having it as an independent commit makes it easier to remove.

[pedrobaeza]

For me the architecture has gone too far, depending on another more module queue_job_batch. Please follow the KISS principle.

[len-foss]

What does this mean? The job batch was added on request of @yvaucher. It's in another module that you don't need to install.

[pedrobaeza]

Yes, but it's the replacement of the threaded one, so for having the same feature now 3 modules are needed instead of one. I don't see the sense. You can keep the threaded feature being not activated by default, or put that on a separate module, or have a simple replacement, but not this complicated dependency chain.

[len-foss]

How is the threading code KISS?
The problem is, it does not work. It only works if your project as a connection pool 3 times bigger than the number of mails you're going to send. This has been a massive problem for end customers.

If you want to have such a mechanism in place, do it in a custom module that you install when you know you can. Having an OCA dependency being a problem in OCA modules is a very strange point of view.

[pedrobaeza]

Well, OK, I'm not saying that, but I abstain of this PR. Proceed as you wish.

Don't agree, but not blocking.

[OCA-git-bot]

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). 🤖

[yvaucher]

/ocabot merge major

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-467-by-yvaucher-bump-major, awaiting test results.

[OCA-git-bot]

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

[yvaucher]

@len-foss thank you for your reactivity. And also thanks to all others involved for their time.

As it was approved I proceeded with the merge.

I also conclude that if someone needs the threaded version back it should go in a separate module.

queue_job is not an easy dependency to setup but in my experience it's a must have in big projects. And it's more likely that big projects need an async feature. So it's good to have it in a separate module.

[len-foss]

Thank you @yvaucher (and @qgroulard ), your help was really appreciated 👍