[10.0][Stock] Added migration scripts for stock module by pepetreshere · Pull Request #837 · OCA/OpenUpgrade

pepetreshere · 2017-05-19T01:31:33Z

Description of the issue/feature this PR addresses: Added migration scripts to v10 for stock module

Current behavior before PR: Module stock fail migration

Desired behavior after PR is merged: Module stock migrated successfully

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

lfreeke · 2017-05-19T08:58:10Z

@jbeficent can you have a look at this?

pedrobaeza

Good work being your first submission to OpenUpgrade. Please follow my remarks.

pedrobaeza · 2017-05-23T17:09:39Z

-# TODO: pre-migration. Default to some value, guessing by looking at source
+# TODO: post-migration. Default to some value, guessing by looking at source
 # and destination locations, and warehouse.
+# Done!


Remove the TODO and put Done: explanation of what you have done

pedrobaeza · 2017-05-23T17:18:16Z

@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+# © 2017 Trescloud <http://trescloud.com>


Use copyright full word

pedrobaeza · 2017-05-23T17:20:06Z

@@ -0,0 +1,104 @@
+# -*- coding: utf-8 -*-
+# © 2017 Trescloud <http://trescloud.com>


Use copyright full word

pedrobaeza · 2017-05-23T17:20:21Z

+from openupgradelib import openupgrade
+
+@openupgrade.migrate(use_env=True)
+def migrate(env, version):


Put this at the end of the file by general convention

pedrobaeza · 2017-05-23T17:21:08Z

+    populate_stock_scrap(env)
+
+def update_picking_type_id(env):
+    '''


According PEP257, method documentation goes with triple double quota, not simple, and first line start in the same one as the initial quotas.

pedrobaeza · 2017-05-23T17:25:22Z

+    Set as default the value of field ordered_qty from the value of field product_uom_qty  
+    :param env: enviroment variable (self)
+    '''
+    #stock        / stock.move               / ordered_qty (float)           : NEW


Don't be too verbose. This line can be removed.

pedrobaeza · 2017-05-23T17:25:44Z

+    #stock        / stock.move               / ordered_qty (float)           : NEW
+    # TODO: post-migration. Default to product_uom_qty
+    env.cr.execute(
+        """


Use one quote when the query is very simple.

pedrobaeza · 2017-05-23T17:26:04Z

+        """)
+
+def populate_stock_scrap(env):
+    '''


You have forgotten to put Done on the work analysis for this.

pedrobaeza · 2017-05-23T17:26:27Z

+
+    #do not call stock_scrap.create as it will create a duplicated stock move, use SQL instead
+    #field package_id not set as no value is defined    
+    env.cr.execute(


Again, using ORM can be simple and the same in speed (more or less).

pedrobaeza · 2017-05-23T17:27:41Z

+
+    #field route_sequence
+    openupgrade.copy_columns(
+        env.cr, {


Use variables to store the list of column renames, copies, and so on, the same as in https://github.com/OCA/OpenUpgrade/pull/839/files#diff-3cdc4348ce357d41a6908e42cadbca5aR8

Group also all the copies in one.

pepetreshere · 2017-05-23T17:46:33Z

Thanks for your remarks @pedrobaeza!, we will work on this by next monday

MiquelRForgeFlow

I just sent you a PR (TRESCLOUD#2) that includes all adjustments I requested here

MiquelRForgeFlow · 2017-05-26T11:03:39Z

 stock        / stock.scrap              / state (selection)             : NEW selection_keys: ['done', 'draft']
-# TODO: Create one record for each move that had as destionation a scrap
-# location
+# Done: pre-migration: Create one record for each move that had as destionation a scrap location


is post-migration

MiquelRForgeFlow · 2017-05-26T11:04:41Z

 stock        / stock.location.path      / auto (selection)              : selection_keys is now '['manual', 'transparent']' ('['auto', 'manual', 'transparent']')
-# TODO: pre-migration. copy column
-# TODO: post-migration. Move 'auto' to some other value
+# Done: pre-migration. copy column and then replace value "auto" with value "manual"


the replacement should be done in post-migration

MiquelRForgeFlow · 2017-05-26T11:06:44Z

+    xml_stock_picking_type_internal = env.ref("stock.picking_type_internal")
+    xml_stock_picking_type_out = env.ref("stock.picking_type_out")
+    xml_stock_picking_type_in = env.ref("stock.picking_type_in")
+    xml_stock_picking_type_manufacturing = env.ref("mrp.picking_type_manufacturing")


this should be removed. the 'manufacture' case should be into the migration of mrp module

MiquelRForgeFlow · 2017-05-26T11:07:39Z

+        if not procurement_rule.picking_type_id:
+            picking_type_id = False
+            env.cr.execute(
+                '''


''' → """

MiquelRForgeFlow · 2017-05-26T11:09:22Z

+                    procurement_rule.location_src_id,
+                )
+            )
+            picking_type_ids = cr.fetchone()


→ env.cr.fetchone()

MiquelRForgeFlow · 2017-05-26T11:15:16Z

+    """
+    env.cr.execute(
+        '''
+        UPDATE stock_move SET ordered_qty = product_uom_qty


the query should be compacted in one line

MiquelRForgeFlow · 2017-05-26T11:20:53Z

+        )
+    scrap_location_ids = env.cr.fetchone()
+    #do not call stock_scrap.create as it will create a duplicated stock move, use SQL instead
+    #field package_id not set as no value is defined    


field package_id can be set, with a witty query, if you relate the move with the stock_quant

MiquelRForgeFlow · 2017-05-26T11:23:40Z

+    openupgrade.copy_columns(env.cr, column_copies)
+    openupgrade.float_to_integer(env.cr, 'stock_location_path', 'route_sequence')
+    openupgrade.float_to_integer(env.cr, 'procurement_rule', 'route_sequence')
+    env.cr.execute(


this replacement should be done in the post-migration

MiquelRForgeFlow · 2017-05-26T11:28:36Z

+                    if xml_stock_picking_type_manufacturing:
+                        picking_type_id = xml_stock_picking_type_manufacturing.id
+            procurement_rule.write({'picking_type_id': picking_type_id})
+


between the defs should be two empty lines

MiquelRForgeFlow · 2017-05-26T11:31:02Z

+@openupgrade.migrate(use_env=True)
+def migrate(env, version):
+    update_picking_type_id(env)
+    update_ordered_qty(env)


pass the cr instead of the env if the def only use the cr

pepetreshere · 2017-05-26T17:49:53Z

Thanks @pedrobaeza and @mreficent for your remarks, I am still doing some tests today and I will likely push a few more commits.

pepetreshere · 2017-05-26T19:25:56Z

@mreficent I have included some minor changes for the migration of stock module, and I think we can merge it now.

I have included again the case for mrp.picking_type_manufacturing with a None option if it is missing (line 24 of post-migration.py), I believe it might be much easier to migrate that field in stock module rather than in mrp module (this change is not even detected in the analysis file of mrp). If you insist I can remove it again, what do you think?. Thanks in advance for your advise.

MiquelRForgeFlow · 2017-05-29T09:28:56Z

put a space between the comma and the False, and also remember to not overpass 80 chars limit by line in all the code

MiquelRForgeFlow · 2017-05-29T09:43:32Z

flake8 wants the whitespaces at the end of the line to be removed. there are 9 lines affected.

pepetreshere · 2017-05-31T19:56:24Z

@pedrobaeza thanks for your fixes, this error appears again in travis:
2017-05-31 14:43:01,815 40924 ERROR openupgrade OpenUpgrade: External ID not found in the system: mrp.picking_type_manufacturing

I think that to avoid this missing ID we should keep the the False option in the method env.ref method, or alternatively move this part to the migration script of mrp (which is not advisable from my point of view as detailed above).

So I have included a solution in this PR, please comment if you agree with the resolution as I don't want to overwrite one line of your last commit without your agreement.

pedrobaeza · 2017-05-31T21:26:09Z

Oh, I was working on that and other missing things right now. Well, I remove that part and continue.

pepetreshere · 2017-06-01T23:09:41Z

Hello @pedrobaeza, just to make sure I understood correctly. I understood that you will make the fix altogether with other fixes, so I have not merged my fix into this branch. If you like me to merge my fix please let me know.

pedrobaeza · 2017-06-01T23:14:55Z

Yeah, I'm making them, but today and tomorrow there is the Odoo days in Spain (https://odoospain.odoo.com/event/jornadas-odoo-2017-2017-06-01-2017-06-02-5), so I'm quite busy. I will try to work tomorrow or on Monday.

pedrobaeza · 2017-06-05T10:11:35Z

@StefanRijnhart the same behavior is happening again with Travis (infinite loop), so the reason should be another one. I'll check it later.

StefanRijnhart · 2017-06-05T14:13:04Z

@pedrobaeza I see, thanks for the heads up. Why is the generic coa set to uninstallable?

pedrobaeza · 2017-06-05T14:17:58Z

That was another try before throwing this error because it tries to create some invoices without account_id. I see that this is not the ideal solution anyway. I'll make other tries later.

hbrunn · 2017-06-06T16:17:16Z

@pepetreshere please rebase your branch on current 10.0, then we should get a better picture from travis

hbrunn · 2017-06-07T09:56:04Z

@StefanRijnhart I feel uncomfortable patching upstream code where it's not absolutely necessary, so I'd prefer not to have 9314ff87188da31d1263b6c3bf7bb4f3d2485ac0 in OpenUpgrade. You know from painful experience in #895 that every change we apply is a possible source of conflicts.

My proposal is: Revert 9314ff87188da31d1263b6c3bf7bb4f3d2485ac0, merge #897, then this should be green too, and we're happy

StefanRijnhart · 2017-06-07T09:58:59Z

@hbrunn I can agree with that, yes.

StefanRijnhart · 2017-06-07T09:59:23Z

Wait, you want to keep setting modules to uninstallable?

pedrobaeza · 2017-06-07T10:17:48Z

@hbrunn I don't think the change in base_action_rule will conflict in the future, but anyway, have you checked if there's any other non migrated module that declares action rules?

hbrunn · 2017-06-07T10:25:01Z

@StefanRijnhart setting modules to uninstallable: Now that I looked at the code for the first time, no, I don't want that either
@pedrobaeza I think https://github.com/odoo/odoo/search?utf8=%E2%9C%93&q=model%3D%22base.action.rule%22+language%3Axml&type=Code makes reasonably sure that's the last module causing trouble

- Added reate_date to stock_scrap, - Removed positive qtys filter as positive and negative are supported - Removed scrapped source location as domains avoids this in v10 - Added explanation about using SQL instead of ORM - Included case for MRP for ease of coding

* PEP8 * Fixes * Some things not attended

pedrobaeza · 2017-06-07T14:43:36Z

We still have problems with website module that don't anticipate before. Should I fix it here or do you prefer to merge this and create another PR fixing website?

StefanRijnhart · 2017-06-07T14:44:48Z

We can merge, if you remove the change of setting the coa modules uninstallable.

pedrobaeza · 2017-06-07T15:20:23Z

Removing that commit, I won't still be able to get a green branch although I fix website part. Can't we let the commit and revert later when dealing with account migration?

pedrobaeza · 2017-06-07T15:31:30Z

OK, finally I'm going to translate my greenifying efforts to the account migration PR, so I have removed the commit that makes uninstallable the l10n_* modules and merging this one although in red. Then, I'll wait for website PR and rebase everything to work on the final solution at account level.

JordiBForgeFlow · 2017-06-07T15:33:21Z

👍

pepetreshere · 2017-06-08T20:40:13Z

👍 :)

pepetreshere mentioned this pull request May 19, 2017

Migration scripts for v10 #757

Closed

pedrobaeza requested changes May 23, 2017

View reviewed changes

MiquelRForgeFlow requested changes May 26, 2017

View reviewed changes

MiquelRForgeFlow reviewed May 29, 2017

View reviewed changes

MiquelRForgeFlow requested changes May 29, 2017

View reviewed changes

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from 1a93cc0 to d3deaa4 Compare May 31, 2017 08:03

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch 4 times, most recently from dd50e48 to 3ad8291 Compare June 5, 2017 08:42

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch 4 times, most recently from a3ef31c to def2b6d Compare June 6, 2017 15:01

pedrobaeza mentioned this pull request Jun 6, 2017

[10.0][MIG] board: Nothing to do #867

Merged

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from def2b6d to 71e67e0 Compare June 6, 2017 15:15

pedrobaeza closed this Jun 6, 2017

pedrobaeza reopened this Jun 6, 2017

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from 9314ff8 to 02fa3f1 Compare June 7, 2017 11:52

pedrobaeza closed this Jun 7, 2017

pedrobaeza reopened this Jun 7, 2017

pedrobaeza mentioned this pull request Jun 7, 2017

[ADD] migrate website #899

Merged

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from 02fa3f1 to f49ebb2 Compare June 7, 2017 13:17

pedrobaeza closed this Jun 7, 2017

pedrobaeza reopened this Jun 7, 2017

pepetreshere and others added 4 commits June 7, 2017 15:56

[ADD] stock: Added migration scripts for stock module

8ed9285

[IMP] stock: Some adjustments as per review comments

a0a9121

[FIX+IMP] stock:

bca97c5

* PEP8 * Fixes * Some things not attended

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from f49ebb2 to 4d9fd56 Compare June 7, 2017 13:56

pedrobaeza closed this Jun 7, 2017

pedrobaeza reopened this Jun 7, 2017

pedrobaeza closed this Jun 7, 2017

pedrobaeza reopened this Jun 7, 2017

[FIX] base: Merge modules instead of removing them

0ca64f2

pedrobaeza force-pushed the 2017-04-AC-MigrationOfStockModule branch from 4d9fd56 to 0ca64f2 Compare June 7, 2017 15:29

pedrobaeza merged commit 031b2bd into OCA:10.0 Jun 7, 2017

		@@ -0,0 +1,32 @@
		# -- coding: utf-8 --
		# © 2017 Trescloud <http://trescloud.com>

		@@ -0,0 +1,104 @@
		# -- coding: utf-8 --
		# © 2017 Trescloud <http://trescloud.com>

Uh oh!

Uh oh!

Conversation

pepetreshere commented May 19, 2017

Uh oh!

lfreeke commented May 19, 2017

Uh oh!

pedrobaeza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepetreshere commented May 23, 2017

Uh oh!

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepetreshere commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepetreshere commented May 26, 2017

Uh oh!

MiquelRForgeFlow May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepetreshere commented May 31, 2017

Uh oh!

pedrobaeza commented May 31, 2017

Uh oh!

pepetreshere commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrobaeza commented Jun 1, 2017

Uh oh!

pedrobaeza commented Jun 5, 2017

Uh oh!

StefanRijnhart commented Jun 5, 2017

Uh oh!

pedrobaeza commented Jun 5, 2017

pepetreshere commented May 26, 2017 •

edited

Loading

MiquelRForgeFlow May 29, 2017 •

edited

Loading

pepetreshere commented Jun 1, 2017 •

edited

Loading