Skip to content

Add migration for analytic - #802

Merged
pedrobaeza merged 3 commits into
OCA:10.0from
gfcapalbo:10.0-analytic-migration
May 11, 2017
Merged

Add migration for analytic#802
pedrobaeza merged 3 commits into
OCA:10.0from
gfcapalbo:10.0-analytic-migration

Conversation

@gfcapalbo

Copy link
Copy Markdown

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

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

NEW ir.ui.view: analytic.account_analytic_tag_form_view
NEW ir.ui.view: analytic.account_analytic_tag_tree_view
DEL ir.ui.view: analytic.account_analytic_chart_view

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this extra line?

# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).
from openupgradelib import openupgrade

@openupgrade.migrate(use_env=False)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change the default use_env=True, and make your calls with env.cr instead of cr, because if the environment is later needed, then we need to change all calls, as it happened in https://github.com/OCA/OpenUpgrade/pull/803/files#diff-cd4cccdd111034d7430f83844f9d6372

# copy columns good practice in pre-mig script format of colum_spec should
# be: { Table_name_in_db : [( old_column_name , new_column_name, type)]
# defaults of new columname are fetchable via method get_legacy_name
openupgrade.copy_columns( cr, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the leading space


@openupgrade.migrate(use_env=False)
def migrate(cr, version):
# copy columns good practice in pre-mig script format of colum_spec should

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to just rename the column to None. You don't need to copy values.

@@ -0,0 +1,19 @@
-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the test that checks that these accounts are inactive.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test code added to repo
@gfcapalbo
gfcapalbo force-pushed the 10.0-analytic-migration branch from ba927fe to c4d10d9 Compare May 10, 2017 11:45

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is due to crm module

@pedrobaeza
pedrobaeza merged commit ab06f76 into OCA:10.0 May 11, 2017
from openerp.tests.common import TransactionCase


class TestAnalytic(TransactionCase)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just discovered that this is not being executed, or this would be marked as error, as it misses the final :

cc @hbrunn @gfcapalbo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fixing the issues directly for getting a green build: https://travis-ci.org/OCA/OpenUpgrade/builds/232892182#L357

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially fixed on 993e0ec. Switching to PR for trying to fix the other error: https://travis-ci.org/OCA/OpenUpgrade/builds/232906089#L428

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants