Merge lp:~hirt/ocb-addons/6.1_documents_fix_unlink_files into lp:ocb-addons/6.1

[Etienne Hirt]

Fix to avoid deleting files that still are referenced from documents. This fix is for 6.1 only, because in 7.0 the document module is redesigned completely.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Etienne, thank you very much for this patch. Indeed, is a needed change, but as you can see in the bug, the patch proposed by OpenERP is not the same as the want you have proposed, so if in the future OpenERP merge their patch into official branch, we will get into conflicts. Is there any problem with the OpenERP patch?

I have also target the bug to our branches to control the status. Please see how it's done for future references on other patches that you propose.

Regards.

review: Needs Information

[Etienne Hirt (hirt)]

On 13.11.2013 09:24, Pedro Manuel Baeza wrote:
> Review: Needs Information
>
> Hi, Etienne, thank you very much for this patch. Indeed, is a needed change, but as you can see in the bug, the patch proposed by OpenERP is not the same as the want you have proposed, so if in the future OpenERP merge their patch into official branch, we will get into conflicts. Is there any problem with the OpenERP patch?
>
> I have also target the bug to our branches to control the status. Please see how it's done for future references on other patches that you propose.
>
> Regards.
Dear Pedro,

I had implemented and tested the patch originally on V6 based on the
attached bugreport.

* I did not like having a for loop in row 308 instead of getting the
number of found elements
* and I'm not sure what happens when the unlink is called in row 319 for
all ids independant of canUnlink. Thus only the prepare_unlink is
considered in the original patch.

Furthermore, the patch was targeted for OpenERP Addons/V6.1 that no
longer exists. The trunk is version 7 or higher and the document.py is
completely rewritten.

I propose therefore the review and merge my proposal, that is tested
heavily in V6.0 which shares the code with 6.1.

Best Regards

Etienne

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Etienne, let me check then the opinion of others to see what we do.

Regards.

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for the patch, Etienne! I think you are correct in simply looking at the number of results instead of looping over the browse records just to get the same number. Also, the alternative patch seems to be broken with respect to the usage of canUnlink when the method is passed multiple ids. By changing the scope of canUnlink you seem to have solved this problem. You could have still kept to collecting redundant files in unres and deal with them after the loop but I don't find this blocking.

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

Thanks for the clarification, Stefan and Etienne.

I approve the MP then.

Regards.

review: Approve

[Stefan Rijnhart (Opener) (stefan-opener)]

On second thoughts, I am reminded that we should allow for the super method to raise an exception (e.g. check on permissions) before we physically remove the documents. So the proper way would be to collect files to remove in unres and only remove them outside the loop, after the ORM call unlink().

review: Needs Fixing

[Etienne Hirt (hirt)]

Dear Stefan,
Is this change what you where looking for?

Thanks
Etienne

[Stefan Rijnhart (Opener) (stefan-opener)]

Yes thanks! One more thing, it seems to me that you do not need to retrieve the attachment pool in line 18, but refer to 'self' in line 35 instead. What do you think?

review: Needs Information

[Etienne Hirt (hirt)]

Correct. Removed the retrieval of attachment pool and use self now.
Can you have a look at my other MP too?

Thanks

Etienne

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for the quick response, approved!

About the other MPs, they are amongst the queue of 174 MPs to be reviewed, so it could be some time I'm afraid. It would help of course if you could review a couple of the other MPs yourself.

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

I'm going to merge this MP, because its more than 5 days older and it has been extensively reviewed.

Thanks.

[Etienne Hirt (hirt)]

Dear Pedro, Dear Stefan,

Thanks a lot for your support and your work for the openerp community

Etienne

On 20.11.2013 02:40, Pedro Manuel Baeza wrote:
> I'm going to merge this MP, because its more than 5 days older and it has been extensively reviewed.
>
> Thanks.

=== modified file 'document/document.py'
--- document/document.py	2012-08-22 09:10:40 +0000
+++ document/document.py	2013-11-16 21:48:34 +0000
@@ -147,6 +147,9 @@
         # filename_uniq is not possible in pure SQL
     ]
     def _check_duplication(self, cr, uid, vals, ids=[], op='create'):
+        """
+        Returns True if not same filename is attached already to an object (res_model, res_id) 
+        """    
         name = vals.get('name', False)
         parent_id = vals.get('parent_id', False)
         res_model = vals.get('res_model', False)
@@ -340,14 +343,21 @@
                     storage_id = par.storage_id
                     break
                 par = par.parent_id
-            #assert storage_id, "Strange, found file #%s w/o storage!" % f.id #TOCHECK: after run yml, it's fail
-            if storage_id:
-                r = stor.prepare_unlink(cr, uid, storage_id, f)
-                if r:
-                    unres.append(r)
-            else:
-                logging.getLogger('document').warning("Unlinking attachment #%s %s that has no storage",
-                                                f.id, f.name)
+            #We get the ids of attachement that correspond to the document
+            attachment_ids = self.search(cr, uid, [('store_fname', '=', f.store_fname), ('parent_id.name', '=', f.parent_id.name)], context=context)
+            #If we have more than 1 attachment for a same file, we will not unlink it.
+            canUnlink = len(attachment_ids)
+            #If canUnlink is bigger than 1 it means that the document has more than 1 attachement.
+            #We therefore cannot unlink that document.
+            if canUnlink == 1:
+                #assert storage_id, "Strange, found file #%s w/o storage!" % f.id #TOCHECK: after run yml, it's fail            
+                if storage_id:
+                    r = stor.prepare_unlink(cr, uid, storage_id, f)
+                    if r:
+                        unres.append(r)
+                else:
+                    logging.getLogger('document').warning("Unlinking attachment #%s %s that has no storage",
+                                                    f.id, f.name)
         res = super(document_file, self).unlink(cr, uid, ids, context)
         stor.do_unlink(cr, uid, unres)
         return res