[16.0] MIG base_product_mass_addition #1512

legalsylvain · 2024-02-09T16:33:05Z

This PR try to finish the migration of base_product_mass_addition.
Previsous work done by @aiendry-aktivsoftware in #1393 that has no activity since July 2023.

✔️ This commit a1d3eac fixes the new V16 error "UserWarning: Field product.product.quick_uom_id in dependency of product.product.quick_uom_category_id should be searchable. This is necessary to determine which records to recompute when uom.uom.category_id is modified. You should either make the field searchable, or simplify the field dependency". (alternative to 16.0 mig base product mass addition #1393 (comment))
✔️ The module works, and can be tested on runboat here [16.0] MIG purchase_quick purchase-workflow#2171 that migrate purchase_quick in V16.
👎 In the meantime, I didn't find a way to avoid the change of LOG_ACCESS_COLUMNS, when writing on the pseudo fields qty_to_process / quick_uom_id.
- This feature was introduced by @ivantodorovich in V14 here : [14.0] [IMP] base_product_mass_addition: avoid writing on product's metadata + add tests #1065
- Current implementation looks dangerous. See 16.0 mig base product mass addition #1393 (comment) by @pedrobaeza
- I tried to overload write function / modified function / change boolean _log_access without success.

@ivantodorovich : Do you have a time to take a look on how to reimplement : #1065 ?

Thanks !

ivantodorovich · 2024-02-14T12:32:08Z

base_product_mass_addition/models/product_product.py

+ quick_fnames = ("qty_to_process", "quick_uom_id")
+ if (
+ self
+ and fnames
+ and any(quick_fname in fnames for quick_fname in quick_fnames)
+ ):
+ for record in self.filtered("id"):
+ towrite = self.env.all.towrite[self._name]
+ vals = towrite[record.id]
+ if not vals: # pragma: no cover
+ continue
+ if all(fname in LOG_ACCESS_COLUMNS for fname in vals.keys()):
+ towrite.pop(record.id)
+ return super().modified(fnames, create=create, before=before)


I take this code doesn't work anymore because of the removal of towrite, right?
I see this has been refactored in odoo/odoo@384fda2

I believe we can keep the same feature, with roughly the same idea, with the necessary changes to adapt for that refactor (using get_dirty_fields and clear_dirty_field instead of directly accessing towrite).

⚠️ I haven't tested this, but I imagine something like this

Suggested change

quick_fnames = ("qty_to_process", "quick_uom_id")

if (

self

and fnames

and any(quick_fname in fnames for quick_fname in quick_fnames)

):

for record in self.filtered("id"):

towrite = self.env.all.towrite[self._name]

vals = towrite[record.id]

if not vals: # pragma: no cover

continue

if all(fname in LOG_ACCESS_COLUMNS for fname in vals.keys()):

towrite.pop(record.id)

return super().modified(fnames, create=create, before=before)

quick_fnames = ("qty_to_process", "quick_uom_id")

if (

self

and fnames

and any(quick_fname in fnames for quick_fname in quick_fnames)

):

if all(field.name in LOG_ACCESS_COLUMNS for field in self.env.cache.get_dirty_fields()):

for fname in LOG_ACCESS_COLUMNS:

self.env.cache.clear_dirty_field(self._fields[fname])

return super().modified(fnames, create=create, before=before)

The previous implementation did this at a record-level: it conditionally cleared the write cache record by record.

The new suggested implementation would do it at the model level: clearing the cache conditionally if only the log access fields are being updated. It's not as smart as the previous one, but it might work in our specific use case (we won't be updating other product fields anyway)

⚠️ again, not tested

In any case, we could re-implement the exact same behavior if we need to, but we'll have to manipulate the self.env.cache._dirty and self.env.cache._data directly IMO

Hi @ivantodorovich. Thanks a lot for your time. (analysis and code proposal)
I tested your implementation (see last fixup commit).

positive point : it works ! I mean, with purchase_quick installed, if change the field qty_to_process field, it is writing on pos.order.line, but doesn't change write_date field 👍

negative point : the test fails. see https://github.com/OCA/product-attribute/actions/runs/7972609836/job/21764713992?pr=1512#step:8:518 👎

Any idea ?

File "/__w/product-attribute/product-attribute/base_product_mass_addition/tests/test_product_mass_addition.py", line 55, in test_quick_should_not_write_on_product self.assertEqual(self.product.write_uid, user_demo) AssertionError: res.users(1,) != res.users(6,)

Hmm, I would put a breakpoint there:

if all( field.name in LOG_ACCESS_COLUMNS for field in self.env.cache.get_dirty_fields() )

And then check why the condition is not matched.

It could be that there's an extra dirty fields in there, during tests. In which case we might need to add some explicit flushes in tests.

Alternatively, it may be due to the simplification of the new implementation:

It's not as smart as the previous one, but it might work in our specific use case (we won't be updating other product fields anyway)

Maybe there are other dirty products in the cache. In which case the solution might be also to flush during tests, or improve the code to restore the previous record-by-record dirty check -- but to do that we'd have to mess with the cache internals as there's no public api for this AFAICS

And then check why the condition is not matched.

i put a breakpoint, and the call of clear_dirty_field is done.
So, self.env.cache.get_dirty_fields() is [write_uid, write_date] at the beginning of the call of modified and when super is executed, it is []. That looks correct. However via test, it doesn't work.

However via test, it doesn't work.

What is the output of self.env.cache.get_dirty_fields() during tests, on that breakpoint? still [write_uid, write_date]? 🤔

Alternatively, try adding a flush after this line: https://github.com/grap/product-attribute/blob/8fd402d307148407b9bee52d754ff4c8daf77197/base_product_mass_addition/tests/test_product_mass_addition.py#L52

Perhaps the issue is there, in the unit tests. As we might be dropping the cache modified by the previous line

Hi as said in the original PR (#1393 (comment)) the tests does not work because it is not possible to change the value of all log_access field.

I wrote a PR to your branch @legalsylvain to fix the test grap#2

legalsylvain · 2024-02-28T11:37:10Z

/ocabot rebase

OCA-git-bot · 2024-02-28T11:37:19Z

Sorry @legalsylvain you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

…ss to product.product may still use the feature

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: product-attribute-15.0/product-attribute-15.0-base_product_mass_addition Translate-URL: https://translation.odoo-community.org/projects/product-attribute-15-0/product-attribute-15-0-base_product_mass_addition/

Currently translated at 100.0% (10 of 10 strings) Translation: product-attribute-15.0/product-attribute-15.0-base_product_mass_addition Translate-URL: https://translation.odoo-community.org/projects/product-attribute-15-0/product-attribute-15-0-base_product_mass_addition/es/

Currently translated at 100.0% (10 of 10 strings) Translation: product-attribute-15.0/product-attribute-15.0-base_product_mass_addition Translate-URL: https://translation.odoo-community.org/projects/product-attribute-15-0/product-attribute-15-0-base_product_mass_addition/it/

[REF] Remove call to flush that are obsolete, in tests.

… the same way, to remove the errror : 'UserWarning: Field product.product.quick_uom_id in dependency of product.product.quick_uom_category_id should be searchable. This is necessary to determine which records to recompute when uom.uom.category_id is modified. You should either make the field searchable, or simplify the field dependency. '

legalsylvain · 2024-03-07T19:42:19Z

@bealdav Could you review this one ? As you need it for sale_quick ? Thanks !

bealdav

LGTM

bealdav · 2024-03-08T10:09:04Z

Thanks a lot @legalsylvain for your decisive implication on this topic

…used images

legalsylvain · 2024-03-08T12:10:45Z

Thanks a lot @legalsylvain for your decisive implication on this topic

you're welcome !

Note : I have updated readme, and I have adopted the module in the last commits.

I think this PR could be merged, CC : @OCA/product-maintainers

legalsylvain · 2024-03-11T08:45:29Z

/ocabot merge nobump

OCA-git-bot · 2024-03-11T08:45:49Z

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

OCA-git-bot · 2024-03-11T08:53:31Z

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

legalsylvain mentioned this pull request Feb 9, 2024

V16 branch looks broken #1513

Closed

legalsylvain changed the title ~~16.0 mig base product mass addition~~ [16.0] MIG base_product_mass_addition Feb 9, 2024

legalsylvain mentioned this pull request Feb 9, 2024

[16.0] MIG purchase_quick OCA/purchase-workflow#2171

Open

1 task

legalsylvain marked this pull request as ready for review February 12, 2024 15:21

ivantodorovich reviewed Feb 14, 2024

View reviewed changes

legalsylvain force-pushed the 16.0-mig-base_product_mass_addition branch from e32dfb5 to 36c70d9 Compare February 28, 2024 11:46

Pierrick Brun and others added 22 commits February 28, 2024 12:47

[ADD] module base_product_mass_addition

4d0a98c

[IMP] do not depend on arbitrary view

0e1c2b0

[IMP] make implementations intercompatible

f89db59

IMP ui and refactor action

1239d58

[FIX] Remove non model-agnostic filter

ff975b1

[UPD] README.rst

7b1e6bf

[UPD] Update base_product_mass_addition.pot

1792526

[ADD] icon.png

2c011b3

[UPD] README.rst

9119db2

[IMP] base_product_mass_addition: black, isort

a530d81

[MIG] base_product_mass_addition: Migration to 13.0

7a7c6d6

[IMP] base_product_mass_addition: black, isort, prettier

146767f

[MIG] base_product_mass_addition: Migration to 14.0

4bef9d2

[IMP] optionnaly use onchange_helper

e222514

[IMP] depend on onchange_helper

5faf9e8

[UPD] Update base_product_mass_addition.pot

fd069d1

[UPD] README.rst

94590cb

[IMP] Add UoM functionality, simplify some code

48d7790

[IMP] Remove purchase-specific element in function name

732bbff

[IMP] Add access rights hack so that users that don't have write acce…

d15d91e

…ss to product.product may still use the feature

[IMP] Remove useless _sync_quick_lines() intermediary

68987b7

[IMP] use related on quick_uom_category_id

2a07d84

angel and others added 13 commits February 28, 2024 12:47

[MIG]base_product_mass_addition: Migration to 15.0

878474e

[UPD] Update base_product_mass_addition.pot

1196609

[UPD] README.rst

1c8c5dc

Added translation using Weblate (Spanish)

058d3c3

[UPD] README.rst

9d59eb3

Added translation using Weblate (Italian)

4e83620

[IMP] base_product_mass_addition: pre-commit execution

3c1e85d

[MIG] base_product_mass_addition: Migration to 16.0

f5511b7

[REF] Remove call to flush that are obsolete, in tests.

[IMP] Adapt modified function to new ORM

c1a9bb4

legalsylvain force-pushed the 16.0-mig-base_product_mass_addition branch from 36c70d9 to c1a9bb4 Compare February 28, 2024 11:49

FranzPoize approved these changes Mar 7, 2024

View reviewed changes

bealdav approved these changes Mar 8, 2024

View reviewed changes

OCA-git-bot added the approved label Mar 8, 2024

legalsylvain added 2 commits March 8, 2024 13:06

[REF] base_product_mass_addition : update documentation and remove un…

745a0a7

…used images

[REF] Add GRAP as coauthor, and adopt a module

1980650

OCA-git-bot added the bot is merging ⏳ label Mar 11, 2024

OCA-git-bot merged commit b8fe68c into OCA:16.0 Mar 11, 2024
7 of 9 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Mar 11, 2024

legalsylvain mentioned this pull request May 26, 2024

16.0 mig base product mass addition #1393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0] MIG base_product_mass_addition #1512

[16.0] MIG base_product_mass_addition #1512

legalsylvain commented Feb 9, 2024 •

edited

Loading

ivantodorovich Feb 14, 2024

legalsylvain Feb 20, 2024

ivantodorovich Feb 20, 2024

legalsylvain Feb 20, 2024

ivantodorovich Feb 20, 2024

FranzPoize Feb 22, 2024 •

edited

Loading

legalsylvain commented Feb 28, 2024

OCA-git-bot commented Feb 28, 2024

legalsylvain commented Mar 7, 2024

bealdav left a comment

bealdav commented Mar 8, 2024

legalsylvain commented Mar 8, 2024

legalsylvain commented Mar 11, 2024

OCA-git-bot commented Mar 11, 2024

OCA-git-bot commented Mar 11, 2024

[16.0] MIG base_product_mass_addition #1512

[16.0] MIG base_product_mass_addition #1512

Conversation

legalsylvain commented Feb 9, 2024 • edited Loading

ivantodorovich Feb 14, 2024

Choose a reason for hiding this comment

legalsylvain Feb 20, 2024

Choose a reason for hiding this comment

ivantodorovich Feb 20, 2024

Choose a reason for hiding this comment

legalsylvain Feb 20, 2024

Choose a reason for hiding this comment

ivantodorovich Feb 20, 2024

Choose a reason for hiding this comment

FranzPoize Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

legalsylvain commented Feb 28, 2024

OCA-git-bot commented Feb 28, 2024

legalsylvain commented Mar 7, 2024

bealdav left a comment

Choose a reason for hiding this comment

bealdav commented Mar 8, 2024

legalsylvain commented Mar 8, 2024

legalsylvain commented Mar 11, 2024

OCA-git-bot commented Mar 11, 2024

OCA-git-bot commented Mar 11, 2024

legalsylvain commented Feb 9, 2024 •

edited

Loading

FranzPoize Feb 22, 2024 •

edited

Loading