Merge lp://staging/~therp-nl/openupgrade-tools/7.0-add-database_cleanup into lp://staging/openupgrade-tools

Hi Stefan,

Thanks for this handy module!

I had an error when purging a model having attachments (poweremail.mailbox). Not sure if this issue lies in the ir.attachment code or the purge though.

In openerp/addons/base/ir/ir_attachment.py the method check() calls exists on the deleted model.

            mids = self.pool.get(model).exists(cr, uid, mids)
            ima.check(cr, uid, model, mode)
            self.pool.get(model).check_access_rule(cr, uid, mids, mode, context=context)

Maybe should the purge delete the attachments or drop the relationship beforehand?

Traceback (most recent call last):
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/netsvc.py", line 292, in dispatch_rpc
    result = ExportService.getService(service_name).dispatch(method, params)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/service/web_services.py", line 626, in dispatch
    res = fn(db, uid, *params)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/osv/osv.py", line 190, in execute_kw
    return self.execute(db, uid, obj, method, *args, **kw or {})
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/osv/osv.py", line 132, in wrapper
    return f(self, dbname, *args, **kwargs)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/osv/osv.py", line 199, in execute
    res = self.execute_cr(cr, uid, obj, method, *args, **kw)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/osv/osv.py", line 187, in execute_cr
    return getattr(object, method)(cr, uid, *args, **kw)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/openupgrade-tools/database_cleanup/model/purge_wizard.py", line 58, in purge_all
    context=context)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/openupgrade-tools/database_cleanup/model/purge_models.py", line 73, in purge
    context=context)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/addons/base_calendar/crm_meeting.py", line 160, in write
    return super(ir_attachment, self).write(cr, uid, ids, vals, context=context)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/addons/document/document.py", line 137, in write
    return super(document_file, self).write(cr, uid, ids, vals, context)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/addons/base/ir/ir_attachment.py", line 277, in write
    self.check(cr, uid, ids, 'write', context=context, values=vals)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/addons/document/document.py", line 77, in check
    super(document_file, self).check(cr, uid, ids, mode, context=context, values=values)
  File "/home/gbaconnier/code/instances/openerp_foobar/trunk7/parts/server/openerp/addons/base/ir/ir_attachment.py", line 211, in check
    mids = self.pool.get(model).exists(cr, uid, mids)
AttributeError: 'NoneType' object has no attribute 'exists'

>
> Maybe should the purge delete the attachments or drop the relationship
> beforehand?
>

This is already what is done through these lines:

                attachment_ids = attachment_pool.search(
                    cr, uid, [('res_model', '=', line.name)], context=context)
                if attachment_ids:
                    attachment_pool.write(
                        cr, uid, attachment_ids, {'res_model': False},
                        context=context)

I have to figure out what happens.

I got it. It happens on models still in ir.model but not in the registry because the python modules are not there.

I replaced the lines

                    attachment_pool.write(
                        cr, uid, attachment_ids, {'res_model': False},
                        context=context)
By
                    cr.execute(
                        "UPDATE ir_attachment SET res_model = FALSE "
                        "WHERE id IN %s",
                        (tuple(attachment_ids), ))

And it worked. Is it an acceptable solution?

When removing the models from ir.model, shouldn't we remove the ir.model.fields having a relation pointing to them?

> When removing the models from ir.model, shouldn't we remove the
> ir.model.fields having a relation pointing to them?

My proposal: https://code.launchpad.net/~camptocamp/openupgrade-tools/7.0-add-database_cleanup-purge-model-relations/+merge/204440

> I got it. It happens on models still in ir.model but not in the registry
> because the python modules are not there.
>
> I replaced the lines
>
> attachment_pool.write(
> cr, uid, attachment_ids, {'res_model': False},
> context=context)
> By
> cr.execute(
> "UPDATE ir_attachment SET res_model = FALSE "
> "WHERE id IN %s",
> (tuple(attachment_ids), ))
>
> And it worked. Is it an acceptable solution?

MP's here https://code.launchpad.net/~camptocamp/openupgrade-tools/7.0-add-database_cleanup-purge-model-attachment-error/+merge/204442

Thank you very much, Guewen! I merged both your contributions into this branch.

Oo I don't know what I did... But my rev5 is obviously wrong (replaced the wrong lines), here is the correct fix https://code.launchpad.net/~camptocamp/openupgrade-tools/7.0-add-database_cleanup-purge-model-attachment-error2/+merge/205125

Sorry!

> Oo I don't know what I did... But my rev5 is obviously wrong (replaced the
> wrong lines), here is the correct fix https://code.launchpad.net/~camptocamp
> /openupgrade-tools/7.0-add-database_cleanup-purge-model-attachment-
> error2/+merge/205125
>
> Sorry!

The one merged in 4, not 5. Decidely need holidays ;-)

Apparently, base.sql comes with its own inconsistencies. Fixed in http://bazaar.launchpad.net/~therp-nl/openupgrade-tools/7.0-add-database_cleanup/revision/7.

Discovered a workflow table without corresponding model that should not be purged.

After some extensive testing I fixed the case of 'sale_id' not being in stock.picking.in, thus to be purged from stock.picking. The fix allows for multiple models to share the same table in a generic way.

The module now allows to purge entries from ir.model.data that refer to a nonexisting row in its model's table.

Hi, Stefan, finally I have tried your module and it's fantastic. For the most of the sections, it works like intended, but in "Purge obsolete tables", I find some tables that cannot be removed due to foreign keys in other tables. Use DROP CASCADE is too risky to be implemented, but you can enclose removal in a try and warns user afterwards, but continuing with tables that possibly contain the foreign key it complains about.

Also a warn that it's convenient to remove obsolete fields before purging obsolete tables is a good improvements.

One more thing: if I try to click on line button to remove tables one by one, I get line form view, but button cannot be clicked. Maybe readonly attribute?

Thanks for this excellent module.

Regards.

P.S.: Don't you think this module better fits on server-env-tools?

Thanks for the review, Pedro! Glad you like it. I'll have a look at continuing after a drop table failed.

You can only click on the icons on individual line after you save the wizard form first. There is nothing that I can do about that, I think.

I could propose on server-env-tools, but as OpenERP 7.0 is pretty good in not leaving these remnants after module updates and uninstalls, this module is typically used after a home grown migration. We would need consensus on whether or not to adopt tools for home grown migration within OCA first. I expect such a discussion to pop up after the formalization of the association in the course of this year. Until there is a clear decision on this topic, I think these projects are better of separately.

Yeah, you are right about the inclusion on server-env-tools just right now.

About wizard saving before clicking on buttons, maybe you can present it on a popup or go to view mode to avoid this. Can it be done?

Regards.

Hi Stefan,

I just stumbled upon this module. This is most useful!

It seems to work fine on 8.0, except the individual purge buttons are disabled in the lists. Not sure why yet.

Is this the latest version, or did it find a home somewhere else?

Thanks!

@Stéphane: glad you found this useful. Doesn't saving the wizard form first work to access the list buttons in 8.0 like it does in 7.0?

I never got round to implementing Pedro's suggestion. I could leave it and propose to server-tools 7.0 as is. It can be improved afterwards. What do you think?

If you don't have to work on the improvement, the module itself is valuable, so I agree that you should propose to server-tools. Please put a section TODO on the module description with the considerations for being taking into account for other possible contributors.

@Stephan,

I indeed found that saving first solved the button issue, shame on me :)

The module is very useful as is, so I'm +1 to have it proposed to OCA/server-tools.

I have a short patch/hack to make it work on 8.0 in case anyone is interested.

@Stefan,

My colleague Anthony has made the pull request to OCA/server-tool.
https://github.com/OCA/server-tools/pull/95

Commit authorship has been preserved. I hope you take no issue with this.

Of course, feel free to clone the git branch and re-do the PR.

Best regards,

Hi Stéphane, thanks for picking this up. I see that I am already too late for the review, as it is already merged ;-)