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

[MIG] account_cutoff_base: Migration to 16.0 #213

Merged
merged 78 commits into from
Feb 16, 2023

Conversation

dzungtran89
Copy link

@dzungtran89 dzungtran89 commented Dec 5, 2022

Needs

Note:

  • I removed the groups from the company_id field is to avoid violating the check by the native, see here

Alexis de Lattre and others added 22 commits December 8, 2022 14:04
Use assert
Remove .keys()
No space before colon
PEP8/Flake8 : getting closer to compliancy
…de domains)

On account.account, type must be <> 'view' and <> 'closed'
…a single warning left !

Add translation template files.
The signature of existing methods is preserved.
Extract a new module account_invoice_start_end_dates from account_cutoff_prepaid
Use triple double quotes for docstring
Replace <openerp> by <odoo> in XML
Remove <data> tags in XML
Remove POT files
Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Non-blocking comments

company_currency_id = fields.Many2one(
related="parent_id.company_currency_id",
string="Company Currency",
readonly=True,

Choose a reason for hiding this comment

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

readonly is redundant here, in case you want to remove it

)
partner_id = fields.Many2one("res.partner", string="Partner", readonly=True)
quantity = fields.Float(digits="Product Unit of Measure", readonly=True)
price_unit = fields.Float(

Choose a reason for hiding this comment

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

This could be a Monetary field

Copy link
Contributor

Choose a reason for hiding this comment

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

No. price_unit are not monetary fields for decimal precision reasons, cf https://github.com/odoo/odoo/blob/16.0/addons/sale/models/sale_order_line.py#L139
A price_unit can have a decimal precision higher than the currency precision.

help="Price per unit (discount included) without taxes in the default "
"unit of measure of the product in the currency of the 'Currency' field.",
)
price_origin = fields.Char(readonly=True)

Choose a reason for hiding this comment

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

I fail to understand the purpose of this field, and why it is a Char but it's name suggests a price (thus it should be Float or Monetary)

Is this field even used? If so, what for? Should this be commented in the help attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a small note field that indicate where the price has been taken from. So it contains an invoice number, a PO number or an SO number.

price_origin = fields.Char(readonly=True)
origin_move_line_id = fields.Many2one(
"account.move.line", string="Origin Journal Item", readonly=True
) # Old name: move_line_id

Choose a reason for hiding this comment

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

Maybe remove the # Old name: comments.
Seem like they're outdated by now

origin_move_date = fields.Date(
related="origin_move_line_id.move_id.date", string="Origin Journal Entry Date"
) # old name: move_date
account_id = fields.Many2one(

Choose a reason for hiding this comment

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

IMO, this model should have _check_company_auto = True, and a check_company on this account_id field.
(Maybe also other fields, if relevant)

ondelete="cascade",
required=True,
)
tax_id = fields.Many2one("account.tax", string="Tax", required=True)

Choose a reason for hiding this comment

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

check_company maybe?

@dzungtran89
Copy link
Author

@ivantodorovich , thank you for you comments, the PR has been updated

About the field price_origin, I'm not sure about its purpose tbh, it was initially added in the commit

@alexis-via
Copy link
Contributor

I'm preparing a PR on this PR, to take into account my comments and make a few improvements

Revert price_unit to fields.Float() and price_origin to fields.Char()
Forward port post_cutoff_move on res.company from v14
Add ACLs for account readonly group
Remove duplicate-xml-fields, cf https://github.com/OCA/maintainer-tools/issues/558
Add name_get() on account.cutoff
Rename field move_label to move_ref
Update code to be able to add additional states (e.g. 'forecast' state introduced account_cutoff_start_end_dates)
Cleanup and improve views
@alexis-via
Copy link
Contributor

Here it is: dzungtran89#1

[16.0] account_cutoff_base: improve your OCA PR
@alexis-via
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-213-by-alexis-via-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f0d754e into OCA:16.0 Feb 16, 2023
@OCA-git-bot
Copy link
Contributor

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

@simahawk
Copy link
Contributor

/ocabot migration account_cutoff_base

@simahawk
Copy link
Contributor

@dzungtran89 next time, please, remove merge commits if you integrate other PRs. Thanks.

@simahawk simahawk added this to the 16.0 milestone Feb 23, 2023
@dzungtran89
Copy link
Author

hi @simahawk, well noted with thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet