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

[IMP][16.0] mis_builder: add possibility to dynamically hide a period depending on instance date #629

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from
Open

[IMP][16.0] mis_builder: add possibility to dynamically hide a period depending on instance date #629

wants to merge 1 commit into from

Conversation

AnizR
Copy link
Contributor

@AnizR AnizR commented Aug 28, 2024

Description

Related to issue: #617

This PR adds a boolean 'hide_period_based_on_instance_date' on periods of an instance.
When activated, the period will be shown only if the date of the instance is greater or equal to the end date of the period.

image

@OCA-git-bot
Copy link
Contributor

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

@AnizR AnizR force-pushed the imp-filter-period-base-date branch 2 times, most recently from 84156ee to aa976ee Compare August 28, 2024 09:27
SAnnabelle
SAnnabelle approved these changes Aug 28, 2024
View reviewed changes
Copy link

@SAnnabelle SAnnabelle left a comment

Choose a reason for hiding this comment

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

Functionally it's correct.

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

sbidoul
sbidoul reviewed Sep 10, 2024
View reviewed changes
mis_builder/models/mis_report_instance.py
@@ -284,6 +284,10 @@ def _compute_dates(self):
help="A domain to additionally filter move lines considered in this column.",
)

hide_period_based_on_instance_date = fields.Boolean(
help="Dynamically hide this period depending on the base date of the instance",
)
Copy link
Member

Choose a reason for hiding this comment

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

By just reading the field name and help text I don't understand what this does. Can you propose something more explanatory ?

Is it not better to talk about pivot date rather than base date? Not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It tried to improve it, what do you think?

mis_builder/models/mis_report_instance.py Outdated Show resolved Hide resolved
mis_builder/models/mis_report_instance.py Outdated Show resolved Hide resolved
mis_builder/views/mis_report_instance.xml Outdated
@@ -414,6 +414,7 @@
options="{'model': 'source_aml_model_name'}"
attrs="{'invisible': [('source_aml_model_name', '=', False)]}"
/>
<field name="hide_period_based_on_instance_date" />
Copy link
Member

Choose a reason for hiding this comment

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

I think this field should be hidden for some period types. For instance it does not make sense for comparison columns?

Copy link
Contributor Author

@AnizR AnizR Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've hide the field for comarison columns, is there any other case where I should hide this feature?

EDIT: I will do some changes #629 (comment)

sbidoul
sbidoul requested changes Oct 1, 2024
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

A few comments to handle.

@AnizR AnizR force-pushed the imp-filter-period-base-date branch 5 times, most recently from bcbded6 to 384bace Compare October 15, 2024 15:42
@AnizR
Copy link
Contributor Author

AnizR commented Oct 16, 2024 •
edited
Loading

I have to make some changes:

  • If the field to dynamically hide period is not set to true, the col is always displayed
  • If the mode is fix, the period is displayed if the pivot date is before the end
  • A period with a source 'Acutals' or 'Actuals (alternative)' will always be displayed if the mode is not 'fix'
  • A period that sums columnns will be displayed if its 'source columns' should be displayed
  • A period that compares columns will be displayed if its 'source columns' should be displayed

@AnizR AnizR marked this pull request as draft October 16, 2024 09:24
@AnizR AnizR force-pushed the imp-filter-period-base-date branch 3 times, most recently from 06b99ff to 6ec757f Compare October 16, 2024 10:17
@AnizR AnizR marked this pull request as ready for review October 16, 2024 10:24
@AnizR AnizR force-pushed the imp-filter-period-base-date branch from 6ec757f to 151cd99 Compare October 22, 2024 07:17
@AnizR AnizR force-pushed the imp-filter-period-base-date branch from 151cd99 to fff0139 Compare October 22, 2024 12:23
@AnizR AnizR force-pushed the imp-filter-period-base-date branch 2 times, most recently from a654214 to 4855ba5 Compare October 24, 2024 14:56
@AnizR
[IMP][16.0] mis_builder: add possibility to dynamically hide a period…
b143553 
… depending on instance date

[IMP] mis_builder: make description of hide_period_based_on_instance_date mor 'explanatory'
[IMP] mis_builder: cleanup code
@AnizR AnizR force-pushed the imp-filter-period-base-date branch from 4855ba5 to b143553 Compare October 28, 2024 07:39
@sbidoul sbidoul added the 16.0 label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbidoul sbidoul sbidoul requested changes

@SAnnabelle SAnnabelle SAnnabelle approved these changes

@acsonefho acsonefho acsonefho approved these changes

Assignees
No one assigned
Labels
16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AnizR @OCA-git-bot @sbidoul @acsonefho @SAnnabelle