From f915b8a1beb876340f5035952c38dc5374c37415 Mon Sep 17 00:00:00 2001
From: Guewen Baconnier <guewen.baconnier@camptocamp.com>
Date: Wed, 13 Jun 2018 17:15:18 +0200
Subject: [PATCH] Ensure that migration of files is commited before deleting
 files

When moving attachments from the filestore to an object storage, the
filesystem files will be deleted only after the commit, so if the
transaction is rollbacked, we still have the local files for another
try.
---
 attachment_s3/models/ir_attachment.py | 49 +++++++++++++++++----------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/attachment_s3/models/ir_attachment.py b/attachment_s3/models/ir_attachment.py
index 9a4ae0e..2db77c3 100644
--- a/attachment_s3/models/ir_attachment.py
+++ b/attachment_s3/models/ir_attachment.py
@@ -27,6 +27,25 @@ except ImportError:
     _logger.debug("Cannot 'import boto'.")
 
 
+def clean_fs(files):
+    _logger.info('cleaning old files from filestore')
+    for full_path in files:
+        if os.path.exists(full_path):
+            try:
+                os.unlink(full_path)
+            except OSError:
+                _logger.info(
+                    "_file_delete could not unlink %s",
+                    full_path, exc_info=True
+                )
+            except IOError:
+                # Harmless and needed for race conditions
+                _logger.info(
+                    "_file_delete could not unlink %s",
+                    full_path, exc_info=True
+                )
+
+
 class IrAttachment(models.Model):
     _inherit = "ir.attachment"
 
@@ -271,22 +290,7 @@ class IrAttachment(models.Model):
                         # on assets
                         'mimetype': self.mimetype})
             _logger.info('moved %s on the object storage', fname)
-            full_path = self._full_path(fname)
-            _logger.info('cleaning fs self')
-            if os.path.exists(full_path):
-                try:
-                    os.unlink(full_path)
-                except OSError:
-                    _logger.info(
-                        "_file_delete could not unlink %s",
-                        full_path, exc_info=True
-                    )
-                except IOError:
-                    # Harmless and needed for race conditions
-                    _logger.info(
-                        "_file_delete could not unlink %s",
-                        full_path, exc_info=True
-                    )
+            return self._full_path(fname)
         elif self.db_datas:
             _logger.info('moving on the object storage from database')
             self.write({'datas': self.datas})
@@ -313,6 +317,7 @@ class IrAttachment(models.Model):
         with self.do_in_new_env(new_cr=new_cr) as new_env:
             attachment_model_env = new_env['ir.attachment']
             ids = attachment_model_env.search(domain).ids
+            files_to_clean = []
             for attachment_id in ids:
                 try:
                     with new_env.cr.savepoint():
@@ -333,11 +338,21 @@ class IrAttachment(models.Model):
                         # attachments on each loop.
                         new_env.clear()
                         attachment = attachment_model_env.browse(attachment_id)
-                        attachment._move_attachment_to_s3()
+                        path = attachment._move_attachment_to_s3()
+                        if path:
+                            files_to_clean.append(path)
                 except psycopg2.OperationalError:
                     _logger.error('Could not migrate attachment %s to S3',
                                   attachment_id)
 
+            def clean():
+                clean_fs(files_to_clean)
+
+            # delete the files from the filesystem once we know the changes
+            # have been committed in ir.attachment
+            if files_to_clean:
+                new_env.cr.after('commit', clean)
+
     @contextmanager
     def do_in_new_env(self, new_cr=False):
         """ Context manager that yields a new environment