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

[16.0] [MIG] account_fiscal_year #1473

Merged
merged 47 commits into from
Oct 19, 2022

Conversation

baimont
Copy link
Contributor

@baimont baimont commented Oct 4, 2022

damdam-s and others added 30 commits October 4, 2022 09:37
* Add contributors to README
* try to find a FY start date according to the start date from choosen period
* Unable to unlink a date_range with type fiscal_year
* `fiscal_year` flag readonly
* add menu to date_range under accounting section
* remove method on  object because it's the same as in  file
* unable to delete  with flag 'fiscal_year' but can delete
* clean __openerp__.py
* account_fiscal_year version number
[MIG] Migrated module 'account_fiscal_year' to V10
[UPD] Update account_fiscal_year.pot
Update translation files

Updated by Update PO files to match POT (msgmerge) hook in Weblate.
This date.range.type in v11 have benn used in some cases. Then, when you migrate, you need to have this data or else delete its xmlid.

We think it's better to keep it, because you may want to still use it.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: account-financial-tools-12.0/account-financial-tools-12.0-account_fiscal_year
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-12-0/account-financial-tools-12-0-account_fiscal_year/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: account-financial-tools-13.0/account-financial-tools-13.0-account_fiscal_year
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-13-0/account-financial-tools-13-0-account_fiscal_year/
@oca-clabot
Copy link

Hey @baimont, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: https://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@baimont
Copy link
Contributor Author

baimont commented Oct 4, 2022

/ocabot migration account_fiscal_year

@OCA-git-bot
Copy link
Contributor

Sorry @baimont you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@adrienpeiffer
Copy link
Contributor

/ocabot migration account_fiscal_year

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 4, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 4, 2022
33 tasks
Copy link

@robinkeunen robinkeunen left a comment

Choose a reason for hiding this comment

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

Not an expert in accounting but here is my review, nothing blocking.

@@ -0,0 +1,127 @@
# Translation of Odoo Server.

Choose a reason for hiding this comment

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

Should this file be merged into fr.PO? It does not look like France-specific french.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is the time and place to do this.
OCA uses weblate for translations, right? https://odoo-community.org/resources/translate

Choose a reason for hiding this comment

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

I never took the time to investigate weblate, I guess it is smart about that.

account_fiscal_year/models/account_fiscal_year.py Outdated Show resolved Hide resolved
Comment on lines +97 to +102
intersection_domain = expression.OR(
[
intersection_domain_from,
intersection_domain_to,
intersection_domain_contain,
]
)

Choose a reason for hiding this comment

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

This could be simplified to the last of these three domains

        intersection_domain = expression.NOT(
            expression.OR([  # fiscal years without intersections
                ("date_to" < date_from),
                ("date_from" > date_to)
            ])
        )
        # equivalent to (Morgan's Law)
        intersection_domain = expression.NOT(
            expression.AND([
                expression.NOT(("date_to" < date_from)),
                expression.NOT("date_from" > date_to)
            ])
        )
        # equivalent to
        intersection_domain = expression.NOT(
            expression.AND([
                ("date_to" >= date_from),
                ("date_from" <= date_to),
            ])
        )

but that may not be as human readable (matter of taste, I understand the last one) not the place for these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the commit for this is c06c1bb

I wonder what @SimoRubi thinks of your comment

Choose a reason for hiding this comment

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

I actually had @SimoRubi just in front of me, he favors

        intersection_domain = expression.NOT(
            expression.OR([  # fiscal years without intersections
                ("date_to" < date_from),
                ("date_from" > date_to)
            ])
        )

Choose a reason for hiding this comment

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

@SimoRubi is no more active, I talked about it with @robinkeunen and yes I think I did it just for human readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robinkeunen ok to let it this way in this pr then?
if you want to change it you can always open a pr for v14.0

Choose a reason for hiding this comment

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

fair enough 👌

account_fiscal_year/models/res_company.py Outdated Show resolved Hide resolved
@baimont baimont force-pushed the 16.0-mig-account_fiscal_year-bai branch from d2817fe to 005211d Compare October 10, 2022 12:36
@baimont
Copy link
Contributor Author

baimont commented Oct 10, 2022

Not an expert in accounting but here is my review, nothing blocking.

Hi @robinkeunen, thanks for your review! I adapted the code or answered to your comments.

@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). 🤖

@rafaelbn
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1473-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5d32c55 into OCA:16.0 Oct 19, 2022
@OCA-git-bot
Copy link
Contributor

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

@Gelo-fl
Copy link
Contributor

Gelo-fl commented Jun 1, 2023

Hi @dreispt,

This module is merged, however the translations are locked:
account-financial-tools-16.0-account_fiscal_year

Can I know why?

Thanks in advance,

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.