[16.0][IMP] account_fiscal_year add fields on res.company #1506

legalsylvain · 2022-11-26T21:18:04Z

Hi, three simple commits to improve account_fiscal_year module.

359b71b : factorize code, adding a new function in account.fiscal.year model.
b920c77 : Add fields start date and end date of the current fiscal year on res.company model, and display on the views
c68335b : improve documentation, adding screenshots

New description

This module allows to create and edit fiscal years from the menu:

Invoicing > Configuration > Accounting > Fiscal Years

The start and end dates of the current fiscal years are then available in the company tree and form views.

OCA-git-bot · 2022-11-26T21:18:29Z

Hi @eLBati,
some modules you are maintaining are being modified, check this out!

legalsylvain · 2022-11-26T21:19:46Z

CC : last contributors : @baimont, @MiquelRForgeFlow, @SirTakobi, @pegonzalezspesol, @AlvaroTForgeFlow

SirTakobi

Thanks for the PR!

About the commit messages, in general they should explain why a change is done, not what is done: the what can be seen by the changes.
Also please follow https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message, more specifically:

The commit message

[REF] account_fiscal_year : factorize code

does not explain why that change is done, but only what is done (also, [REF] already says that there is a refactoring); it might be changed with something like allow to override fiscal year search or do not repeat same search?
There is no [DOC] prefix (I would squash it with the [IMP] commit)
The commit message

[IMP] account_fiscal_year : Add fields start date and end date of the current fiscal year on res.company model, and display on the views

is too long, and also does not explain why that change is done, but only what is done

The only thing that has to be changed is editing the po(t) file.
Everything else is just nitpicking 😄 and is not really needed for merge in my opinion.

SirTakobi · 2022-11-28T15:17:18Z

account_fiscal_year/models/account_fiscal_year.py

+    @api.model
+    def _get_fiscal_year(self, company, date_from, date_to):
+        """Return a fiscal year for the given company
+        that contains the two dates. (or False if no fiscal years)


This does not return False when no match is found, but an empty recordset of model account.fiscal.year.

indeed. Fixed.

SirTakobi · 2022-11-28T15:18:04Z

account_fiscal_year/models/account_fiscal_year.py

+    def _get_fiscal_year(self, company, date_from, date_to):
+        """Return a fiscal year for the given company
+        that contains the two dates. (or False if no fiscal years)
+        matches the selection"""


Maybe this line should be in the parenthesis? Or just removed?

SirTakobi · 2022-11-28T15:20:14Z

account_fiscal_year/models/account_fiscal_year.py

@@ -107,3 +107,17 @@ def _get_overlapping_domain(self):
                intersection_domain,
            ]
        )
+
+    @api.model
+    def _get_fiscal_year(self, company, date_from, date_to):


A method account.fiscal.year._get_fiscal_year looks like a bit vague in my opinion, maybe it can be moved in the company and called _get_fiscal_year_by_date?
Or it should have a name explaining what it does

Fixed. Thanks.

SirTakobi · 2022-11-28T15:25:26Z

account_fiscal_year/models/res_company.py

@@ -20,15 +20,13 @@ def compute_fiscalyear_dates(self, current_date):
        """
        self.ensure_one()

+        AccountFiscalYear = self.env["account.fiscal.year"]


Variable names should use snake_case both because it is the default for variables in Python (https://peps.python.org/pep-0008/#function-and-variable-names) and because the new code should fit in the style of its surroundings.

SirTakobi · 2022-11-28T15:28:54Z

account_fiscal_year/i18n/account_fiscal_year.pot

@@ -48,6 +48,11 @@ msgstr ""
 msgid "End Date"
 msgstr ""

+#. module: account_fiscal_year


po(t) files should not be edited: they are managed by weblate. This is also stated in the guidelines:

Pull requests should never directly modify .po files

(ref. https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#11translations)

Hi. I do not understand that rule.

How can I translate in my own instance, a new field created by an unmerged PR ? how do you do ?

SirTakobi · 2022-11-28T15:30:57Z

account_fiscal_year/models/res_company.py

+    )
+
+    def _compute_fiscal_year_dates(self):
+        today = fields.Date.today()


Can a test be added covering this new code?

rafaelbn · 2022-12-23T13:13:37Z

Hello @legalsylvain , you have here pending answering the review 👍 🙏

legalsylvain · 2023-12-04T11:17:10Z

Hello @legalsylvain , you have here pending answering the review 👍 🙏

Hi @rafaelbn
I totally missed this one. Thanks for the reminder !

I'll take a look.

github-actions · 2024-04-07T12:20:56Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

legalsylvain

Fixed. @rafaelbn is it now OK ?

legalsylvain · 2024-02-12T15:47:19Z

account_fiscal_year/i18n/account_fiscal_year.pot

@@ -48,6 +48,11 @@ msgstr ""
 msgid "End Date"
 msgstr ""

+#. module: account_fiscal_year


Hi. I do not understand that rule.

How can I translate in my own instance, a new field created by an unmerged PR ? how do you do ?

legalsylvain · 2024-02-12T15:52:47Z

account_fiscal_year/models/account_fiscal_year.py

@@ -107,3 +107,17 @@ def _get_overlapping_domain(self):
                intersection_domain,
            ]
        )
+
+    @api.model
+    def _get_fiscal_year(self, company, date_from, date_to):


Fixed. Thanks.

legalsylvain · 2024-02-12T15:54:01Z

account_fiscal_year/models/account_fiscal_year.py

+    @api.model
+    def _get_fiscal_year(self, company, date_from, date_to):
+        """Return a fiscal year for the given company
+        that contains the two dates. (or False if no fiscal years)


indeed. Fixed.

rafaelbn · 2024-04-07T18:38:01Z

/ocabot rebase

… current fiscal year on res.company model, and display on the views

OCA-git-bot · 2024-04-07T18:38:29Z

Congratulations, PR rebased to 16.0.

rafaelbn · 2024-04-07T18:40:58Z

@SirTakobi test pass could you please review the review? 😄 @legalsylvain attend comments!

github-actions · 2024-08-11T12:25:57Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

legalsylvain · 2024-08-11T12:41:55Z

@rafaelbn. Could we merge this one ? it is in production since 1 year without any problem.

rafaelbn · 2024-08-12T23:06:49Z

Hello @SirTakobi ,

Could we please have your approval here?

Lot of work ❤️

Thank you! 😄

rafaelbn · 2024-08-14T20:03:33Z

/ocabot merge minor

OCA-git-bot · 2024-08-14T20:04:03Z

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1506-by-rafaelbn-bump-minor, awaiting test results.

rafaelbn · 2024-08-14T20:04:18Z

Done @legalsylvain !

OCA-git-bot · 2024-08-14T20:17:59Z

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

SirTakobi suggested changes Nov 28, 2022

View reviewed changes

rafaelbn added this to the 16.0 milestone Dec 23, 2022

rafaelbn added the needs review label Dec 23, 2022

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 7, 2024

legalsylvain commented Apr 7, 2024

View reviewed changes

rafaelbn requested a review from SirTakobi April 7, 2024 18:37

rafaelbn removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 7, 2024

legalsylvain added 3 commits April 7, 2024 18:38

[REF] account_fiscal_year : factorize code

0ddb7ba

[IMP] account_fiscal_year : Add fields start date and end date of the…

687e14a

… current fiscal year on res.company model, and display on the views

[DOC] account_fiscal_year : improve documentation, adding screenshots

5272859

OCA-git-bot force-pushed the 16.0-account_fiscal_year-refactor branch from c68335b to 5272859 Compare April 7, 2024 18:38

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 11, 2024

OCA-git-bot added the bot is merging ⏳ label Aug 14, 2024

OCA-git-bot merged commit f8ec450 into OCA:16.0 Aug 14, 2024
6 of 7 checks passed

OCA-git-bot removed the bot is merging ⏳ label Aug 14, 2024

OCA-git-bot added the merged 🎉 label Aug 14, 2024

legalsylvain mentioned this pull request Aug 15, 2024

[FW][17.0][IMP] account_fiscal_year add fields on res.company #1924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][IMP] account_fiscal_year add fields on res.company #1506

[16.0][IMP] account_fiscal_year add fields on res.company #1506

legalsylvain commented Nov 26, 2022

OCA-git-bot commented Nov 26, 2022

legalsylvain commented Nov 26, 2022

SirTakobi left a comment

SirTakobi Nov 28, 2022

legalsylvain Feb 12, 2024

SirTakobi Nov 28, 2022

SirTakobi Nov 28, 2022

legalsylvain Feb 12, 2024

SirTakobi Nov 28, 2022

SirTakobi Nov 28, 2022

legalsylvain Feb 12, 2024

SirTakobi Nov 28, 2022

rafaelbn commented Dec 23, 2022

legalsylvain commented Dec 4, 2023

github-actions bot commented Apr 7, 2024

legalsylvain left a comment

legalsylvain Feb 12, 2024

legalsylvain Feb 12, 2024

legalsylvain Feb 12, 2024

rafaelbn commented Apr 7, 2024

OCA-git-bot commented Apr 7, 2024

rafaelbn commented Apr 7, 2024

github-actions bot commented Aug 11, 2024

legalsylvain commented Aug 11, 2024

rafaelbn commented Aug 12, 2024

rafaelbn commented Aug 14, 2024

OCA-git-bot commented Aug 14, 2024

rafaelbn commented Aug 14, 2024

OCA-git-bot commented Aug 14, 2024

[16.0][IMP] account_fiscal_year add fields on res.company #1506

[16.0][IMP] account_fiscal_year add fields on res.company #1506

Conversation

legalsylvain commented Nov 26, 2022

New description

OCA-git-bot commented Nov 26, 2022

legalsylvain commented Nov 26, 2022

SirTakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Dec 23, 2022

legalsylvain commented Dec 4, 2023

github-actions bot commented Apr 7, 2024

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Apr 7, 2024

OCA-git-bot commented Apr 7, 2024

rafaelbn commented Apr 7, 2024

github-actions bot commented Aug 11, 2024

legalsylvain commented Aug 11, 2024

rafaelbn commented Aug 12, 2024

rafaelbn commented Aug 14, 2024

OCA-git-bot commented Aug 14, 2024

rafaelbn commented Aug 14, 2024

OCA-git-bot commented Aug 14, 2024