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.

Sign up for GitHub

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

Jump to bottom

[16.0][MIG] mail_attach_existing_attachment_account from 14 to 16 #1508

Merged
merged 12 commits into from
Dec 17, 2024
Merged

[16.0][MIG] mail_attach_existing_attachment_account from 14 to 16 #1508

merged 12 commits into from
Dec 17, 2024

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Nov 22, 2024

No description provided.

tbaden and others added 10 commits November 22, 2024 19:06
@tbaden @sbidoul
[12.0][ADD]mail_attach_existing_attachment_account
327e566 
[UPD] README.rst

[ADD] icon.png

[UPD] Update mail_attach_existing_attachment_account.pot

[UPD] README.rst
@victoralmau @sbidoul
[IMP] mail_attach_existing_attachment_account: black, isort, prettier
bff9143
@Rad0van @sbidoul
[MIG] mail_attach_existing_attachment_account: Migration to 13.0
6f2280e 
TT28899

[UPD] Update mail_attach_existing_attachment_account.pot

[UPD] README.rst
@victoralmau @sbidoul
[IMP] mail_attach_existing_attachment_account: black, isort, prettier
f1b5604
@sbidoul
[MIG] mail_attach_existing_attachment_account: Migration to 14.0
0e7ef43 
TT32120
@oca-travis @sbidoul
[UPD] Update mail_attach_existing_attachment_account.pot
86c8ad5
@OCA-git-bot @sbidoul
[UPD] README.rst
40abec7
@pedrobaeza @sbidoul
[FIX] mail_attach_existing_attachment_account: Resilient tests on mul…
a9c9e80 
…ti-company

Doing a bulk search on accounts without company filter can lead to
selecting an account of other company, and thus, making the test to
fail.

We restrict the account search to the current company.
@sbidoul
[MIG] mail_attach_existing_attachment_account from 14.0
6f0d14e
@sbidoul
[IMP] mail_attach_existing_attachment_account: pre-commit stuff
d44f179
@sbidoul sbidoul marked this pull request as draft November 22, 2024 18:27
@sbidoul sbidoul marked this pull request as ready for review November 22, 2024 18:33
@sbidoul
Copy link
Member Author

sbidoul commented Nov 22, 2024

/ocabot migration mail_attach_existing_attachment_account

This remains useful for the Send and Print wizard on invoices.

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Nov 22, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 22, 2024
Open
39 tasks
@sbidoul
[DOC] mail_attach_existing_attachment_account: improve description
6df1e5a
@sbidoul
[IMP] mail_attach_existing_attachment_account: align with base module
6d61a47 
We show the field with the same technique as in the base module, which
improves the display, showing the label.
tarteo
tarteo approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Thanks! Works, tested it on runboat

image

tarteo added a commit to onesteinbv/addons-oca that referenced this pull request Dec 6, 2024
@tarteo
[UPD] mail_attach_existing_attachment_account from OCA/social (OCA/so…
0a6b4d2 
…cial#1508)
@pedrobaeza
Copy link
Member

pedrobaeza commented Dec 6, 2024 •
edited
Loading

I wonder if applying a similar technique like OCA/web#2804 in the base module, we can avoid this module at all. cc @CarlosRoca13

@CarlosRoca13
Copy link
Contributor

CarlosRoca13 commented Dec 9, 2024 •
edited
Loading

I wonder if applying a similar technique like OCA/web#2804 in the base module, we can avoid this module at all. cc @CarlosRoca13

@pedrobaeza I think it wouldn't apply in this case... The loading order is used for the assets. In this case, I believe the issue is that the wizard account.invoice.send is built from mail.compose.message, but it is done using _inherits, which requires adapting it in a certain way.

Although I am not entirely familiar with the module, perhaps @victoralmau can provide some insight on the matter.

@pedrobaeza
Copy link
Member

OK, thanks. It was just a random thought without digging deeper, as it's a nuisance to require this extension for accounting.

victoralmau
victoralmau approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Functional review OK.

I don't like the need for this module, but indeed #1508 (comment) seems to be necessary.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1508-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit dbccfbe into OCA:16.0 Dec 17, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at a9601af. Thanks a lot for contributing to OCA. ❤️

@sbidoul sbidoul deleted the 16.0-mig-mail_attach_existing_attachment_account branch December 17, 2024 14:59
@sbidoul
Copy link
Member Author

sbidoul commented Dec 17, 2024

Thanks for the reviews and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victoralmau victoralmau victoralmau approved these changes

@tarteo tarteo tarteo approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
9 participants
@sbidoul @pedrobaeza @CarlosRoca13 @OCA-git-bot @victoralmau @tarteo @tbaden @Rad0van @oca-travis