[14 0] [IMP] sale_commission: possibility to add payment date limit on settle commissions #441

odooNextev · 2023-07-28T12:21:04Z

This module add a field on wizard that allow to restring commission settle base on payment date. All invoice that are paid after that date will be removed from settle

francesco-ooops

Functional ok!

aleuffre · 2023-08-30T12:29:10Z

sale_commission_settlement_payment_date/README.rst

You can generate the readme using oca-gen-addon-readme

https://github.com/OCA/maintainer-tools#readme-generator

aleuffre · 2023-08-30T12:30:06Z

sale_commission_settlement_payment_date/__manifest__.py

@@ -0,0 +1,13 @@
+{
+    "name": "Commission Settlement base on payment date",


Suggested change

"name": "Commission Settlement base on payment date",

"name": "Commission Settlement based on payment date",

aleuffre · 2023-08-30T12:32:07Z

sale_commission_settlement_payment_date/static/description/index.html

This also need to be generated again (oca-gen-addon-readme will take care of this as well)

aleuffre · 2023-08-30T12:42:53Z

sale_commission_settlement_payment_date/wizard/wizard_settle.py

+        for line in agent_lines.filtered(
+            lambda r: r.commission_id.invoice_state == "paid"
+            and self.date_payment_to != "false"
+        ):


date_payment_to will always be different from the string "false" since it can only be a Date object or the boolean False

In general, I don't think this particular check should go in the lambda since it's not meant to filter agent_lines but it's meant to check if the filtering should happen at all. Additionally I'd group the other checks together since they're not logically separate.

if self.date_payment_to: for line in agent_lines.filtered(lambda r: r.commission_id.invoice_state == "paid" and r.invoice_id.invoice_payments_widget and not self.check_payment_date(r)):

or

if self.date_payment_to: for line in agent_lines: if line.commission_id.invoice_state == "paid" and line.invoice_id.invoice_payments_widget and not self.check_payment_date(line):

(and then let Black format that of course :) )

Here condition on "false" it's my mistake. I will change it and re-publish all changes

aleuffre · 2023-08-30T12:46:54Z

sale_commission_settlement_payment_date/wizard/wizard_settle.py

+            and self.date_payment_to != "false"
+        ):
+            if (
+                line.invoice_id.invoice_payments_widget != "false"


Same as above. It's a text field. It'll probably be empty ("" An empty string) or False, but never the string "false". Might as well just do an if line.invoice_id.invoice_payments_widget

@aleuffre Thanks for your review. I know that sound weird, but invoice_payments_widget it is possible to be a string "false".

ok, that's interesting haha

It happens when an invoice is not paid yet. I could change condition on invoice.state without checking invoice_payments_widget. What could you recommend me?

aleuffre

Code looks good, not sure about the test

aleuffre · 2023-09-06T10:56:05Z

sale_commission_settlement_payment_date/tests/test_commission_payment_date.py

+    def test_commission_single_invoice(self):
+        sale_order = self._create_sale_order(
+            self.agent_monthly, self.commission_net_paid
+        )
+        sale_order.action_confirm()
+        self._invoice_sale_order(sale_order)
+        date = fields.Date.today()
+        sale_order.invoice_ids.write(
+            {
+                "invoice_date": date + relativedelta(months=-1),
+                "date": date + relativedelta(months=-1),
+            }
+        )
+        sale_order.invoice_ids.action_post()
+        sale_order = self._create_sale_order(
+            self.agent_monthly, self.commission_net_paid
+        )
+        sale_order.action_confirm()
+        inv = self._invoice_sale_order(sale_order)
+        self.register_payment(inv)
+        self._settle_agent(self.agent_monthly, 1)
+        settlements = self.env["sale.commission.settlement"].search(
+            [
+                (
+                    "agent_id",
+                    "=",
+                    self.agent_monthly.id,
+                ),
+                ("state", "=", "settled"),
+            ]
+        )
+        self.assertEqual(1, len(settlements))


I'm not entirely sure, so please correct me if I'm wrong, but it seems to me that this test only creates a single payment and checks that it's included in the wizard.

You could improve the test by creating two payments (or two sales orders with one payment each, if it's easier), one in the past (as the one you've already created) and one in the future, and making sure that when you call the wizard, only the one before date_payment_to is actually included in the wizard and the other is not.

As it is, if I understand correctly, your module could do nothing and this test would still pass.

aleuffre

Code review, LGTM

aleuffre · 2023-09-08T15:10:35Z

Only thing, try to make the first line of the commit message shorter! It should be less than 80 characters in total.

You can add more details in other lines.

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#id48

OCA-git-bot · 2023-09-08T15:19:32Z

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

francesco-ooops · 2023-09-08T15:19:42Z

@pedrobaeza what do you think?

francesco-ooops · 2023-09-13T08:45:26Z

@pedrobaeza good for merge?

pedrobaeza · 2023-09-13T09:47:59Z

In this case, I consider reasonable to add this to the base module, but if you want to keep it as separate module, no problem.

francesco-ooops · 2023-09-13T10:01:30Z

We'll gladly turn this PR into an IMP to sale_commission :)

@odooNextev can you proceed?

pedrobaeza · 2023-09-13T10:16:14Z

Try to make it in one shot excluding from the initial domain instead of removing them later.

aleuffre

Generally code review LGTM.

There's some minor code cleanup that could be done (in one file, relativedelta is imported twice, with one import overwriting the other; also it's slightly confusing to be using both timedelta and relativedelta, especially when the latter is much nicer to use) but it doesn't really matter.

I'd echo the request by Pedro Baeza of trying to move as much of the search as possible to the domain in the .search method so it's done in the DB directly rather than in python, but I don't know how to do get the payment date info so I can't give advice there without looking into it in more detail.

aleuffre · 2023-09-15T08:39:34Z

Oh, and as always, make sure to squash into a single commit, and follow OCA guidelines for the commit message

odooNextev · 2023-09-15T09:03:21Z

Hi @aleuffre, thanks for your review. I made some cleanup that you recommended to me. I tried to move everything inside a single domain but it seems, in my opinion, not possible.

aleuffre · 2023-09-15T09:12:25Z

sale_commission/wizard/wizard_settle.py

+        if self.date_payment_to:
+            aila = aila.filtered(
+                lambda line: line.commission_id.invoice_state != "paid"
+                or line.invoice_id.invoice_payments_widget == "false"
+                or self.check_payment_date(line)
+            )


If you want to limit lines to a certain payment date, aren't you with this including all lines that aren't paid as well?

I changed it and now I think that's works properly. Is it possible to call check_payment_date method inside domain?

unfortunately not. Domains inside a .search are translated into SQL and passed to the DB directly. It's also why you can't use fields that are computed and not stored, since they don't exist as columns in the DB.

aleuffre · 2023-09-18T15:45:01Z

sale_commission/wizard/wizard_settle.py

+        if self.date_payment_to:
+            aila = aila.filtered(
+                lambda line: not (
+                    line.commission_id.invoice_state == "paid"
+                    and line.invoice_id.invoice_payments_widget != "false"
+                    and not self.check_payment_date(line)
+                )
+            )
        return aila
+
+    def check_payment_date(self, agent_line):
+        move_dict_content = json.loads(agent_line.invoice_id.invoice_payments_widget)[
+            "content"
+        ]
+        return all(
+            [
+                self.date_payment_to > datetime.strptime(x["date"], "%Y-%m-%d").date()
+                for x in move_dict_content
+            ]
+        )


Done some digging into this in order to get around using the widget.

As far as I can tell there is no simple field that tells you when an invoice is fully paid. But you can get all the payments associated with an invoice (self.env["account.payment"].search("reconciled_invoice_ids", "in", invoice.id) and then check the date on those.

I also think that this type of check shouldn't be done here but higher up in this file (lines 103 and following).

There's also a function _skip_settlement that does part of the filtering you already want. So perhaps another if condition: continue up there, right after _skip_settlement would make the most sense.

Also, I'd like to point out that according to the existing logic of _skip_settlement, it seems to me (please correct me if I'm wrong) that as long as the invoice is partially paid, the commission settlement is created in full (for commissions of type "Payment Based") so I think we should respect that for now and maybe change the logic in a future PR

aleuffre

Looks clean, finally! :D

Just squash into a single commit following OCA conventions please.

francesco-ooops · 2023-09-19T15:01:27Z

Try to make it in one shot excluding from the initial domain instead of removing them later.

@pedrobaeza could it work like this?

pedrobaeza · 2023-09-19T16:28:36Z

sale_commission/wizard/wizard_settle.py

@@ -12,6 +12,8 @@ class SaleCommissionMakeSettle(models.TransientModel):
    _description = "Wizard for settling commissions in invoices"

    date_to = fields.Date("Up to", required=True, default=fields.Date.today())
+    date_payment_to = fields.Date("Date Payment Up to", default=fields.Date.today())


The name of the field and the label is not very intuitive. Also non having help doesn't help, hehe. Can you please look for other name and put a help for this new field.

@pedrobaeza since this is becoming a feature of base module, I would suggest changing labels of both date fields in the wizard for consistency:

Invoice date up to
Payment date up to

shall we proceed?

https://github.com/OCA/commission/pull/441/files#diff-7dfa7dd0fc23295fc2cd2da1c87e3535d33ddbf4480da661bdab6a3eaab7ea51R14

@pedrobaeza go?

pedrobaeza

Thanks for the changes

/ocabot merge minor

OCA-git-bot · 2023-09-20T11:10:35Z

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-441-by-pedrobaeza-bump-minor, awaiting test results.

OCA-git-bot · 2023-09-20T11:26:21Z

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

odooNextev mentioned this pull request Aug 25, 2023

[IMP][14.0]l10n_it_riba_sale_commission: generazione provvigioni con data pagamento OCA/l10n-italy#3540

Merged

francesco-ooops approved these changes Aug 29, 2023

View reviewed changes

aleuffre suggested changes Aug 30, 2023

View reviewed changes

aleuffre reviewed Sep 6, 2023

View reviewed changes

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch 5 times, most recently from b08724a to f0a7af4 Compare September 8, 2023 14:03

aleuffre approved these changes Sep 8, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 8, 2023

OCA-git-bot added the ready to merge label Sep 8, 2023

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from f0a7af4 to 9943923 Compare September 8, 2023 15:22

pedrobaeza added this to the 14.0 milestone Sep 13, 2023

odooNextev changed the title ~~[14 0][Add]Sale_commission_payment_date: possibility to add payment date limit on settle commissions~~ [14 0][Imp]Sale_commission: possibility to add payment date limit on settle commissions Sep 15, 2023

aleuffre approved these changes Sep 15, 2023

View reviewed changes

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from baa7a34 to e110826 Compare September 15, 2023 08:55

aleuffre reviewed Sep 15, 2023

View reviewed changes

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch 3 times, most recently from 05651ca to bc3cc6a Compare September 15, 2023 09:36

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from bc3cc6a to a83f29b Compare September 15, 2023 09:40

aleuffre reviewed Sep 18, 2023

View reviewed changes

aleuffre approved these changes Sep 19, 2023

View reviewed changes

odooNextev changed the title ~~[14 0][Imp]Sale_commission: possibility to add payment date limit on settle commissions~~ [14 0] [IMP] Sale_commission: possibility to add payment date limit on settle commissions Sep 19, 2023

odooNextev changed the title ~~[14 0] [IMP] Sale_commission: possibility to add payment date limit on settle commissions~~ [14 0] [IMP] sale_commission: possibility to add payment date limit on settle commissions Sep 19, 2023

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from 1a6f3a4 to 217c77b Compare September 19, 2023 14:56

pedrobaeza reviewed Sep 19, 2023

View reviewed changes

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from 217c77b to ad8f775 Compare September 20, 2023 10:22

[IMP]sale_commission:settle commissions based on payment date

9ac7706

odooNextev force-pushed the 14-0-add-sale_commission_payment_date branch from ad8f775 to 9ac7706 Compare September 20, 2023 10:30

pedrobaeza approved these changes Sep 20, 2023

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Sep 20, 2023

OCA-git-bot merged commit 16c87ad into OCA:14.0 Sep 20, 2023
6 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Sep 20, 2023

		@@ -0,0 +1,13 @@
		{
		"name": "Commission Settlement base on payment date",

	"name": "Commission Settlement base on payment date",
	"name": "Commission Settlement based on payment date",

[14 0] [IMP] sale_commission: possibility to add payment date limit on settle commissions #441

[14 0] [IMP] sale_commission: possibility to add payment date limit on settle commissions #441

Conversation

odooNextev commented Jul 28, 2023 • edited Loading

francesco-ooops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre left a comment

Choose a reason for hiding this comment

aleuffre commented Sep 8, 2023

OCA-git-bot commented Sep 8, 2023

francesco-ooops commented Sep 8, 2023

francesco-ooops commented Sep 13, 2023

pedrobaeza commented Sep 13, 2023

francesco-ooops commented Sep 13, 2023

pedrobaeza commented Sep 13, 2023

aleuffre left a comment • edited Loading

Choose a reason for hiding this comment

aleuffre commented Sep 15, 2023

odooNextev commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

aleuffre Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

aleuffre left a comment

Choose a reason for hiding this comment

francesco-ooops commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Sep 20, 2023

OCA-git-bot commented Sep 20, 2023

odooNextev commented Jul 28, 2023 •

edited

Loading

aleuffre Aug 30, 2023 •

edited

Loading

aleuffre left a comment •

edited

Loading

aleuffre Sep 18, 2023 •

edited

Loading

aleuffre Sep 18, 2023 •

edited

Loading