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

[14.0.0][MIG] account_template_active #1187

Merged
merged 3 commits into from
Aug 30, 2021
Merged

[14.0.0][MIG] account_template_active #1187

merged 3 commits into from
Aug 30, 2021

Conversation

StefanRijnhart
Copy link
Member

@StefanRijnhart StefanRijnhart commented May 18, 2021 •
edited
Loading

Port of #978

@StefanRijnhart StefanRijnhart added this to the 14.0 milestone May 18, 2021
@StefanRijnhart StefanRijnhart mentioned this pull request May 18, 2021
Merged
@StefanRijnhart StefanRijnhart force-pushed the 14.0-MIG-account_template_active branch from c4907ad to fc05652 Compare May 19, 2021 07:29
legalsylvain
legalsylvain reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

One minor remark. LGTM otherwise. Thanks for porting this module.

account_template_active/readme/USAGE.rst Outdated Show resolved Hide resolved
@StefanRijnhart StefanRijnhart force-pushed the 14.0-MIG-account_template_active branch from fc05652 to 980efa0 Compare May 24, 2021 11:43
@StefanRijnhart StefanRijnhart force-pushed the 14.0-MIG-account_template_active branch from 980efa0 to 73fe7cb Compare July 6, 2021 09:26
@legalsylvain
Copy link
Contributor

hi @OCA/accounting-maintainers : some could review and merge this PR ?

the diff review is quite simple : 73fe7cb

thanks !

jcdrubay
jcdrubay reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@jcdrubay jcdrubay left a comment

Choose a reason for hiding this comment

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

LGTM after checking the last commit only.

Just wondering why not using the widget="boolean_toggle" on both form views

  • view_account_template_form
  • view_account_tax_template_form

@StefanRijnhart
Copy link
Member Author

@jcdrubay The boolean fields that there are on these forms (in core Odoo) do not apply the widget either (not in Odoo/master either). Mixing boolean widget looks ugly IMHO, and the usability improvement of the widget is less obvious in the case of the form view. Unless you've noticed that the widget is applied on form views throughout Odoo (14, or master), we have to assume that the convention in core Odoo is not to apply the widget in form views and we should stick to that.

@legalsylvain
Copy link
Contributor

@jcdrubay : thanks for your review.

@legalsylvain legalsylvain mentioned this pull request Aug 10, 2021
Closed
@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). 🤖

joao-p-marques
joao-p-marques reviewed Aug 30, 2021
View reviewed changes
Copy link
Member

@joao-p-marques joao-p-marques left a comment

Choose a reason for hiding this comment

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

Code review 👍

Could you just squash the 2 latest commits together please so that they follow the standard?

@StefanRijnhart @jcdrubay
[MIG] account_tax_template: Migration to 14.0
a7d2aa0 
Co-Authored-By: Jean-Charles Drubay <jc@komit-consulting.com>
@StefanRijnhart StefanRijnhart force-pushed the 14.0-MIG-account_template_active branch from 446d1b6 to a7d2aa0 Compare August 30, 2021 06:07
@StefanRijnhart
Copy link
Member Author

@joao-p-marques squash done, thanks!

joao-p-marques
joao-p-marques approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@joao-p-marques joao-p-marques left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! I think we can merge this, then

@joao-p-marques
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-1187-by-joao-p-marques-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4ac55cf into OCA:14.0 Aug 30, 2021
@OCA-git-bot
Copy link
Contributor

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

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

@joao-p-marques joao-p-marques joao-p-marques approved these changes

@legalsylvain legalsylvain legalsylvain approved these changes

@jcdrubay jcdrubay jcdrubay approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@StefanRijnhart @legalsylvain @OCA-git-bot @joao-p-marques @jcdrubay