[16.0] [MIG] eater #521

victor-champonnois · 2023-08-03T12:47:52Z

internal task : https://gestion.coopiteasy.be/web#id=10884&action=475&active_id=492&model=project.task&view_type=form&menu_id=

I need technical input on two questions : bca5176 and 9902d88

@polchampion Since the field "customer" has been removed in v13, I removed

the restriction on customer in the domain for adding new eaters.
The invisibility of the field for non-customer contact : so the field Eater/worker is now visible on all contacts

This is not ideal, but I don't see a better solution. We could use the customer_rank field, which starts at zero and increases with the contact's orders. But this would mean that new contacts or contact that never shopped could not been added as Eater.
Maybe we could make the field invisible for companies ?

polchampion · 2023-08-03T13:46:40Z

@victor-champonnois I think it's fine as you did, let's keep it simple. Can you ping me when it's live in test?

cathLemb · 2023-08-21T13:07:41Z

"Maybe we could make the field invisible for companies ?"
Not possible as bees for example uses eater field on companies.

carmenbianca · 2023-09-06T09:27:50Z

I just realised that #516 is not is not included in this PR, and that the changes to customer will conflict with some changes. I'll fix up #516 and then rebase this work on top of that later down the line.

carmenbianca

some notes

carmenbianca · 2023-09-06T13:49:14Z

eater/static/font/Roboto-Bold.ttf

This font (and some other static resources) are deleted in the 13.0 migration commit. Why?

I could have removed it in a separate commit. It's just that it's not used and it makes no sense to keep this file here.

carmenbianca · 2023-09-06T13:49:44Z

eater/views/partner.xml

-                    attrs="{'invisible': ['|', ('customer', '=', False), ('eater', 'not in', ('worker_eater'))]}"
+                    attrs="{'invisible': [('eater', 'not in', ('worker_eater'))]}"


This should be in the 13.0 migration commit.

carmenbianca · 2023-09-06T13:49:50Z

eater/views/partner.xml

-                <field name="eater" attrs="{'invisible': [('customer', '=', False)]}" />
+                <field name="eater" />


carmenbianca · 2023-09-06T13:50:45Z

eater/models/partner.py

-                and partner.parent_eater_id.id != values.get("parent_eater_id")
+                and partner.parent_eater_id != values.get("parent_eater_id")


This makes no sense to me.

carmenbianca · 2023-09-06T13:52:16Z

eater/models/partner.py

+        # fixme: shouldn't the check on "values.get("parent_eater_id")" be before
+        # the for loop to avoid looping through all the contacts at every write?


carmenbianca · 2023-09-06T13:59:04Z

eater/models/partner.py

-        # replace many2many command when writing on child_eater_ids to just
-        # remove the link
+        # fixme : is this still necessary ? In a many2many widget, removing an item
+        # is a "3" (unlink) command. In which case would this be a delete command ?
+        # replace many2many command when writing on child_eater_ids to unlink
+        # rather than delete
        if "child_eater_ids" in values:
            for command in values["child_eater_ids"]:
                if command[0] == 2:


I guess it depends on why this exists. The apparent goal to me is 'don't accidentally delete the linked child eaters when severing the link'. But why would the '2' command ever be sent when the '3' command is intended?

For context:

(2, id, _) removes the link to and deletes the id related record.

(3, id, _) removes the link to, but does not delete, the id related record.
This is usually what you will use to delete related records on many-to-many
fields.

In any case, if we keep this line, a test should be added.

I would be in favor of removing it then. I don't see why this would be needed for this specific many2many field. Do you agree ?

It's not a Many2many field. Not sure why the comment calls it a 'many2many command'.

I'm happy to drop this code also. Let's just make a note of it somewhere that the functional tester can be aware of this.

(note in the task)

robinkeunen · 2023-10-06T15:20:06Z

The migration commits should be stashed together.

huguesdk · 2023-10-09T07:22:59Z

isn’t the module name too generic for such a specific module?

[UPD] Update member_card.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[UPD] Update eater.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[UPD] Update eater_member_card.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] put splitted modules as dependencies [FIX] test-requirements [FIX] remove beesdoo_base demo data in manifest Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[FIX] contributors [FIX] remove deprecated demo data Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

We do not need to rename view names when moved from one module to the other. Odoo cleans up by himself as long as they are not defined as noupdate. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

into: - product_main_supplier - sale_suggested_price - sale_adapt_price_wizard [REF] split beesdoo_product -> product_main_supplier [FIX] sale_suggested_price dep on product_main_supplier [ADD] module sale_edit_price_wizard [REF] rename sale_edit_price_wizard -> sale_adapt_price_wizard [REF] move rounding_method to sale_suggested_price [REF] refactor following PR 320 [REF] search product_code -> product_main_seller [FIX] bug adapt_sales_price [IMP] use float_compare to compare prices [REF] move list_price_write_date to sale_adapt_price_wizard [FIX] beesdoo_product: main_seller_id search [FIX] run precommit fix Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

carmenbianca

I rebased this on top of the latest 12.0 to also include #516. I also applied a fix at the end, but I'm not entirely sure why the code that I fixed exists.

I'd like a confirmation that the tagged code is useful before we merge this.

carmenbianca · 2023-10-09T09:25:15Z

eater/models/partner.py

+        if values.get("parent_eater_id"):
+            for partner in self:
+                if (
+                    partner.parent_eater_id
+                    and partner.parent_eater_id.id != values.get("parent_eater_id")
+                ):
+                    raise ValidationError(
+                        _(
+                            "You are trying to assign %(eater)s as an eater to"
+                            " a new partner, but this eater is already"
+                            " assigned to %(old_partner)s. Please remove the link"
+                            " before creating a new one."
+                        )
+                        % {
+                            "eater": partner.name,
+                            "old_partner": partner.parent_eater_id.name,
+                        }
+                    )


What is the use case here?

You can't be the eater of two workers.

But this would change the parent eater ("worker"), no? So after the operation, you still only have one parent.

Indeed, that's weird.

It dates back from 8 years ago. Let's trust ourselves and state that this is useless. 3f4d9f6

codecov-commenter · 2023-10-09T09:53:21Z

Codecov Report

Merging #521 (3d7f339) into 16.0 (4803b52) will increase coverage by 1.31%.
The diff coverage is 93.75%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             16.0     #521      +/-   ##
==========================================
+ Coverage   66.26%   67.58%   +1.31%     
==========================================
  Files          45       53       +8     
  Lines        1592     1672      +80     
  Branches      217      230      +13     
==========================================
+ Hits         1055     1130      +75     
- Misses        495      500       +5     
  Partials       42       42

Files	Coverage Δ
eater/__init__.py	`100.00% <100.00%> (ø)`
eater/models/__init__.py	`100.00% <100.00%> (ø)`
eater/tests/__init__.py	`100.00% <100.00%> (ø)`
eater/tests/test_eater.py	`100.00% <100.00%> (ø)`
eater/tests/test_partner.py	`100.00% <100.00%> (ø)`
eater/wizard/__init__.py	`100.00% <100.00%> (ø)`
eater/models/partner.py	`92.00% <92.00%> (ø)`
eater/wizard/new_eater_wizard.py	`76.92% <76.92%> (ø)`

robinkeunen · 2023-10-24T12:48:40Z

eater/tests/test_eater.py

@@ -6,15 +6,15 @@ class TestEater(TransactionCase):
    def test_assign_parent_eater(self):
        self.assertTrue(
            self.env.ref("eater.eater2").write(
-                {"parent_eater_id": self.env.ref("eater.eater1")}
+                {"parent_eater_id": self.env.ref("eater.worker1").id}


it's better practice to create records in the tests and leave demo data for demo.

robinkeunen · 2023-10-24T12:49:04Z

eater/demo/eaters.xml

+    <record id="worker1" model="res.partner">
+        <field name="name">Gijs Eaterdam</field>
+        <field name="firstname">Gijs</field>
+        <field name="lastname">Eaterdam</field>
+        <field name="eater">worker_eater</field>
+        <field name="is_company" eval="False" />
+    </record>


It's better to overload existing partners than creating new ones.

carmenbianca

LGTM @robinkeunen

This check is not necessary. In a many2many widget, removing an item is a "3" (unlink) command. There doesn't seem to be a case where it would be a "2" command.

The check wasn't quite right (it compared the recordset to an id). The English was also really bad. Patched that up. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

robinkeunen · 2023-10-24T14:21:17Z

/ocabot merge nobump

github-grap-bot · 2023-10-24T14:21:23Z

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

github-grap-bot · 2023-10-24T14:27:13Z

Congratulations, your PR was merged at 48ba329. Thanks a lot for contributing to beescoop. ❤️

victor-champonnois requested review from huguesdk, robinkeunen, carmenbianca and remytms August 3, 2023 12:47

github-grap-bot added the needs review label Aug 3, 2023

carmenbianca mentioned this pull request Sep 6, 2023

[FIX] eater: Add restraints on parent and child eaters #516

Merged

4 tasks

carmenbianca requested changes Sep 6, 2023

View reviewed changes

victor-champonnois force-pushed the 16.0-mig-eater branch from 3ae5b3a to fc277e7 Compare September 7, 2023 08:31

victor-champonnois requested a review from carmenbianca September 7, 2023 08:31

robinkeunen approved these changes Oct 6, 2023

View reviewed changes

victor-champonnois and others added 15 commits October 9, 2023 10:53

[REF] split beesdoo_base -> member_card

e7765df

[UPD] Update member_card.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] split beesdoo_base -> eater

2ffa9dc

[UPD] Update eater.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] split beesdoo_base -> eater_member_card

9be273c

[UPD] Update eater_member_card.pot Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] split beesdoo_base : fix dependencies

2fea183

[REF] put splitted modules as dependencies [FIX] test-requirements [FIX] remove beesdoo_base demo data in manifest Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REM] deprecated functionalities

9b6a991

[FIX] contributors [FIX] remove deprecated demo data Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] beesdoo_base: remove view renames

014d1b0

We do not need to rename view names when moved from one module to the other. Odoo cleans up by himself as long as they are not defined as noupdate. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] eater: rename eater.new.wizard

44c1754

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] eater: dependencies to point_of_sale, purchase

e93d0f8

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[REF] member_card: rename membercard.new.wizard

735d26e

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[IMP] generate POT and PO files

acc3843

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[IMP] update french translations

716901b

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[UPD] README.rst

c92f781

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[IMP] translations

dc7749a

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[FIX] fix fr translation

133c69c

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[MIG] eater: Migration to 14.0

edb70be

carmenbianca force-pushed the 16.0-mig-eater branch from fc277e7 to 5b1dfaf Compare October 9, 2023 09:24

carmenbianca requested changes Oct 9, 2023

View reviewed changes

robinkeunen requested changes Oct 24, 2023

View reviewed changes

carmenbianca self-requested a review October 24, 2023 13:51

carmenbianca approved these changes Oct 24, 2023

View reviewed changes

victor-champonnois and others added 11 commits October 24, 2023 15:53

[MIG] eater: Migration to 15.0

779c7d7

[MIG] eater: Migration to 16.0

38848db

[FIX] eater: condition on parent_eater_id

4ab1799

[ADD] suggestion to improve performance

542c883

[ADD] improve comment and propose removal

9c0e431

[ADD] eater: tests

5112427

[IMP] perf imp: check for parent eater before loop

f6b7e67

[REM] no-delete on child_eater_ids many2many

3356eb9

This check is not necessary. In a many2many widget, removing an item is a "3" (unlink) command. There doesn't seem to be a case where it would be a "2" command.

[FIX] eater: Improve English, fix check

9cce452

The check wasn't quite right (it compared the recordset to an id). The English was also really bad. Patched that up. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

[IMP] eater: do not display tab if partner not eater/worker

aedd3bd

[REF] eater: do not raise validation error on parent worker update

d8fc3f3

robinkeunen force-pushed the 16.0-mig-eater branch from 4d6e3b8 to d8fc3f3 Compare October 24, 2023 13:53

robinkeunen self-requested a review October 24, 2023 13:53

robinkeunen approved these changes Oct 24, 2023

View reviewed changes

github-grap-bot added the approved label Oct 24, 2023

github-grap-bot added the bot is merging ⏳ label Oct 24, 2023

github-grap-bot merged commit 80eadce into 16.0 Oct 24, 2023
2 checks passed

github-grap-bot deleted the 16.0-mig-eater branch October 24, 2023 14:27

github-grap-bot added merged 🎉 and removed bot is merging ⏳ labels Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0] [MIG] eater #521

[16.0] [MIG] eater #521

victor-champonnois commented Aug 3, 2023 •

edited

Loading

polchampion commented Aug 3, 2023

cathLemb commented Aug 21, 2023

carmenbianca commented Sep 6, 2023

carmenbianca left a comment

carmenbianca Sep 6, 2023

victor-champonnois Sep 7, 2023

carmenbianca Sep 6, 2023

victor-champonnois Sep 7, 2023

carmenbianca Sep 6, 2023

victor-champonnois Sep 7, 2023

carmenbianca Sep 6, 2023

carmenbianca Sep 6, 2023

victor-champonnois Sep 7, 2023

carmenbianca Sep 6, 2023

victor-champonnois Sep 7, 2023

carmenbianca Sep 7, 2023

victor-champonnois Sep 7, 2023

victor-champonnois Sep 7, 2023

robinkeunen commented Oct 6, 2023

huguesdk commented Oct 9, 2023

carmenbianca left a comment

carmenbianca Oct 9, 2023

robinkeunen Oct 9, 2023

carmenbianca Oct 9, 2023

robinkeunen Oct 24, 2023

robinkeunen Oct 24, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023

robinkeunen Oct 24, 2023

robinkeunen Oct 24, 2023

carmenbianca left a comment

robinkeunen commented Oct 24, 2023

github-grap-bot commented Oct 24, 2023

github-grap-bot commented Oct 24, 2023

		attrs="{'invisible': ['\|', ('customer', '=', False), ('eater', 'not in', ('worker_eater'))]}"
		attrs="{'invisible': [('eater', 'not in', ('worker_eater'))]}"

		<field name="eater" attrs="{'invisible': [('customer', '=', False)]}" />
		<field name="eater" />

		and partner.parent_eater_id.id != values.get("parent_eater_id")
		and partner.parent_eater_id != values.get("parent_eater_id")

		# fixme: shouldn't the check on "values.get("parent_eater_id")" be before
		# the for loop to avoid looping through all the contacts at every write?

[16.0] [MIG] eater #521

[16.0] [MIG] eater #521

Conversation

victor-champonnois commented Aug 3, 2023 • edited Loading

polchampion commented Aug 3, 2023

cathLemb commented Aug 21, 2023

carmenbianca commented Sep 6, 2023

carmenbianca 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

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

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

robinkeunen commented Oct 6, 2023

huguesdk commented Oct 9, 2023

carmenbianca 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

Choose a reason for hiding this comment

robinkeunen Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 9, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmenbianca left a comment

Choose a reason for hiding this comment

robinkeunen commented Oct 24, 2023

github-grap-bot commented Oct 24, 2023

github-grap-bot commented Oct 24, 2023

victor-champonnois commented Aug 3, 2023 •

edited

Loading

robinkeunen Oct 24, 2023 •

edited

Loading