Skip to content

[IMP] set web_diagram as merged into web to avoid to uninstall it manually#3232

Merged
pedrobaeza merged 1 commit intoOCA:14.0from
legalsylvain:14.0-web_diagram-into-web
Jul 14, 2022
Merged

[IMP] set web_diagram as merged into web to avoid to uninstall it manually#3232
pedrobaeza merged 1 commit intoOCA:14.0from
legalsylvain:14.0-web_diagram-into-web

Conversation

@legalsylvain
Copy link
Copy Markdown
Contributor

rational

So before that patch, we should uninstall manually that module when migrating from 13 to 14.

question
However, I don't know if web_diagram has been dropped, or has moved into EE. If yes, is this PR OK ? I don't think that the objective of openupgrade project is to migrate EE instance, but maybe I'm wrong...

comments / alternative welcome.

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Hi @MiquelRForgeFlow, @StefanRijnhart, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

@pedrobaeza
Copy link
Copy Markdown
Member

I prefer to not do such merge, as the module doesn't have a replacement as is. Most of the installations don't make use of the diagram view, but imagine you have a module that is using it, and doing this merge will put the module automatically as not absent, and its dependencies redirected to web. And maybe someone in OCA wants to migrate web_diagram module to upper versions, and with this, you are preventing it from a smooth migration path.

@MiquelRForgeFlow
Copy link
Copy Markdown
Contributor

FYI odoo/odoo@3620656

They completely removed it, they didn't do a merge.

@legalsylvain
Copy link
Copy Markdown
Contributor Author

I understand your point of view. Does it mean that for each migration from 13 to 14, each people should manually uninstall this module ?
Do you see any alternative ?
Idea : in the end-migration.py, if the module is not available in the addons_path, uninstall it.

Note : the commit mentionned by @MiquelRForgeFlow says " [REM] web_diagram: remove unused module". So no chance that the module will be ported in 14 ?

@pedrobaeza
Copy link
Copy Markdown
Member

You can check if there's any remaining ir.ui.view with such type, and only merge it if there isn't any.

@legalsylvain
Copy link
Copy Markdown
Contributor Author

You can check if there's any remaining ir.ui.view with such type, and only merge it if there isn't any.

👍 Thanks !

@legalsylvain
Copy link
Copy Markdown
Contributor Author

Hi @pedrobaeza : not sure to see exactly what you mean.
I propose to add before that line https://github.com/OCA/OpenUpgrade/blob/14.0/openupgrade_scripts/scripts/base/14.0.1.3/pre-migrate.py#L127
a request to know if there are web diagram views. if there are any, pop the key web_diagram in the merged_modules dict. Otherwise, let it.

Is this what you have in mind?

@pedrobaeza
Copy link
Copy Markdown
Member

Not exactly: in base end-migration, search for any ir.ui.view with type='diagram'. If not found, then manually call there:

openupgrade.update_module_names(cr, [("web_diagram", "web")], merge_modules=True)

@legalsylvain legalsylvain force-pushed the 14.0-web_diagram-into-web branch from 0d0aa08 to fb277c4 Compare July 11, 2022 15:32
@legalsylvain
Copy link
Copy Markdown
Contributor Author

Thanks @pedrobaeza, I made the changes and it fixed my problem. (no need to uninstall web_diagram manually).

@pedrobaeza pedrobaeza added this to the 14.0 milestone Jul 11, 2022
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.

Can you please avoid empty extra lines inside a method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you please avoid empty extra lines inside a method?

Done. Is it an OCA rules ? If yes, could be great to have a pre-commit for that point.

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.

It's stated as advisable in PEP8, and my thumb rule is to not introduce such empty lines where the previous code didn't have them.

…ually, if no views (type='diagram') are available in the database
@legalsylvain legalsylvain force-pushed the 14.0-web_diagram-into-web branch from fb277c4 to 9e58990 Compare July 11, 2022 21:26
@pedrobaeza pedrobaeza merged commit 37a6f02 into OCA:14.0 Jul 14, 2022
@zurawsm3
Copy link
Copy Markdown

What does it IMP tag mean?
important?

@pedrobaeza
Copy link
Copy Markdown
Member

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message

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.

5 participants