Skip to content

[FIX] utm: avoid issue '"utm_stage" does not exist'#2372

Merged
OCA-git-bot merged 1 commit intoOCA:13.0from
ForgeFlow:13.0-fix-issue-utm-migration
Aug 12, 2020
Merged

[FIX] utm: avoid issue '"utm_stage" does not exist'#2372
OCA-git-bot merged 1 commit intoOCA:13.0from
ForgeFlow:13.0-fix-issue-utm-migration

Conversation

@MiquelRForgeFlow
Copy link
Contributor

Only affected databases without mass_mailing, because those cannot obtain "utm_stage" table from "mail_mass_mailing_stage".

Description of the issue/feature this PR addresses:

utm.stage model is created after utm.campaign model, but stage_id field of utm.campaign relates to utm.stage. When installing the module, it doesn't break. But for unkown reason, when migrating, it breaks.

Thus, as an easy solution, we define the utm.stage model before utm.campaign model.

Another solution could be create the utm_stage table manually in pre-migration.

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

@MiquelRForgeFlow MiquelRForgeFlow added this to the 13.0 milestone Aug 3, 2020
@pedrobaeza
Copy link
Member

As talked privately, I don't think this is the ideal solution, and other thing should be on the background. Anyway, your patch is declaring 2 times the same model with _name. That's incorrect.

Copy link

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Functionally speaking, it works :)

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-fix-issue-utm-migration branch from 3e3e72f to 01e37a2 Compare August 3, 2020 15:26
@legalsylvain
Copy link
Contributor

Hi. Thanks for the patch. As pedro said, I don't think it's a good idea to add code in modules. The second option you mention is more in the spirit of Openupgrade. (Create table in premigration script, if not exist) and not more complex.

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

I can live with this

@pedrobaeza
Copy link
Member

See my comment in #2392 (comment)

@StefanRijnhart
Copy link
Member

@pedrobaeza that's true, triggering the default will be avoided if the column is precreated. Probably the best solution.

@dennybiasiolli
Copy link

@pedrobaeza @StefanRijnhart another solution could be moving class UtmStage defininition on top of class UtmCampaign definition.
The tables would be created "top-down" and everything works fine.
Now I'm trying the "create column" solution, I'll let you know in a moment.

@dennybiasiolli
Copy link

@pedrobaeza @StefanRijnhart I think stage_id column it's already created at this line in addons/utm/migrations/13.0.1.0/pre-migration.py, so maybe the best solution it' the moving of class UtmStage on top of class UtmCampaign definition.

@MiquelRForgeFlow
Copy link
Contributor Author

@dennybiasiolli what you are saying is what this PR already is doing.

@pedrobaeza the column cannot be pre-created because the table doesn't exist.

@dennybiasiolli
Copy link

Something like this does not solve the problem:

diff --git a/addons/utm/migrations/13.0.1.0/pre-migration.py b/addons/utm/migrations/13.0.1.0/pre-migration.py
index d0bd85cf48a..a8eff443974 100644
--- a/addons/utm/migrations/13.0.1.0/pre-migration.py
+++ b/addons/utm/migrations/13.0.1.0/pre-migration.py
@@ -45,6 +45,12 @@ def move_mailing_campaign_to_utm_campaign(env):
     )


+def add_empty_stage_id_to_utm_campaign(env):
+    openupgrade.add_fields(env, [
+        ("stage_id", "utm.campaign", "utm_campaign", "integer", False, "utm"),
+    ])
+
+
 @openupgrade.migrate()
 def migrate(env, version):
     cr = env.cr
@@ -63,3 +69,5 @@ def migrate(env, version):
             ],
             False,
         )
+    else:
+        add_empty_stage_id_to_utm_campaign(env)

I've already made a PR to odoo repo (odoo/odoo#55795) in order to fix this at the source, but I can provide a PR here too, what do you think?

@dennybiasiolli
Copy link

@MiquelRForgeFlow I was thinking about something like this: #2393
Without adding new code, but only "moving" the class definitions

@pedrobaeza
Copy link
Member

@MiquelRForgeFlow if the problem is the table, pre-create the table as well. It's one SQL...

Only affected databases without mass_mailing.
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-fix-issue-utm-migration branch from 01e37a2 to 6968020 Compare August 12, 2020 10:49
@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Aug 12, 2020

@dennybiasiolli you are not taken into account that this repo is getting updated from upstream from time to time, and we like to do the minimum changes in odoo code to avoid conflicts.

@pedrobaeza ok, done. I just didn't want to create tables this way to avoid unexpected issues.

@pedrobaeza
Copy link
Member

Let me share with you why I'm so insistent with not modifying core source code: we are doing experiments with new features with migration addons path for avoiding to maintain full Odoo source code, and let OpenUpgrade only for pure migration scripts. There are still challenges like the changes in loading modules code, but it would be great to avoid to do upstream merges.

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-2372-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit bf0d65e into OCA:13.0 Aug 12, 2020
@MiquelRForgeFlow MiquelRForgeFlow deleted the 13.0-fix-issue-utm-migration branch August 12, 2020 11:22
@dennybiasiolli
Copy link

I'm trying right now the repo with the updated code, but it's not working. The newly created utm_stage table is missing sequence column.

psycopg2.errors.UndefinedColumn: column utm_stage.sequence does not exist
LINE 1: SELECT "utm_stage".id FROM "utm_stage" ORDER BY "utm_stage"....

After adding that column in pre-migration script everything works fine.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants