[18.0][ADD] account_credit_control_aggregate_level - Aggregate multiple levels in credit lines to make single communications#470
Conversation
len-foss
left a comment
len-foss
left a comment
There was a problem hiding this comment.
[ADD] is supposed to be for new modules. Here there's a small [REF] refactoring of the module, and mostly [IMP] improvements to it. Which I'm not sure should go into it and not into another module. The rationale for that is that this module is already quite buggy, so I would think keeping it as lightweight as possible would be best.
yvaucher
force-pushed
the
18.0-credit_control_auto_process_lower_levels-ypt
branch
from
195a0ab to
b47175e
Compare
|
@len-foss Thanks for your input, I'll modify this PR to create a module |
|
@yvaucher that's great! There's another that you're unlikely to catch with simple test: |
yvaucher
changed the title
yvaucher
force-pushed
the
18.0-credit_control_auto_process_lower_levels-ypt
branch
7 times, most recently
from
3b739fb to
926387a
Compare
Add the possibility to mark a policy so that processing a line generated by this policy will also process lower level lines for the same partner (and policy). Also add a filter for credit control line tree view in order to only display highest level lines for each partner and policy.
Revert to the SQL query and extend it with auto_process, ignoring level to group more lines. Revert the yielding method to simply order lines on search to get the highest level in an auto_process group.
yvaucher
force-pushed
the
18.0-credit_control_auto_process_lower_levels-ypt
branch
4 times, most recently
from
8ec67c9 to
12ffa8f
Compare
This is an extraction of the feature added in account_credit_control
qgroulard
force-pushed
the
18.0-credit_control_auto_process_lower_levels-ypt
branch
from
12ffa8f to
a9017b1
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
Bot
added
the
stale
|
@len-foss could you land an updated review on this one? |
yvaucher
removed
the
stale
|
@yvaucher It seems I had a pending review from back then where I forgot to push the submit button. I'm not 100% sure they're all valid, but a quick glance suggests these questions still make sense -- so I'm just going to push the button now. Let me know and I can have another look afterwards. |
| else: | ||
| return self | ||
|
|
||
| def button_schedule_activity(self): |
There was a problem hiding this comment.
Just a question: it seems to have been removed without explanation in e7be29a; should it go back into the module but with a setting to hide the button under certain conditions?
| cls.invoice = invoice_form.save() | ||
| cls.invoice.action_post() | ||
|
|
||
| def test_check_run_date(self): |
There was a problem hiding this comment.
It seems this test is copy-pasted from the original module. It does not seem to provide any value, so it would be better to remove it.
| readonly=True, | ||
| ) | ||
|
|
||
| def _update_aggregation(self, exclude_ids=None): |
There was a problem hiding this comment.
Is there a specific reason you can't use a compute, with a trigger on say communication.line_ids? The module does really weird things with how relations are managed so it's likely there is one, so a comment would be good to know why precisely (since this is making the unorthodox linking situation worse).
| regex_result = re.match(report_regex, control_run.report) | ||
| self.assertIsNotNone(regex_result) | ||
|
|
||
| def test_generate_credit_lines_with_max_level(self): |
There was a problem hiding this comment.
Same thing; please remove things that are copy-pasted so that the tests actually check for the functional changes that this module brings. Duplicated lines have negative value; in particular these tests are more likely to break due to another module. If the tests are shorter they are more useful as documentation.
| def create(self, vals_list): | ||
| lines = super().create(vals_list) | ||
| for line in lines: | ||
| if line.state == "sent": |
There was a problem hiding this comment.
Is this the case of a force_send, or something else?
3 participants
Forward port of #22
In case of recurring invoicing you might have multiple lines with different levels, however you don't want to multiply the communications to the creditor.
This adds the possibility to communicate only the highest due and attach the lower levels to it to not de-multiply the communications.
Compared to
Apply max policy level, the aim is to keep distinct level values on credit lines and not create them at the same level."Auto Process Lower levels" means that you have to care only about the highest level and that other lines for the same partner, policy will be communicated as well.