Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FW][FIX] mail: show all attachment options #166094

Conversation

fw-bot
Copy link
Contributor

@fw-bot fw-bot commented May 19, 2024

The template for the attachment list adds a class to show multiple options when attachments can be deleted.

However the condition to display the delete button is different from the condition to apply that style.

Meaning if one condition showDelete is true while the other isDeletable is false it is impossible to access the download button as the correct style is not applied.

  • Fix the template to use the same condition everywhere
  • Fix the condition for showDelete to always be false is deletion is disallowed

Issue noticeable since [1] when attachments were made conditionally deletable as opposed to always deletable previously.

Steps to reproduce:

  • Post a couple attachments on the same message as a user in a chatter
  • Switch to a different user that is not allowed to delete the attachment (not admin)
  • You can't select the download button without using the tab key

1: 32a80a3

task-3519815


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

Forward-Port-Of: #166086
Forward-Port-Of: #165332

@robodoo robodoo added forwardport This PR was created by @fw-bot conflict There was an error while creating this forward-port PR labels May 19, 2024
@robodoo
Copy link
Contributor

robodoo commented May 19, 2024

Pull request status dashboard.

@fw-bot
Copy link
Contributor Author

fw-bot commented May 19, 2024

@reth-odoo cherrypicking of pull request #165332 failed.

stdout:

Auto-merging addons/mail/static/src/core/common/attachment_list.js
Auto-merging addons/mail/static/tests/message/message.test.js
CONFLICT (content): Merge conflict in addons/mail/static/tests/message/message.test.js

stderr:

09:51:29.280832 git.c:463               trace: built-in: git cherry-pick 3ce0d76771317802b03672fd0ef8b9c351e1c6d6
error: could not apply 3ce0d7677131... [FIX] mail: show all attachment options
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
----------
status:

Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).

In the former case, you may want to edit this PR message as well.

⚠️ after resolving this conflict, you will need to merge it via @robodoo.

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

@C3POdoo C3POdoo added the RD research & development, internal work label May 19, 2024
@reth-odoo reth-odoo force-pushed the saas-17.2-17.0-show-all-attachment-options-when-deletable-reth-lezI-fw branch from a3fc2ca to 0d26a92 Compare May 19, 2024 08:33
@reth-odoo
Copy link
Contributor

@robodoo r+

@C3POdoo C3POdoo requested review from a team May 19, 2024 08:35
The template for the attachment list adds a class to show multiple options
when attachments can be deleted.

However the condition to display the delete button is different from
the condition to apply that style.

Meaning if one condition `showDelete` is true while the other `isDeletable` is false
it is impossible to access the download button as the correct style is not applied.

- Fix the template to use the same condition everywhere
- Fix the condition for showDelete to always be false is deletion is disallowed

Issue noticeable since [1] when attachments were made conditionally deletable
as opposed to always deletable previously.

1: 32a80a3

task-3519815

X-original-commit: 281140f
@reth-odoo reth-odoo force-pushed the saas-17.2-17.0-show-all-attachment-options-when-deletable-reth-lezI-fw branch from 0d26a92 to f3daf5b Compare May 19, 2024 15:41
@reth-odoo
Copy link
Contributor

Can't use patchUserWithCleanup anymore because it's not in hoot, patching the pyenv setup method to get isAdmin which is even better anyway.

@robodoo r+

robodoo pushed a commit that referenced this pull request May 19, 2024
The template for the attachment list adds a class to show multiple options
when attachments can be deleted.

However the condition to display the delete button is different from
the condition to apply that style.

Meaning if one condition `showDelete` is true while the other `isDeletable` is false
it is impossible to access the download button as the correct style is not applied.

- Fix the template to use the same condition everywhere
- Fix the condition for showDelete to always be false is deletion is disallowed

Issue noticeable since [1] when attachments were made conditionally deletable
as opposed to always deletable previously.

1: 32a80a3

task-3519815

closes #166094

X-original-commit: 281140f
Signed-off-by: Renaud Thiry (reth) <reth@odoo.com>
@robodoo robodoo closed this May 19, 2024
Comment on lines +1173 to +1193
[
0,
0,
{
mimetype: "image/jpeg",
name: "BLAH",
res_id: partnerId,
res_model: "res.partner",
},
],
[
0,
0,
{
mimetype: "image/png",
name: "BLEH",
res_id: partnerId,
res_model: "res.partner",
},
],
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Command.create(data) rather than [0, 0, data]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, could probably also have used a list comprehension to be honest in hindsight

@fw-bot fw-bot deleted the saas-17.2-17.0-show-all-attachment-options-when-deletable-reth-lezI-fw branch June 2, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict There was an error while creating this forward-port PR forwardport This PR was created by @fw-bot RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants