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

This change is necessary to add quarterly period: Adding a quarter period type Improved fiscal period . Fixes #622 . #643

Draft
wants to merge 13 commits into
base: 17.0
Choose a base branch
from

Conversation

collokip169
Copy link

@collokip169 collokip169 commented Oct 15, 2024

This pull request seeks to add quarterly period . Adding a quarter period type Improved fiscal period . Fixes #622 . #643

@OCA-git-bot
Copy link
Contributor

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

@collokip169 collokip169 marked this pull request as draft October 15, 2024 06:08
@collokip169
Copy link
Author

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

okay

@collokip169
Copy link
Author

okay

@collokip169 collokip169 marked this pull request as ready for review October 15, 2024 08:14
@collokip169 collokip169 marked this pull request as draft October 15, 2024 08:14
@collokip169 collokip169 marked this pull request as ready for review October 15, 2024 08:15
@collokip169
Copy link
Author

@thomaspaulb could you please review this when you have time?

@thomaspaulb
Copy link

This pull request implements the following features:

This needs to be in the description of the PR, you can edit it there and remove this

@thomaspaulb
Copy link

Please also fix pre-commit errors

image

@collokip169 collokip169 changed the title quarterly period This pull request implements the following features: Adding a quarter period type Improved fiscal period . Fixes #622 . Oct 16, 2024
@collokip169 collokip169 marked this pull request as draft October 23, 2024 05:13
@collokip169 collokip169 changed the title This pull request implements the following features: Adding a quarter period type Improved fiscal period . Fixes #622 . This change is necessary to add quarterly period: Adding a quarter period type Improved fiscal period . Fixes #622 . Oct 23, 2024

Choose a reason for hiding this comment

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

Why are you adding an extra check?

Choose a reason for hiding this comment

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

What is this ZIP file doing here

Choose a reason for hiding this comment

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

You cant remove copyright headers

Choose a reason for hiding this comment

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

New submodule?

@@ -190,16 +203,18 @@ def _compute_dates(self):
("d", _("Day")),
("w", _("Week")),
("m", _("Month")),
("q", _("quarterly")),

Choose a reason for hiding this comment

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

To be in line with the others, this needs to be capitalized and the "ly" removed

Choose a reason for hiding this comment

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

Capitalized

("y", _("Year")),
("date_range", _("Date Range")),
],
string="Period type",
)
is_ytd = fields.Boolean(
is_ytd = (

Choose a reason for hiding this comment

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

This change is a bit weird, normally it's not necessary to put brackets around a field definition

<field name="offset" />
<field name="duration" />
<field name="offset" string="Offset (Quarters)" attrs="{'invisible': [('type', '!=', 'q')]}"/>
<field name="duration" string="Duration (Quarters)" attrs="{'invisible': [('type', '!=', 'q')]}"/>

Choose a reason for hiding this comment

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

This looks like a breaking change - why are people not allowed to see the offset and duration in other modes anymore? Eg for month offset and duration are also used, but now it would become invisible.

Choose a reason for hiding this comment

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

Not solved?

@@ -133,6 +133,19 @@ def _compute_dates(self):
record.date_from = fields.Date.to_string(date_from)
record.date_to = fields.Date.to_string(date_to)
record.valid = True

elif record.mode == MODE_REL and record.type == "q": # Quarterly period logic
date_from = d.replace(day=1)

Choose a reason for hiding this comment

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

This always takes the date_from as the start of the month, but in case of quarterly logic, it should be the start of the quarter that the d date falls in.

.pre-commit-config.yaml Show resolved Hide resolved
@@ -133,6 +133,27 @@ def _compute_dates(self):
record.date_from = fields.Date.to_string(date_from)
record.date_to = fields.Date.to_string(date_to)
record.valid = True

elif record.mode == MODE_REL and record.type == "Q": # Quarterly period logic
date_from = d.month(month=1)

Choose a reason for hiding this comment

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

month is not a function it's an integer, so this gives an error

elif d.month in [7, 8, 9]:
date_from = d.replace(month=7, day=1) # Q3
else:
date_from = d.replace(month=10, day=1) # Q4

Choose a reason for hiding this comment

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

You could combine this to date_from = replace(month=(datetime(2024, 7, 7).month - 1) % 3 + 1)

<field name="offset" />
<field name="duration" />
<field name="offset" string="Offset (Quarters)" attrs="{'invisible': [('type', '!=', 'q')]}"/>
<field name="duration" string="Duration (Quarters)" attrs="{'invisible': [('type', '!=', 'q')]}"/>

Choose a reason for hiding this comment

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

Not solved?

@sbidoul
Copy link
Member

sbidoul commented Nov 11, 2024

Hi, thanks a lot for contributing!

Could you please do a clean PR with only the changes to mis_builder, and nothing else: no changes to pre-commit config, no change to mis_builder_budget, ... I will then review.

@sbidoul sbidoul mentioned this pull request Nov 11, 2024
@pedrobaeza
Copy link
Member

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

Successfully merging this pull request may close these issues.

Add Quarter period type
6 participants