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

pivi-odoo · 2023-11-14T14:27:58Z

Issue

Main attachments on expense sheets was never correctly set, leading to missing attachment previews 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 an 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

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

robodoo · 2023-11-14T14:29:09Z

.

addons/hr_expense/models/hr_expense.py

addons/hr_expense/models/ir_attachment.py

pivi-odoo · 2023-11-22T08:07:11Z

Hi @xmo-odoo, thanks for your review.

Because the attachment is not copied over, the access checks will not be correctly performed and the relation is inconsistent (the main attachment of the sheet is not in any way related to a sheet let alone this one)

I've reworked completely the patch to follow your suggestion, I've had a bit of a hard time maintaining the sync between the attachments of the expenses and those on the sheet, so I decided to use the checksum of the attachments to avoid unnecessary trashing. If you see a better solution, I am all for it.

Let me know what you think.

JulienVR

It's good to me. Y ou'll just have to ask a runbot exception to the upgrade team to make the CI happy I think. :)

addons/hr_expense/models/hr_expense.py

Xavier-Do · 2023-11-27T08:56:11Z

Upgrade exception #277 added.

Waiting the forward-port of this pr, the exception will be applied on all builds, and create an inconsistent state for migrations.
Please forward port this asap up to master without change. Exception should be forward-ported before the end of the week.

If you need to apply any change before it reaches master, please notify runbot team.

Details:

field:hr.expense.sheet.attachment_ids
field:hr.expense.attachment_ids

cc @KangOl @nseinlet

pivi-odoo · 2023-11-27T08:57:49Z

Hi @JulienVR,

The Runbot's template exception has been granted, it just need the r+ :)

JulienVR · 2023-11-27T08:59:24Z

I don't have r+ rights 😓, but @csnauwaert do (I'm only pre-reviewing PRs)

seb-odoo

Hello, some comments as I'm passing by.

Also, as this is rather tricky, we should have some tests to cover this behavior.

addons/hr_expense/models/hr_expense.py

pivi-odoo · 2023-11-28T08:50:29Z

@JulienVR @csnauwaert I've updated the PR, to apply @seb-odoo and @ryv-odoo suggestions of moving the syncing of the attachments to the CRUD operations. I've also added a test that covers the use-cases that I could think of.

I guess the previous approval is considered outdated, since the patch completely changed.

Since I'm extending ir.attachment now to override the create and unlink, I guess I need to reping @odoo/rd-security (@xmo-odoo) but I think it's ok.

xmo-odoo

I don't really get why you're now full-sync-ing the sheet with the expenses, my understanding of the original was just that the sheet should get an (any) attachment from its expenses in order not to be attachmentless, but 🤷

addons/hr_expense/models/hr_expense.py

addons/hr_expense/models/ir_attachment.py

addons/hr_expense/models/hr_expense.py

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in /pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 Part-of: #155417

In /pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in /pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes #155417 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in odoo/pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 X-original-commit: 912b91c

In odoo/pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in odoo/pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. X-original-commit: 0624487

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in /pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 X-original-commit: 912b91c Part-of: #164640

In /pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in /pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes #164640 X-original-commit: 0624487 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in odoo/pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922

In odoo/pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in odoo/pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes odoo#155417 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in /pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 Part-of: #164645

In /pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in /pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes #155417 closes #164645 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com> Signed-off-by: Ali Alfie (alal) <alal@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in odoo/pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922

In odoo/pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in odoo/pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes odoo#155417 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in /pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 Part-of: #165117

In /pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in /pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes #155417 closes #165117 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

In an expense sheet, when an expense line is clicked on, its receipts are shown in the attachment viewer. This was broken in odoo/pull/142029 since it duplicates the attachments from the expense, leaving inconsistency between the attachment ID of the lines and the ones in the sheet. To fix this, the checksum is used instead to find which attachment in the sheet corresponds to the one in the line that was clicked on. task-3758922 Part-of: odoo#165117

In odoo/pull/138387, a new feature was added where clicking on an expense line in an expense sheet will highlight the receipts of that line in the attachment viewer if any. A JS test was included for this feature that mocked the fetching of the sheet's attachment. Since this changed in the backend in odoo/pull/142029, the feature no longer worked but the test never broke since it was mocking the backend. This commit adds a tour to prevent that from happening again. closes odoo#155417 closes odoo#165117 Signed-off-by: Habib Ayob (ayh) <ayh@odoo.com>

C3POdoo requested review from a team and JulienVR and removed request for a team November 14, 2023 14:29

C3POdoo added the OE the report is linked to a support ticket (opw-...) label Nov 14, 2023

JulienVR reviewed Nov 15, 2023

View reviewed changes

addons/hr_expense/models/hr_expense.py Show resolved Hide resolved

xmo-odoo reviewed Nov 16, 2023

View reviewed changes

addons/hr_expense/models/ir_attachment.py Outdated Show resolved Hide resolved

pivi-odoo force-pushed the 16.0-opw-3572440-pivi branch from 8a809b2 to 07fc04f Compare November 21, 2023 15:21

pivi-odoo requested a review from xmo-odoo November 22, 2023 08:07

JulienVR approved these changes Nov 24, 2023

View reviewed changes

addons/hr_expense/models/hr_expense.py Outdated Show resolved Hide resolved

pivi-odoo force-pushed the 16.0-opw-3572440-pivi branch 2 times, most recently from d75e136 to de9bf7c Compare November 27, 2023 07:46

JulienVR requested a review from csnauwaert November 27, 2023 08:57

seb-odoo reviewed Nov 27, 2023

View reviewed changes

addons/hr_expense/models/hr_expense.py Outdated Show resolved Hide resolved

addons/hr_expense/models/hr_expense.py Outdated Show resolved Hide resolved

addons/hr_expense/models/hr_expense.py Outdated Show resolved Hide resolved

ryv-odoo reviewed Nov 27, 2023

View reviewed changes

pivi-odoo force-pushed the 16.0-opw-3572440-pivi branch from de9bf7c to e5ff916 Compare November 28, 2023 06:59

xmo-odoo requested changes Nov 28, 2023

View reviewed changes

pivi-odoo marked this pull request as draft November 28, 2023 09:56

pivi-odoo force-pushed the 16.0-opw-3572440-pivi branch 2 times, most recently from 068ef92 to 0defce2 Compare December 6, 2023 12:07

This was referenced May 6, 2024

[FW][FIX] hr_expense: view receipts of expense line when clicked on #164640

Closed

[FW][FIX] hr_expense: view receipts of expense line when clicked on #164645

Closed

fw-bot mentioned this pull request May 10, 2024

[FW][FIX] hr_expense: view receipts of expense line when clicked on #165117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pivi-odoo commented Nov 14, 2023 •

edited

robodoo commented Nov 14, 2023 •

edited

pivi-odoo commented Nov 22, 2023

JulienVR left a comment

Xavier-Do commented Nov 27, 2023

pivi-odoo commented Nov 27, 2023

JulienVR commented Nov 27, 2023

seb-odoo left a comment •

edited

pivi-odoo commented Nov 28, 2023 •

edited

xmo-odoo left a comment

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

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

Conversation

pivi-odoo commented Nov 14, 2023 • edited

Issue

Steps to reproduce

Cause

Fix

Affected versions

Reference

robodoo commented Nov 14, 2023 • edited

pivi-odoo commented Nov 22, 2023

JulienVR left a comment

Choose a reason for hiding this comment

Xavier-Do commented Nov 27, 2023

pivi-odoo commented Nov 27, 2023

JulienVR commented Nov 27, 2023

seb-odoo left a comment • edited

Choose a reason for hiding this comment

pivi-odoo commented Nov 28, 2023 • edited

xmo-odoo left a comment

Choose a reason for hiding this comment

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

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

pivi-odoo commented Nov 14, 2023 •

edited

robodoo commented Nov 14, 2023 •

edited

seb-odoo left a comment •

edited

pivi-odoo commented Nov 28, 2023 •

edited