OCA / OpenUpgrade

Open source upgrade path for Odoo/OpenERP
https://oca.github.io/OpenUpgrade/
GNU Affero General Public License v3.0
742 stars 696 forks source link

[9.0][account] Analytic line views not removed during migration #577

Closed JordiBForgeFlow closed 3 years ago

JordiBForgeFlow commented 8 years ago

Impacted versions: 9.0

Steps to reproduce: Migrate a demo database in v8 with modules 'analytic' and 'account' installed.

Current behavior: The form, search and tree views for model 'account.analytic.line.tree' are not deleted.

Depending on how Odoo decides to render the views, this may result in an error such as: _File "/home/jordi/Odoo9/openerp/addons/base/ir/ir_ui_view.py", line 463, in raise_view_error raise AttributeError(message) AttributeError: Field journal_id does not exist Error context: View account.analytic.line.form [view_id: 566, xml_id: n/a, model: account.analytic.line, parentid: n/a]

Expected behavior: The following views should be deleted during the migration process, or during the database cleanup: account.view_account_analytic_line_tree account.view_account_analytic_line_form account.view_account_analytic_line_filter

Video/Screenshot link (optional): image

image

hbrunn commented 8 years ago

have a look at the log of a successful migration: https://travis-ci.org/OCA/OpenUpgrade/builds/147954920#L4416

At the end, odoo will delete all records pointed to by obsolete xmlids from migrated models. Check out how this looks in your migration, probably you'll find a line where odoo tries to do this, but there are other views inheriting from it or some other reason leads to the view not being deleted.

JordiBForgeFlow commented 8 years ago

Looking at Travis I can see: 2016-07-28 07:14:48,586 24245 INFO openupgrade openerp.addons.base.ir.ir_model: Deleting 824@ir.ui.view 2016-07-28 07:14:48,589 24245 INFO openupgrade openerp.sql_db: bad query: delete from ir_ui_view where id IN (824) 2016-07-28 07:14:48,590 24245 WARNING openupgrade openerp.addons.base.ir.ir_model: Could not delete obsolete record with id: 824 of model ir.ui.view

Them the following error message at the end of the log: Error context: View account.analytic.line.form [view_id: 824, xml_id: n/a, model: account.analytic.line, parent_id: n/a] 2016-07-28 07:14:58,348 24245 INFO openupgrade openerp.addons.base.ir.ir_ui_view: Field journal_id does not exist

This seems to indicate that openupgrade tried to, but could not delete the view, and is causing trouble later on.

hbrunn commented 8 years ago

so deleting the views is inhibited, which is a good think. Look at this:

odoo8_demo=> select id from ir_model_data where name in ('view_account_analytic_line_form');
  id  
------
 824
(1 row)

odoo8_demo=> delete from ir_ui_view where id=824;
ERROR:  update or delete on table "ir_ui_view" violates foreign key constraint "ir_ui_view_inherit_id_fkey" on table "ir_ui_view"
DETAIL:  Key (id)=(824) is still referenced from table "ir_ui_view".
odoo8_demo=> select id from ir_ui_view where inherit_id=824;
  id  
------
 1574
 2180
(2 rows)

odoo8_demo=> select module || '.' || name from ir_model_data where model='ir.ui.view' and res_id in (select id from ir_ui_view where inherit_id=824);
                               ?column?                               
----------------------------------------------------------------------
 hr_timesheet_invoice.view_account_analytic_line_form_inherit
 project_timesheet.view_account_analytic_line_form_inherit_account_id

So it's a matter of patience until those modules are migrated too, then the inheriting views will be gone too, and we should be fine.

JordiBForgeFlow commented 8 years ago

hmm, thanks. I will then speed up the migration of those modules. Because otherwise it gives me trouble.

hbrunn commented 8 years ago

database cleanup should be your friend here too

JordiBForgeFlow commented 8 years ago

project_timesheet is already migrated, and hr_timesheet_invoice does not migrate at all, so the problem should be solved by now.

When you purge obsolete views with database_cleanup, the views associated to 'hr_timesheet_invoice' are deleted, but not the views from 'project_timesheet'.

During the migration the views are losing the external ID. See below. When I exported the views, they generated a new external ID. image

This would explain why updating the modules and performing a database cleanup is not removing these obsolete views.

hbrunn commented 8 years ago

hmmm, yes. The obsolete module is the problem, didn't pay attention to that. And indeed the xmlids will be purged, after that we don't really have a chance to detect views to be deleted.

@OCA/openupgrade-maintainers how should we handle this kind of situation? I'd be in favor of adding obsolete modules in openupgrade with the old dependencies, but further empty manifest and init. Then odoo's xmlid deletion mechanism should do the rest. @jbeficent could you try if that works for you?

Another approach would be to patch the server not to delete the xmlid when it is impossible to delete the record pointed to (is this not actually a bug in odoo?). Then database_cleanup can do its work. This would involve removing the id in question when we arrive in https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L1248 from ids, so that it is not unlinked in https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L1278 - this might actually be the better approach.

pedrobaeza commented 8 years ago

I prefer to add the removed addons from version to version as dummy modules (without content), or add content if needed for migration purposes. In the documentation, they should also be included in the module coverage, because when you check if your DB can be migrated from v8 to v9, you are seeing if v8 modules are covered in this table. If you don't see the entry in the table, then you don't know what to do with them.

hbrunn commented 8 years ago

@pedrobaeza my difficulty with this is that the side effect of the dummy module is that all the module's columns/tables will be dropped, which contradicts our stance of keeping as much data intact as possible. And moving all columns/tables away in such a case also seems a bit untenable. What's you opinion about the second option? The more I think about this, the more I think this is a bug that should be fixed in upstream anyways, independently of openupgrade. For example when you uninstall a module that adds some record on a table with a uniqueness constraint. For whatever reason this record cannot be deleted, but the rest goes fine, so the action succeeds. Then when reinstalling the module, you'll get an exception because odoo will try to recreate the record that couldn't be deleted, uniqueness constraint, boom. Maybe we can find some upstream module that adds categories, where it should be simple to create such a constellation. With this, we can go to odoo/odoo and have it merged there, and remerge here (yes, I understand how optimistic/naive this is)

pedrobaeza commented 8 years ago

OK, I see the problem. Yes, you're right, we can't do that.

Let's try then the second path.

Anyway, I think the covered modules issue should be treated as mentioned. Putting for example "Merged and treated in module X", or "Nothing to do", or "Migrated module in OCA/repository", what do you think about this part?

hbrunn commented 8 years ago

that obsolete modules should show up on https://doc.therp.nl/openupgrade/modules80-90.html, possible with a sentence on how to replace the functionality? sure, good one

hbrunn commented 8 years ago

If you want to upvote my PR: https://github.com/odoo/odoo/pull/12954 @jbeficent can you try if this fixes your problem? You'll have to adapt this a bit because the function is still v7 api in 9. So for this, I think you'll be better off creating a dict with the tuples as keys and the id as value in https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L1203, then use this to determine the id in question in https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L1248, add that to some list and subtract this list from ids in https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L1278

hbrunn commented 8 years ago

@pedrobaeza I'm not so sure if you did this PR a favor by mentioning the ugly O-word...

pedrobaeza commented 8 years ago

Ouch, I didn't remember that we are in a world not so open... but they said that the migration scripts are not now a core part of their business. Sorry anyway for not being aware of that.

JordiBForgeFlow commented 8 years ago

Silly question, but as a workaround, could we just delete the xml data records including all views where it is used in the pre-migration script? The proposed approach seems to me a bit like killing mosquitoes with cannons ;)

lasley commented 8 years ago

I've had more luck in refilling the XML IDs (although not in the context of a migration) because the records are typically remaining due to relational restrictions.

hbrunn commented 8 years ago

@jbeficent if you just want to get the stuff done, delete the views and the inherits before the migration. But this doesn't scale (we don't want to delete this kind of stuff manually in migration scripts), and in order to have this fixed once and for all for everything, we need the bazooka

StefanRijnhart commented 8 years ago

So, would it help if we merge @hbrunn's Odoo PR in OpenUpgrade now?

pedrobaeza commented 7 years ago

Is this still relevant? I haven't got any problem with project_timesheet module installed.

pedrobaeza commented 7 years ago

I think we have finally covered this with:

So I close the issue. Feel free to reopen if you think the contrary.

StefanRijnhart commented 7 years ago

The problem still exists with the following views

pedrobaeza commented 7 years ago

What is the problem you have, Stefan?

StefanRijnhart commented 7 years ago

Basically the same problem that Jordi has: the Odoo interface breaks on remaining views for analytic lines that contain the obsolete journal_id field. Views are blocked from removal by inheritance from views from hr_timesheet_invoice. After removing this module, the views need to be removed manually because the XML ids have been deleted. So the first step would be to get https://github.com/OCA/OpenUpgrade/pull/594 merged, and maybe merge hr_timesheet_invoice with another module like you suggested. Which module did you merge it with?

pedrobaeza commented 6 years ago

@StefanRijnhart this is already achieved with your latest PR about disabling invalid views, isn't it?

StefanRijnhart commented 6 years ago

I am not sure. My change concerned custom views, which are views without an XMLID. Ironically, these views would have lost their XMLID before @hbrunn's fix. Not saying we should revert that, though.

pedrobaeza commented 6 years ago

Hehe, I see. I have no clue then how to solve it.

pedrobaeza commented 4 years ago

I'm not finding this problem anymore in any of the migrations. Is this still relevant?

StefanRijnhart commented 4 years ago

On my current migration project these views are gone after running the 9.0 migration. Closing.

sebastienhasa commented 4 years ago

Basically the same problem that Jordi has: the Odoo interface breaks on remaining views for analytic lines that contain the obsolete journal_id field. Views are blocked from removal by inheritance from views from hr_timesheet_invoice. After removing this module, the views need to be removed manually because the XML ids have been deleted. So the first step would be to get #594 merged, and maybe merge hr_timesheet_invoice with another module like you suggested. Which module did you merge it with?

I got the same problem, with latest version of Openupgrade (get from the branch 8.0 of this repo this morning)

StefanRijnhart commented 4 years ago

@sebastienhasa can you do an -u all on your resulting 9.0 database (can be on a regular Odoo instance, not necessarily openupgrade) and see if that fixes the problem?

pedrobaeza commented 3 years ago

Closing as no further notice.