[FIX] hr_expense: set main attachment on hr.expense.sheet

Issue:
Main attachments on expense sheets was never correctly set, leading
to missing attachment previous in the hr.expense.sheet form view,
and unnecessary RPC requests to `register_main_attachment`, because
the attachment was never set.

Steps to reproduce:
- Install Expenses
- Create 2 different expenses with different attachments
- Create a report from those 2 expenses, save it
- Notice there is nothing in the preview for the attachments, but if
  you zoom in or zoom out the page, then you can see it. Also
  swiping on the list of attachments isn't persistent, once you
  refresh the page, the main attachment we selected lastly is lost.

Cause:
There are 2 main issues:
- `hr.expense.sheet` has `message_main_attachment_id`, but it's
  never set.
- When writing an attachment via `register_main_attachment`, we
  pass only the id of the attachment. Then in the backend, the
  related record is based on the model of the attachment, but the
  attachment is linked to `hr.expense`, not `hr.expense.sheet`.
  Therefor we re-write the same attachment on the record we read the
  attachment from. And then the frontend will continue to call
  `register_main_attachment` everytime it tries to find the
  attachments linked to the sheet, but it's never done.

Fix:
- Add a compute to initially set the value of the
  `message_main_attachment_id` based on the spec "we take the first
  line that has a main attachment (attachment if no main attachment)
  and we set it on the sheet"
- The `hr.expense.sheet` now maintains a copies of the attachments
  linked to it's expenses, for proper ACL and functionality of
  `register_main_attachment`.

Affected versions:
16.0 up to master = saas-17.1

Reference:
task-3572440

closes odoo#151183

X-original-commit: 6ae4729
Signed-off-by: Cedric Snauwaert <csn@odoo.com>
Signed-off-by: Piryns Victor (pivi) <pivi@odoo.com>

addons/hr_expense/models/__init__.py

@@ -8,6 +8,7 @@
 from . import hr_department
 from . import hr_expense
 from . import hr_expense_sheet
+from . import ir_attachment
 from . import product_product
 from . import product_template
 from . import res_config_settings

addons/hr_expense/models/hr_expense.py

@@ -76,6 +76,12 @@ def _default_employee_id(self):
     quantity = fields.Float(required=True, digits='Product Unit of Measure', default=1)
     description = fields.Text(string="Internal Notes")
     nb_attachment = fields.Integer(string="Number of Attachments", compute='_compute_nb_attachment')
+    attachment_ids = fields.One2many(
+        comodel_name='ir.attachment',
+        inverse_name='res_id',
+        domain="[('res_model', '=', 'hr.expense')]",
+        string="Attachments",
+    )
     state = fields.Selection(
         selection=[
             ('draft', 'To Report'),
@@ -537,8 +543,11 @@ def _unlink_except_posted_or_approved(self):
                 raise UserError(_('You cannot delete a posted or approved expense.'))
 
     def write(self, vals):
+        expense_to_previous_sheet = {}
         if 'sheet_id' in vals:
             self.env['hr.expense.sheet'].browse(vals['sheet_id']).check_access_rule('write')
+            for expense in self:
+                expense_to_previous_sheet[expense] = expense.sheet_id
         if 'tax_ids' in vals or 'analytic_distribution' in vals or 'account_id' in vals:
             if any(not expense.is_editable for expense in self):
                 raise UserError(_('You are not authorized to edit this expense report.'))
@@ -554,8 +563,35 @@ def write(self, vals):
                     self.sheet_id.write({'employee_id': vals['employee_id']})
                 elif len(employees) > 1:
                     self.sheet_id = False
+        if 'sheet_id' in vals:
+            # The sheet_id has been modified, either by an explicit write on sheet_id of the expense,
+            # or by processing a command on the sheet's expense_line_ids.
+            # We need to delete the attachments on the previous sheet coming from the expenses that were modified,
+            # and copy the attachments of the expenses to the new sheet,
+            # if it's a no-op (writing same sheet_id as the current sheet_id of the expense),
+            # nothing should be done (no unlink then copy of the same attachments)
+            attachments_to_unlink = self.env['ir.attachment']
+            for expense in self:
+                previous_sheet = expense_to_previous_sheet[expense]
+                checksums = set((expense.attachment_ids - previous_sheet.expense_line_ids.attachment_ids).mapped('checksum'))
+                attachments_to_unlink += previous_sheet.attachment_ids.filtered(lambda att: att.checksum in checksums)
+                if vals['sheet_id'] and expense.sheet_id != previous_sheet:
+                    for attachment in expense.attachment_ids.with_context(sync_attachment=False):
+                        attachment.copy({
+                            'res_model': 'hr.expense.sheet',
+                            'res_id': vals['sheet_id'],
+                        })
+            attachments_to_unlink.with_context(sync_attachment=False).unlink()
         return res
 
+    def unlink(self):
+        attachments_to_unlink = self.env['ir.attachment']
+        for sheet in self.sheet_id:
+            checksums = set((sheet.expense_line_ids.attachment_ids & self.attachment_ids).mapped('checksum'))
+            attachments_to_unlink += sheet.attachment_ids.filtered(lambda att: att.checksum in checksums)
+        attachments_to_unlink.with_context(sync_attachment=False).unlink()
+        return super().unlink()
+
     @api.model
     def get_empty_list_help(self, help_message):
         return super().get_empty_list_help((help_message or '') + self._get_empty_list_mail_alias())

addons/hr_expense/models/hr_expense_sheet.py

@@ -195,6 +195,13 @@ def _default_journal_id(self):
         domain="[('id', 'in', selectable_payment_method_line_ids)]",
         help="The payment method used when the expense is paid by the company.",
     )
+    attachment_ids = fields.One2many(
+        comodel_name='ir.attachment',
+        inverse_name='res_id',
+        domain="[('res_model', '=', 'hr.expense.sheet')]",
+        string='Attachments of expenses',
+    )
+    message_main_attachment_id = fields.Many2one(compute='_compute_main_attachment', store=True)
     accounting_date = fields.Date(string="Accounting Date", compute='_compute_accounting_date', store=True)
     account_move_ids = fields.One2many(
         string="Journal Entries",
@@ -294,6 +301,17 @@ def _compute_state(self):
             else:
                 sheet.state = 'draft'
 
+    @api.depends('expense_line_ids.attachment_ids')
+    def _compute_main_attachment(self):
+        for sheet in self:
+            attachments = sheet.attachment_ids
+            if not sheet.message_main_attachment_id or sheet.message_main_attachment_id not in attachments:
+                expenses = sheet.expense_line_ids
+                expenses_mma_checksums = expenses.message_main_attachment_id.mapped('checksum')
+                sheet.message_main_attachment_id = attachments.filtered(
+                    lambda att: att.checksum in expenses_mma_checksums
+                )[:1] or attachments[:1]
+
     @api.depends('expense_line_ids.currency_id', 'company_currency_id')
     def _compute_currency_id(self):
         for sheet in self:
@@ -456,19 +474,6 @@ def _unlink_except_posted_or_paid(self):
     # Mail Thread
     # --------------------------------------------
 
-    def _get_mail_thread_data_attachments(self):
-        """
-            In order to see in the sheet attachment preview the corresponding
-            expenses' attachments, the latter attachments are added to the fetched data for the sheet record.
-        """
-        self.ensure_one()
-        res = super()._get_mail_thread_data_attachments()
-        expense_attachments = self.env['ir.attachment'].search(
-            [('res_id', 'in', self.expense_line_ids.ids), ('res_model', '=', 'hr.expense')],
-            order='id desc',
-        )
-        return res | expense_attachments
-
     def _track_subtype(self, init_values):
         self.ensure_one()
         if 'state' in init_values and self.state == 'draft':

addons/hr_expense/models/ir_attachment.py

@@ -0,0 +1,39 @@
+from odoo import models, api
+
+
+class IrAttachment(models.Model):
+    _inherit = 'ir.attachment'
+
+    @api.model_create_multi
+    def create(self, vals_list):
+        attachments = super().create(vals_list)
+        if self.env.context.get('sync_attachment', True):
+            expenses_attachments = attachments.filtered(lambda att: att.res_model == 'hr.expense')
+            if expenses_attachments:
+                expenses = self.env['hr.expense'].browse(expenses_attachments.mapped('res_id'))
+                for expense in expenses.filtered('sheet_id'):
+                    checksums = set(expense.sheet_id.attachment_ids.mapped('checksum'))
+                    for attachment in expense.attachment_ids.filtered(lambda att: att.checksum not in checksums):
+                        attachment.copy({
+                            'res_model': 'hr.expense.sheet',
+                            'res_id': expense.sheet_id.id,
+                        })
+        return attachments
+
+    def unlink(self):
+        if self.env.context.get('sync_attachment', True):
+            attachments_to_unlink = self.env['ir.attachment']
+            expenses_attachments = self.filtered(lambda att: att.res_model == 'hr.expense')
+            if expenses_attachments:
+                expenses = self.env['hr.expense'].browse(expenses_attachments.mapped('res_id'))
+                for expense in expenses.exists().filtered('sheet_id'):
+                    checksums = set(expense.attachment_ids.mapped('checksum'))
+                    attachments_to_unlink += expense.sheet_id.attachment_ids.filtered(lambda att: att.checksum in checksums)
+            sheets_attachments = self.filtered(lambda att: att.res_model == 'hr.expense.sheet')
+            if sheets_attachments:
+                sheets = self.env['hr.expense.sheet'].browse(sheets_attachments.mapped('res_id'))
+                for sheet in sheets.exists():
+                    checksums = set((sheet.attachment_ids & sheets_attachments).mapped('checksum'))
+                    attachments_to_unlink += sheet.expense_line_ids.attachment_ids.filtered(lambda att: att.checksum in checksums)
+            super(IrAttachment, attachments_to_unlink).unlink()
+        return super().unlink()

addons/hr_expense/tests/test_expenses.py

@@ -1,6 +1,7 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
-from datetime import date
 
+import base64
+from datetime import date
 from freezegun import freeze_time
 
 from odoo import Command
@@ -792,3 +793,91 @@ def test_payment_edit_fields(self):
             payment.write({'amount': 500})
 
         payment.write({'is_move_sent': True})
+
+    def test_expense_sheet_attachments_sync(self):
+        """
+        Test that the hr.expense.sheet attachments stay in sync with the attachments associated with the expense lines
+        Syncing should happen when:
+        - When adding/removing expense_line_ids on a hr.expense.sheet <-> changing sheet_id on an expense
+        - When deleting an expense that is associated with an hr.expense.sheet
+        - When adding/removing an attachment of an expense that is associated with an hr.expense.sheet
+        """
+        def assert_attachments_are_synced(sheet, attachments_on_sheet, sheet_has_attachment):
+            if sheet_has_attachment:
+                self.assertTrue(bool(attachments_on_sheet), "Attachment that belongs to the hr.expense.sheet only was removed unexpectedly")
+            self.assertSetEqual(
+                set(sheet.expense_line_ids.attachment_ids.mapped('checksum')),
+                set((sheet.attachment_ids - attachments_on_sheet).mapped('checksum')),
+                "Attachments between expenses and their sheet is not in sync.",
+            )
+
+        for sheet_has_attachment in (False, True):
+            expense_1, expense_2, expense_3 = self.env['hr.expense'].create([{
+                'name': 'expense_1',
+                'employee_id': self.expense_employee.id,
+                'product_id': self.product_c.id,
+                'total_amount': 1000,
+            }, {
+                'name': 'expense_2',
+                'employee_id': self.expense_employee.id,
+                'product_id': self.product_c.id,
+                'total_amount': 999,
+            }, {
+                'name': 'expense_3',
+                'employee_id': self.expense_employee.id,
+                'product_id': self.product_c.id,
+                'total_amount': 998,
+            }])
+            self.env['ir.attachment'].create([{
+                'name': "test_file_1.txt",
+                'datas': base64.b64encode(b'content'),
+                'res_id': expense_1.id,
+                'res_model': 'hr.expense',
+            }, {
+                'name': "test_file_2.txt",
+                'datas': base64.b64encode(b'other content'),
+                'res_id': expense_2.id,
+                'res_model': 'hr.expense',
+            }, {
+                'name': "test_file_3.txt",
+                'datas': base64.b64encode(b'different content'),
+                'res_id': expense_3.id,
+                'res_model': 'hr.expense',
+            }])
+
+            sheet = self.env['hr.expense.sheet'].create({
+                'company_id': self.env.company.id,
+                'employee_id': self.expense_employee.id,
+                'name': 'test sheet',
+                'expense_line_ids': [Command.set([expense_1.id, expense_2.id, expense_3.id])],
+            })
+
+            sheet_attachment = self.env['ir.attachment'].create({
+                'name': "test_file_4.txt",
+                'datas': base64.b64encode(b'yet another different content'),
+                'res_id': sheet.id,
+                'res_model': 'hr.expense.sheet',
+            }) if sheet_has_attachment else self.env['ir.attachment']
+
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            expense_1.attachment_ids.unlink()
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            self.env['ir.attachment'].create({
+                'name': "test_file_1.txt",
+                'datas': base64.b64encode(b'content'),
+                'res_id': expense_1.id,
+                'res_model': 'hr.expense',
+            })
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            expense_2.sheet_id = False
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            expense_2.sheet_id = sheet
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            sheet.expense_line_ids = [Command.set([expense_1.id, expense_3.id])]
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            expense_3.unlink()
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)
+            sheet.attachment_ids.filtered(
+                lambda att: att.checksum in sheet.expense_line_ids.attachment_ids.mapped('checksum')
+            ).unlink()
+            assert_attachments_are_synced(sheet, sheet_attachment, sheet_has_attachment)