[16.0][MIG] stock_picking_analytic: Migration to 16.0 #527

yostashiro · 2023-01-21T14:06:07Z

Switch field from analytic_account_id to analytic_distribution.
Change depends referenced field from move_ids_without_package to move_ids. move_ids_without_package is a non-stored compute field and therefore is not appropriate to be used as a trigger (you get a warning message in running tests without this change). Add dummy search arg to move_ids_without_package to bypass warning.
Remove groups="analytic.group_analytic_accounting" from the analytic_distribution field added in the picking form, because it would cause an error Field 'analytic_distribution' used in context ({'default_company_id': company_id, 'default_date': scheduled_date, 'default_date_deadline': date_deadline, 'picking_type_code': picking_type_code, 'default_picking_id': id, 'form_view_ref': 'stock.view_move_form', 'address_in_id': partner_id, 'default_picking_type_id': picking_type_id, 'default_location_id': location_id, 'default_location_dest_id': location_dest_id, 'default_partner_id': partner_id, 'default_analytic_distribution': analytic_distribution}) is restricted to the group(s) analytic.group_analytic_accounting in loading the view. Looks like this refactoring odoo/odoo@0501bbd has added the check. Not sure there is a good way to still add the group to the field, however there is not much point doint it since the module wouldn't be installed without enabling the analytic accounting. -> Add an invisible analytic_distribution field without group assignment (thanks @luisg123v ).
~~Extension of the search view to add analytic_distribution field is disabled as the search on json fields do not work in a meaningful manner as of now.~~

github-actions · 2023-05-28T12:31:59Z

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

rafaelbn · 2023-06-01T01:01:24Z

/ocabot migration stock_picking_analytic

rousseldenis · 2023-06-01T07:29:38Z

@yostashiro Shouldn't this deserves a migration script ?

yostashiro · 2023-06-01T12:35:29Z

@yostashiro Shouldn't this deserves a migration script ?

@rousseldenis I was thinking no, as I thought the whole point of having analytic account (analytic distribution now) in the picking was to propagate the value to stock moves, and few people would care about the data persistency of itself (but I might be wrong). If anything, migration script should be added in https://github.com/OCA/account-analytic/tree/16.0/stock_analytic for analytic distribution of stock moves?

desdelinux

Functional review is ok.

cc @rolandojduartem @rousseldenis @luisg123v

desdelinux · 2023-06-20T18:59:00Z

@yostashiro Shouldn't this deserves a migration script ?

@rousseldenis I was thinking no, as I thought the whole point of having analytic account (analytic distribution now) in the picking was to propagate the value to stock moves, and few people would care about the data persistency of itself (but I might be wrong). If anything, migration script should be added in https://github.com/OCA/account-analytic/tree/16.0/stock_analytic for analytic distribution of stock moves?

@rousseldenis I agree with this

rolandojduartem · 2023-06-20T19:12:25Z

LGTM +1

OCA-git-bot · 2023-06-20T21:00:02Z

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

luisg123v

You're including commits that are not merged in the stable branch, some of which are even WIP:

Those are migration commits to 15.0, but this module is not migrated to 15.0 yet. I'd suggest either:

removing those ones from this migration
Or creating (first) a migration PR to 15.0 and, after that one is merged, resume this one

On the other hand:

"pre-commit" -> "pre-commit stuff"

luisg123v · 2023-06-21T22:27:04Z

stock_picking_analytic/models/stock_picking.py

+    # Assign a dummy search arg to the field to avoid triggering the warning.
+    # Without this, use of move_ids_without_package in @api.depends() of the compute
+    # method would trigger a warning from
+    # https://github.com/odoo/odoo/blob/ae94f14a844352f66490ec326fe2d1a716023891/odoo/fields.py#L808-L812  # noqa


If you use a shorter hash, line won't be as long and noqa won't be required, e.g.:

Suggested change

# https://github.com/odoo/odoo/blob/ae94f14a844352f66490ec326fe2d1a716023891/odoo/fields.py#L808-L812 # noqa

# https://github.com/odoo/odoo/blob/ae94f14a/odoo/fields.py#L808-L812

luisg123v · 2023-06-21T22:30:29Z

stock_picking_analytic/tests/test_picking_analytic_account.py

+                "move_ids": [
+                    (
+                        0,
+                        0,


Prefer the Command syntax

Same below

luisg123v · 2023-06-21T22:31:56Z

stock_picking_analytic/tests/test_picking_analytic_account.py

+        Check analytic distribution on picking is set
+        """
+        picking = self.picking
+        self.assertNotEqual(len(picking.move_ids), 0)


Suggested change

self.assertNotEqual(len(picking.move_ids), 0)

self.assertTrue(picking.move_ids)

Same below

luisg123v · 2023-06-21T22:35:14Z

stock_picking_analytic/views/stock_picking.xml

+        <field name="inherit_id" ref="stock.view_picking_form" />
+        <field name="arch" type="xml">
+            <xpath expr="//group/group/field[@name='picking_type_id']" position="after">
+                <!-- groups="analytic.group_analytic_accounting" has been removed from


You just need to add the field again as invisible, so the field is included no matter if current user belongs to the group

luisg123v · 2023-06-21T22:54:22Z

PR description is a comprehensive one, it would be a shame if it's not included also in commit description.

desdelinux · 2023-07-06T00:29:28Z

@yostashiro Hello, you can apply the suggested changes

yostashiro · 2023-07-06T03:12:49Z

@luisg123v @desdelinux Thanks for your review comments. Will try and follow up on them but give me some time. A bit swamped at the moment.

rousseldenis · 2023-07-19T11:40:47Z

@yostashiro Any news on this ?

yostashiro · 2023-07-20T06:10:10Z

I plan to update the PR this weekend.

…of erasing it

…in depends + tests

yostashiro · 2023-07-24T03:43:29Z

@luisg123v @desdelinux @rousseldenis Thanks for waiting. Just updated the PR. I believe I took care of all the points in the feedback.

desdelinux

Functional Review is ok.

cc @luisg123v @rousseldenis

rolandojduartem · 2023-07-27T16:08:42Z

@luisg123v could you review this PR?

luisg123v · 2023-08-01T23:39:33Z

stock_picking_analytic/views/stock_picking.xml

+                    widget="analytic_distribution"
+                    groups="analytic.group_analytic_accounting"
+                />
+                <!-- Just to prevent error in subsequent context update - assigining


Typo: assigining -> assigning

But I think this comment should be removed, this is not a special case but just the way v16 works. Most of views containing fields with groups (e.g. almost all views with the company_id field) define the same field as invisible.

A similar case occurred in previous versions when using a field in attrs that was not already present in the view

luisg123v · 2023-08-01T23:40:26Z

stock_picking_analytic/views/stock_picking.xml

+                an invisible field that is accessible to everyone. -->
+                <field
+                    name="analytic_distribution"
+                    widget="analytic_distribution"


Since it's invisible, widget is meaningless

luisg123v · 2023-08-01T23:48:50Z

stock_picking_analytic/tests/test_picking_analytic_account.py

        )
-        picking.move_ids_without_package.write(
+        picking.move_ids_without_package.update(


Why update instead of write?

write is preferred, because update writes fields one by one

Same below

write() would throw an error for picking.move_ids_without_package not having the database record when tests started. I have switched to assign move_ids instead of move_ids_without_package to the picking in the setup to use write() in tests.

- Switch field from analytic_account_id to analytic_distribution. - Assign a dummy search arg to move_ids_without_package field (stock.picking) to bypass the warning from https://github.com/odoo/odoo/blob/0f84366/odoo/fields.py#L808-L812. - Add an invisible analytic distribution field without group assignment in the picking form, due to newly added check in odoo/odoo@0501bbd.

desdelinux

Functional Review is ok.

@rousseldenis

desdelinux · 2023-08-04T14:51:35Z

@OCA/accounting-maintainers This one is ready

luisg123v

LGTM 👍

luisg123v · 2023-08-08T00:59:46Z

@moylop260,

Could you review/merge, please?

Regards,

OCA-git-bot · 2023-08-08T01:00:02Z

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

moylop260 · 2023-08-08T01:09:52Z

/ocabot merge nobump

OCA-git-bot · 2023-08-08T01:11:20Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-527-by-moylop260-bump-nobump, awaiting test results.

yostashiro force-pushed the 16.0-mig-stock_picking_analytic branch 2 times, most recently from 4718794 to 88a8944 Compare January 21, 2023 14:29

yostashiro changed the title ~~[16.0][MIG] stock_picking_analytic~~ [16.0][MIG] stock_picking_analytic: Migration to 16.0 Jan 21, 2023

yostashiro force-pushed the 16.0-mig-stock_picking_analytic branch from 88a8944 to 8b8ae36 Compare January 21, 2023 16:08

yostashiro mentioned this pull request Jan 23, 2023

[15.0][MIG]stock_picking_analytic: Migration to 15.0 #520

Closed

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 28, 2023

rafaelbn removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 1, 2023

rafaelbn added the needs review label Jun 1, 2023

OCA-git-bot added this to the 16.0 milestone Jun 1, 2023

OCA-git-bot mentioned this pull request Jun 1, 2023

Migration to version 16.0 #489

Open

17 tasks

yostashiro force-pushed the 16.0-mig-stock_picking_analytic branch 2 times, most recently from 2b9e8e8 to 0c7d0f8 Compare June 14, 2023 15:45

desdelinux approved these changes Jun 20, 2023

View reviewed changes

rolandojduartem approved these changes Jun 20, 2023

View reviewed changes

OCA-git-bot added approved ready to merge labels Jun 20, 2023

luisg123v suggested changes Jun 21, 2023

View reviewed changes

OCA-git-bot removed approved ready to merge labels Jun 21, 2023

rousseldenis and others added 7 commits July 24, 2023 02:01

[14.0][ADD] stock_picking_analytic

6aba7e0

[14.0][IMP] stock_picking_analytic: Allows to extend context instead …

0d6ac05

…of erasing it

[14.0][IMP] stock_picking_analytic: Add original_analytic_account_id …

11f65ca

…in depends + tests

[IMP] stock_picking_analytic: Add move lines default in view

271b0be

[UPD] Update stock_picking_analytic.pot

ff55ef7

[UPD] README.rst

04e4d1e

[IMP] stock_picking_analytic: pre-commit stuff

b0c86a6

yostashiro force-pushed the 16.0-mig-stock_picking_analytic branch from 0c7d0f8 to bc92469 Compare July 24, 2023 03:27

desdelinux approved these changes Jul 25, 2023

View reviewed changes

luisg123v suggested changes Aug 1, 2023

View reviewed changes

yostashiro force-pushed the 16.0-mig-stock_picking_analytic branch from bc92469 to def630d Compare August 2, 2023 01:29

desdelinux approved these changes Aug 2, 2023

View reviewed changes

desdelinux approved these changes Aug 4, 2023

View reviewed changes

rolandojduartem approved these changes Aug 4, 2023

View reviewed changes

luisg123v approved these changes Aug 8, 2023

View reviewed changes

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

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

OCA-git-bot added the bot is merging ⏳ label Aug 8, 2023

OCA-git-bot merged commit be6f573 into OCA:16.0 Aug 8, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][MIG] stock_picking_analytic: Migration to 16.0 #527

[16.0][MIG] stock_picking_analytic: Migration to 16.0 #527

yostashiro commented Jan 21, 2023 •

edited

Loading

github-actions bot commented May 28, 2023

rafaelbn commented Jun 1, 2023

rousseldenis commented Jun 1, 2023

yostashiro commented Jun 1, 2023

desdelinux left a comment

desdelinux commented Jun 20, 2023

rolandojduartem commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

luisg123v left a comment •

edited

Loading

luisg123v Jun 21, 2023

luisg123v Jun 21, 2023

luisg123v Jun 21, 2023

luisg123v Jun 21, 2023

luisg123v commented Jun 21, 2023

desdelinux commented Jul 6, 2023

yostashiro commented Jul 6, 2023

rousseldenis commented Jul 19, 2023

yostashiro commented Jul 20, 2023

yostashiro commented Jul 24, 2023

desdelinux left a comment

rolandojduartem commented Jul 27, 2023

luisg123v Aug 1, 2023

luisg123v Aug 1, 2023

luisg123v Aug 1, 2023

yostashiro Aug 2, 2023

desdelinux left a comment

desdelinux commented Aug 4, 2023

luisg123v left a comment

luisg123v commented Aug 8, 2023

OCA-git-bot commented Aug 8, 2023

moylop260 commented Aug 8, 2023

OCA-git-bot commented Aug 8, 2023

	# https://github.com/odoo/odoo/blob/ae94f14a844352f66490ec326fe2d1a716023891/odoo/fields.py#L808-L812 # noqa
	# https://github.com/odoo/odoo/blob/ae94f14a/odoo/fields.py#L808-L812

	self.assertNotEqual(len(picking.move_ids), 0)
	self.assertTrue(picking.move_ids)

[16.0][MIG] stock_picking_analytic: Migration to 16.0 #527

[16.0][MIG] stock_picking_analytic: Migration to 16.0 #527

Conversation

yostashiro commented Jan 21, 2023 • edited Loading

github-actions bot commented May 28, 2023

rafaelbn commented Jun 1, 2023

rousseldenis commented Jun 1, 2023

yostashiro commented Jun 1, 2023

desdelinux left a comment

Choose a reason for hiding this comment

desdelinux commented Jun 20, 2023

rolandojduartem commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

luisg123v left a comment • 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

luisg123v commented Jun 21, 2023

desdelinux commented Jul 6, 2023

yostashiro commented Jul 6, 2023

rousseldenis commented Jul 19, 2023

yostashiro commented Jul 20, 2023

yostashiro commented Jul 24, 2023

desdelinux left a comment

Choose a reason for hiding this comment

rolandojduartem commented Jul 27, 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

desdelinux left a comment

Choose a reason for hiding this comment

desdelinux commented Aug 4, 2023

luisg123v left a comment

Choose a reason for hiding this comment

luisg123v commented Aug 8, 2023

OCA-git-bot commented Aug 8, 2023

moylop260 commented Aug 8, 2023

OCA-git-bot commented Aug 8, 2023

yostashiro commented Jan 21, 2023 •

edited

Loading

luisg123v left a comment •

edited

Loading