Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Master add views migration 17 #83

Closed

Conversation

jjscarafia
Copy link
Collaborator

No description provided.

@Bruno-Zanotti Bruno-Zanotti force-pushed the master-add-views_migration_17 branch 2 times, most recently from e2eb1f8 to 4554e0b Compare November 21, 2023 13:12
@jjscarafia jjscarafia marked this pull request as ready for review November 22, 2023 01:19
@jjscarafia
Copy link
Collaborator Author

@pedrobaeza what do you think about this?

Making this script a commont script inside the tool was to heavy. We've created a new folder "addons" and a new module to help on views migration on 17.

We've been using it and is working like a charm.

Simply:

  1. run odoo-module-migrate as always
  2. install modules changing server wide modules so that the patch is loaded and views are fixed. for eg: odoo -i [module_name] -d [database_name] --load=base,web,views_migration_17

Copy link
Collaborator

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi @jjscarafia. Thanks for your contribution.

Question :

Making this script a commont script inside the tool was to heavy

This PR adds > 1600 lines of codes that is very huge. How much was common script more heavy ? (More or less)

Blocking point : This project is a pithon library to migrate odoo modules.
It should not host odoo modules, something is wrong in the design.
No test, no possible CI to test, etc...

If the feature is interesting, please :

  • create a dedicated OCA repo to host such modules.
  • just mention the module in the current project.

@pedrobaeza
Copy link
Member

Yes, I also see weird to require a module to do this.

@jjscarafia
Copy link
Collaborator Author

I also see it weird. But to be honest we've been using this to migate modules and it's awesome. It fix all the views.
For me, no problems not to merge it. I don´t see any nicier not so difficult solution.

If you prefer, we can close the pr.

We create it because we believe it could help a lot to migrate modules.

The approach is not nice but it works.

A dedicated hosted repository looks good to me, but I feel that it could be too much. For now it's just for this module, this use case of migration between 16 and 17.

Thanks you both!

@pedrobaeza
Copy link
Member

But why the same algorithm can't be done with regular Python code as the rest of the module migrator does?

@jjscarafia
Copy link
Collaborator Author

Hi @pedrobaeza
We've started doing so but it was taking more time that the one available on our side.
There are some challengues as:

@pedrobaeza
Copy link
Member

Yeah, I see. I understand if the work is very huge and you don't want to continue. The problem is that this library wants to be independent from Odoo and light, so doing that way will compromise the foundation principles. A new repo or server-tools module can be added though.

@jjscarafia
Copy link
Collaborator Author

thanks @pedrobaeza
So proposing this module on server-tools for v17 seems a good one?
If I'm not wrong, the mdoule can be uninstallble and still be loaded for the patch. So with a good readme it will not break anything.

@pedrobaeza
Copy link
Member

Yeah, I think so.

@Bruno-Zanotti
Copy link
Contributor

Hey @pedrobaeza @jjscarafia, here it is the new PR OCA/server-tools#2775, it perfectly works with the module uninstallable=False.. I think we can go ahead, merge it and close this one.

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.

None yet

4 participants