Skip to content

[12.0][FIX] use the new methods for next period calculation result of the refactoring in #434 to consider the recurring_invoicing_type for monthlylastday mode and clean code - #439

Merged
OCA-git-bot merged 2 commits into
OCA:12.0from
acsone:12.0-fix_forecast-sbj
Dec 23, 2019

Conversation

@sbejaoui

@sbejaoui sbejaoui commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

After the #434, the invoicing type (pre-paid and post-paid) became relevant for monthly last day mode. This PR fix the invoice date for the forecast of this recurring mode.

@pedrobaeza pedrobaeza added this to the 12.0 milestone Dec 16, 2019
@sbidoul sbidoul changed the title [12.0][FIX] - Use get_next_invoice_date to compute forecast invoice date [12.0][FIX] contract_forecast: use get_next_invoice_date to compute forecast invoice date Dec 18, 2019
@sbidoul

sbidoul commented Dec 18, 2019

Copy link
Copy Markdown
Member

@sbejaoui is this a fix or an improvement? If this is a fix, could you add, as first commit, a failing test that reveals the error, then as a second commit, the fix, then as subsequent commits other improvements? That will facilitate review a lot. Thanks!

@sbejaoui

This comment has been minimized.

@sbejaoui
sbejaoui force-pushed the 12.0-fix_forecast-sbj branch from 2edb4f1 to eda0823 Compare December 18, 2019 16:39
@sbejaoui sbejaoui changed the title [12.0][FIX] contract_forecast: use get_next_invoice_date to compute forecast invoice date [12.0][FIX] use the new methods for next period calculation result of the refactoring in #434 to consider the recurring_invoicing_type for monthlylastday mode and clean code Dec 18, 2019
use the new methods for next period calculation result of the refactoring in OCA#434
to consider the recurring_invoicing_type for monthlylastday mode and clean code
@sbejaoui
sbejaoui force-pushed the 12.0-fix_forecast-sbj branch from eda0823 to f160b68 Compare December 18, 2019 16:50

@sbidoul sbidoul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

)
period_date_start, period_date_end, recurring_next_date = (
period_dates
):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this can be simplified by first initializing recurring_next_date = rec.recurring_next_date; period_date_end = rec.next_period_date_end; period_date_start = rec.next_period_date_start and then removing if rec.recurring_next_date and moving that test in the while condition. This is not blocking for this PR and can be done in a follow-up.

@sbejaoui

Copy link
Copy Markdown
Contributor Author

@gva-acsone, can you test and review this

@gva-acsone gva-acsone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM Functional test

@OCA-git-bot

Copy link
Copy Markdown
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). 🤖

@sbejaoui

Copy link
Copy Markdown
Contributor Author

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-439-by-sbejaoui-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Dec 23, 2019
Signed-off-by sbejaoui
@OCA-git-bot
OCA-git-bot merged commit f160b68 into OCA:12.0 Dec 23, 2019
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@sbidoul
sbidoul deleted the 12.0-fix_forecast-sbj branch December 23, 2019 09:56
BT-etejeda pushed a commit to BT-etejeda/contract that referenced this pull request May 17, 2024
Syncing from upstream OCA/contract (15.0)
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.

5 participants