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][IMP] partner_invoicing_mode: Allow better inheritance + use correctly grouping option #1515

Merged
merged 9 commits into from
May 2, 2024

Conversation

rousseldenis
Copy link
Contributor

@rousseldenis rousseldenis commented Jul 27, 2023

Improvements:

  • Move the invoicing code to base module from weekly module (and test code).
  • Fix the grouping functionality as it does not work if several sale orders have mixed values (for grouping).
  • Add a field that compute the next invoice date (e.g.: for the 14 days module, see [ADD] partner_invoicing_mode_fourteen_days #1534)
  • Allow to change the grouping option per sale order.

UPDATE:

  • Added some hooks to allow changing behavior when validating invoices.
  • Uses the grouping function from sale module

@rousseldenis
Copy link
Contributor Author

@StefanRijnhart @jcoux @TDu

@rafaelbn
Copy link
Member

@rafaelbn rafaelbn closed this Aug 16, 2023
@rafaelbn rafaelbn added this to the 16.0 milestone Aug 16, 2023
@rafaelbn rafaelbn reopened this Aug 16, 2023
@rousseldenis
Copy link
Contributor Author

@rousseldenis this módulo is already merged!

https://github.com/OCA/account-invoicing/tree/16.0/partner_invoicing_mode

😄

Maaaaah, this is an improvement... So, please reopen it

@rafaelbn
Copy link
Member

Hahaha! Sorry! After-holidays stuf!!

@StefanRijnhart , please could you review this improvements?

Thank you ! And sorry for the noise! 🙏🏼

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

I usualy don't like more dependencies , but I leave the review to @TDu and @StefanRijnhart .

@@ -8,9 +8,11 @@
"website": "https://github.com/OCA/account-invoicing",
"license": "AGPL-3",
"category": "Accounting & Finance",
"depends": ["account", "queue_job", "sale"],
"depends": ["account", "base_partition", "queue_job", "sale"],
Copy link
Member

Choose a reason for hiding this comment

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

So the main big change is using a new dependency. We are not using in this moment this module. I didn't know about it

https://github.com/OCA/server-tools/tree/16.0/base_partition

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's not a big one. Technical one.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, code reuse is always good.

@rafaelbn rafaelbn requested a review from TDu August 16, 2023 17:40
@rafaelbn
Copy link
Member

@rousseldenis in case this PR is merged with new dependencies, then a new PR is needed to https://github.com/OCA/account-invoicing/tree/16.0/partner_invoicing_mode_monthly

Is it?

Runbots don't looks like building 🟠

@rousseldenis
Copy link
Contributor Author

Runbots don't looks like building 🟠

It's not a failed build (not red)

It happens from time to time, relaunching the build is sufficient in most cases.

@rousseldenis
Copy link
Contributor Author

IMHO @OCA/accounting-maintainers automatic generation of invoices should deserves its own repository

@rafaelbn
Copy link
Member

rafaelbn commented Aug 18, 2023

IMHO https://github.com/orgs/OCA/teams/accounting-maintainers automatic generation of invoices should deserves its own repository

I agree, this module prepare to automate but don't automate. We use this one and the others, month, week, etc to classify not to automate

This module just add the periodicity to invoicing, monthly, weekley and so no like terms (due date) or methods (how the pay)

  1. Payment term (dates)
  2. Payment mode (how)
  3. Invoicing period (every xx time or in a concret moment like shipping) <== Invoicing Mode

That's why I don't like to depend in base_partition because then for this so simple function we should leave this module and create a new one simpler for the point 3

Let's see what @TDu and @StefanRijnhart says 😄

It happens from time to time, relaunching the build is sufficient in most cases.

Thank you @rousseldenis !

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Tested, I'm unable to make this works see:

test1-16-IMP-partner_invoicing_mode-1515.mp4

@rousseldenis
Copy link
Contributor Author

That's why I don't like to depend in base_partition because then for this so simple function we should leave this module and create a new one simpler for the point 3

I don't understand the 'why'... This is a helper module that implement a partition on recordsets in order to split them on criteria.

To avoid writing too much code, that one is very useful in many modules. It does not introduces database nor models changes. So not harmful at all. So, don't block for that.

@rousseldenis
Copy link
Contributor Author

rousseldenis commented Aug 22, 2023

@TDu I have two questions on this as it seems to have not been integrated in OCA module:

  • The grouping mode and the invoicing mode on payment mode level
  • The ability to define the grouping mode on sale order level separately from configuration on partner or payment mode (as one may want to exclude some sale orders individually)

Thanks for your answers

@jbaudoux

@rousseldenis
Copy link
Contributor Author

I have answered the first one (not used).

I will implement the second one here too. So, we'll be able to set the grouping option on sale order individually (e.g.: grouping is set on partner but he wants to have a separate invoice for a particular sale order).

@rousseldenis rousseldenis force-pushed the 16.0-imp-partner-invoicing-mode-dro branch 2 times, most recently from 8246e47 to 36fc3e1 Compare August 22, 2023 16:17
@rousseldenis rousseldenis force-pushed the 16.0-imp-partner-invoicing-mode-dro branch from b81c01c to 9854381 Compare March 29, 2024 19:13
@rousseldenis
Copy link
Contributor Author

@jbaudoux Done

In order to be able to benefits from standard grouping function, override it
and add missing keys.
Copy link

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

All goods for me.

Would be nice to merge this one as there are many PR depending on it

@rousseldenis
Copy link
Contributor Author

@OCA/accounting-maintainers

@cyrilmanuel
Copy link

cyrilmanuel commented Apr 18, 2024

hi @sbidoul, @pedrobaeza can you merge this PR please ?

Copy link
Contributor

@bosd bosd left a comment

Choose a reason for hiding this comment

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

Code review LGTM

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

@bosd
Copy link
Contributor

bosd commented May 1, 2024

@OCA/accounting-maintainers
Or @AaronHForgeFlow Do you have super powers here?

@StefanRijnhart
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1515-by-StefanRijnhart-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 1, 2024
Signed-off-by StefanRijnhart
@StefanRijnhart
Copy link
Member

I'm afraid merges are stalled by OCA/oca-addons-repo-template#250 too.

@OCA-git-bot
Copy link
Contributor

@StefanRijnhart your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1515-by-StefanRijnhart-bump-major.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@StefanRijnhart
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1515-by-StefanRijnhart-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 10b2a82 into OCA:16.0 May 2, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

9 participants