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

[ADD] l10n_latam_check: new module #96074

Closed
wants to merge 2 commits into from

Conversation

zaoral
Copy link
Contributor

@zaoral zaoral commented Jul 14, 2022

latam task 445
related to opw-2528442

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Jul 14, 2022

@vbe-odoo
Copy link
Contributor

vbe-odoo commented Aug 5, 2022

@jjscarafia @jco-odoo I've tested the last changes and LGTM. All sequences are working properly when changing checkbooks, and all sequences are working as expected (even when changing them).

Ready for additional final tech review.
cc' @fvz-odoo @ren-odoo

addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
@jjscarafia jjscarafia force-pushed the master-latam-445-kz branch 2 times, most recently from 10fe2e0 to 42abc21 Compare August 26, 2022 10:15
@vbe-odoo
Copy link
Contributor

vbe-odoo commented Sep 6, 2022

Hello, @william-andre can you help me with the review of this PR, as this is a high-priority PR for AR.
If you can help us with your review this week, next week we can check the final details with @jco-odoo (he already did his last tech review) upon his return, and merge it.
Thanks!
cc' @ren-odoo @fvz-odoo

@william-andre
Copy link
Contributor

I will have a look at it after the freeze, I have quite a few of important PRs to review before this one as they are blocking for it.

@vbe-odoo
Copy link
Contributor

vbe-odoo commented Sep 6, 2022

@fhe-odoo as we discussed, we are still waiting for the last technical review and approval, but it will be reviewed after the freeze is done.

Still, this is a high-priority task from our end, so hopefully, it won't take too much time after the freeze is done.
Thanks!
cc' @ren-odoo @fvz-odoo @jco-odoo

@vbe-odoo
Copy link
Contributor

Hello, @william-andre @jco-odoo any luck with this PR review?
Thanks!
cc' @fhe-odoo @ren-odoo @fvz-odoo @jjscarafia @zaoral

@zaoral zaoral changed the base branch from master to 16.0 October 25, 2022 20:16
@C3POdoo C3POdoo requested review from a team and Levizar and removed request for a team October 25, 2022 20:21
Copy link
Contributor

@william-andre william-andre left a comment

Choose a reason for hiding this comment

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

I think I don't get the point of most of this.
Regarding third party checks, why aren't we going for something that looks like a simple approach (but naive?):

  • create a new model for checks not cashed in yet
  • create an delete (or change the status of) these records when you receive/give them when making a payment record

I think that a lot of the complicated things/tricks from the implementation will simply disappear with that approach.

addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/__manifest__.py Outdated Show resolved Hide resolved
@vbe-odoo
Copy link
Contributor

vbe-odoo commented Oct 26, 2022

I think I don't get the point of most of this. Regarding third party checks, why aren't we going for something that looks like a simple approach (but naive?):

  • create a new model for checks not cashed in yet
  • create an delete (or change the status of) these records when you receive/give them when making a payment record

I think that a lot of the complicated things/tricks from the implementation will simply disappear with that approach.

Just to give you some answers/ideas on the "why" of these 2 points (why we didn't use/create a check object, as that was one of the ideas at the beginning):

  1. We tried to avoid a new Object as much as possible
  2. The accounting team didn't want/suggested not to split the account.move.lines in the account.move coming from the payment in order to do the bank reconciliation per line/per check (as every check will have its own number). Because then on the bank statement, every check will be 1 line to be reconciled.

So based on the feedback on that point, from a technical perspective (no new object) and from a functional/accounting perspective (better not to split the journal items) is that we decided to move forward with the current approach of using the payment as a check (1 payment = 1 check).

image
image
image

cc' @william-andre @jco-odoo @jjscarafia @zaoral @fvz-odoo

addons/l10n_latam_check/__manifest__.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_payment.py Outdated Show resolved Hide resolved
addons/l10n_latam_check/models/account_journal.py Outdated Show resolved Hide resolved
@zaoral zaoral force-pushed the master-latam-445-kz branch 2 times, most recently from 180f4a1 to 8543eda Compare November 18, 2022 16:32
@jjscarafia
Copy link
Contributor

jjscarafia commented Dec 2, 2022

@william-andre @jco-odoo
Last comments where attended. Please let us know if we should make any other change.

Once you confirm it's ok we make a final squash.

Can you trigger install and test of this module on runbot? Because it's not installed automatically due to be "l10n_x"

cc @zaoral @vbe-odoo

Copy link
Contributor

@william-andre william-andre left a comment

Choose a reason for hiding this comment

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

Can you trigger install and test of this module on runbot? Because it's not installed automatically due to be "l10n_x"

Done!

@zaoral
Copy link
Contributor Author

zaoral commented Dec 2, 2022

@william-andre
All the commits has been squashed

@Xavier-Do
Copy link
Contributor

Upgrade exception #168 added.

Waiting the forward-port of this pr, the exception will be applied on all builds, and create an inconsistent state for migrations.
Please forward port this asap up to master without change. Exception should be forward-ported before the end of the week.

If you need to apply any change before it reaches master, please notify runbot team.

Details:

field:account.payment.show_check_number

cc @KangOl @nseinlet

@jco-odoo
Copy link
Contributor

jco-odoo commented Dec 2, 2022

In order to make this modules to also match new module l10n_latam_check.

* refactory of some methods to make it more heritable
* check number now is showed only when is defined and is related to
check_printing payment method.
@zaoral
Copy link
Contributor Author

zaoral commented Dec 2, 2022

Hi @jco-odoo

About the comment made by @Xavier-Do
The error is related to the new field show_check_number we are adding in the first commit of this PR. As far as I remember we already have that problem in the past. That was the reason we change the PR to master. In this case what we need to do to solve it?

@jco-odoo
Copy link
Contributor

jco-odoo commented Dec 2, 2022

@zaoral It was solved by @Xavier-Do allowing the exception.

New features to manage checks and managed them Odoo in latam countries
where they are widely use.

Own Checks Management
---------------------

Extends 'Check Printing Base' module to manage own checks with more features:

* allow using own checks that are not printed but filled manually by the user
* allow to use deferred or electronic checks
  * printing is disabled
  * check number is set manually by the user
* add an optional "Check Cash-In Date" for post-dated checks (deferred payments)
* add a menu to track own checks

Third Party Checks Management
-----------------------------

Add new "Third party check Management" feature.

There are 2 main Payment Methods additions:

* New Third Party Checks:

   * allow the user create a check on the fly from a payment
   * create a third party check from a customer payment

* Existing Third party check:

   * allow the user to reuse a Third party check already created
   * pay a vendor bill using an existing Third party check
   * move an existing checks between journals (i.e. move to Rejected)
   * Send/Receive again a check already used in a Vendor Bill/Customer INV
   * allow the user to do mass check transfers
@zaoral
Copy link
Contributor Author

zaoral commented Dec 2, 2022

Hi everyone!

Good news, PR is green, including l10n staging tests!
We think is ready fore merge @jco-odoo

@william-andre
Copy link
Contributor

@robodoo r+

@robodoo
Copy link
Contributor

robodoo commented Dec 2, 2022

@zaoral @william-andre because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@zaoral
Copy link
Contributor Author

zaoral commented Dec 2, 2022

ping @william-andre

@jco-odoo
Copy link
Contributor

jco-odoo commented Dec 2, 2022

@robodoo rebase-ff

@robodoo
Copy link
Contributor

robodoo commented Dec 2, 2022

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Dec 2, 2022
In order to make this modules to also match new module l10n_latam_check.

* refactory of some methods to make it more heritable
* check number now is showed only when is defined and is related to
check_printing payment method.

Part-of: #96074
@robodoo robodoo closed this in cbec68b Dec 2, 2022
@robodoo robodoo temporarily deployed to merge December 2, 2022 22:39 Inactive
@zaoral zaoral deleted the master-latam-445-kz branch December 5, 2022 18:25
StevenSermeus pushed a commit to acsone/odoo that referenced this pull request Mar 9, 2023
In order to make this modules to also match new module l10n_latam_check.

* refactory of some methods to make it more heritable
* check number now is showed only when is defined and is related to
check_printing payment method.

Part-of: odoo#96074
StevenSermeus pushed a commit to acsone/odoo that referenced this pull request Mar 9, 2023
New features to manage checks and managed them Odoo in latam countries
where they are widely use.

Own Checks Management
---------------------

Extends 'Check Printing Base' module to manage own checks with more features:

* allow using own checks that are not printed but filled manually by the user
* allow to use deferred or electronic checks
  * printing is disabled
  * check number is set manually by the user
* add an optional "Check Cash-In Date" for post-dated checks (deferred payments)
* add a menu to track own checks

Third Party Checks Management
-----------------------------

Add new "Third party check Management" feature.

There are 2 main Payment Methods additions:

* New Third Party Checks:

   * allow the user create a check on the fly from a payment
   * create a third party check from a customer payment

* Existing Third party check:

   * allow the user to reuse a Third party check already created
   * pay a vendor bill using an existing Third party check
   * move an existing checks between journals (i.e. move to Rejected)
   * Send/Receive again a check already used in a Vendor Bill/Customer INV
   * allow the user to do mass check transfers

closes odoo#96074

Signed-off-by: William André (wan) <wan@odoo.com>
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.

None yet

7 participants