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

[13.0] Create module account_financial_discount #1040

Conversation

grindtildeath
Copy link
Contributor

No description provided.

@pedrobaeza
Copy link
Member

@grindtildeath
Copy link
Contributor Author

@pedrobaeza

  1. this is still a draft
  2. the module you mention doesn't provide the same functionalities ;)

@pedrobaeza
Copy link
Member

Well, it's more or less the same, but yes, it's not the same, but @sbidoul can say about an exact replacement of this module that they develop.

@grindtildeath
Copy link
Contributor Author

@pedrobaeza If his module hasn't been submitted in the OCA yet, what's the point of recommending it here? Sorry but I don't understand your logic here...
Anyway you're still welcome to review and propose improvements on this PR :)

@pedrobaeza
Copy link
Member

Well, I don't look for anything outside OCA, but my comment was from memory for avoiding your extra work if exists. I have looked, and the module I was mentioning is: https://odoo-community.org/shop/product/account-cash-discount-base-4612

You also have from your company OCA/account-payment#231

@sbidoul
Copy link
Member

sbidoul commented Aug 11, 2020

Yes, there is this suite of cash discount modules in v10. It works well and can handle different VAT scenarios.
I think we'll port them to v13. I'll check with my colleagues.

@grindtildeath
Copy link
Contributor Author

grindtildeath commented Aug 11, 2020

Well I just gave a look to account_cash_discount_base and depending modules in account-payment v10 and must say it is close to what we did here.
But with the all the changes between 10.0 and 13.0 on the reconciliation part, IMO we should try to merge these modules together, if not for v13.0 at least for v14.0.

But account_early_payment_discount is also missing a lot of features on this and that's why we developed this new module.

fyi @fclementic2c

@sbidoul
Copy link
Member

sbidoul commented Aug 11, 2020

cc/ @Cedric-Pigeon @benwillig @chrisdec

@sbidoul
Copy link
Member

sbidoul commented Sep 30, 2020

cross-referencing the PR for the cash_discount modules v13: OCA/account-payment#334

@grindtildeath grindtildeath force-pushed the 13.0-add-account_financial_discount branch from 94cc57a to 55d8f93 Compare October 29, 2021 14:27
@grindtildeath grindtildeath force-pushed the 13.0-add-account_financial_discount branch from 55d8f93 to 006ec84 Compare October 29, 2021 14:32
@grindtildeath grindtildeath marked this pull request as ready for review October 29, 2021 14:33
@pedrobaeza
Copy link
Member

AFAIK, Acsone was doing something very similar. @sbidoul can you post the link to your work?

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2021

@grindtildeath
Copy link
Contributor Author

@pedrobaeza @sbidoul We considered account_cash_discount modules, however, the issue for us is to rely on account_payment_order whereas this module allows reconciliation from the bank statements.

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2021

@grindtildeath account_cash_discount_base does not depend on account_payment_order. It provides the base data on invoices, and I think it should be possible to build the bank statement reconciliation part on top of it ? account_cash_discount_payment builds on it to facilitate supplier payment with cash discount.

@grindtildeath
Copy link
Contributor Author

@sbidoul That's true but the design choices are quite different in terms of storing discount values. We could consider trying to merge modules together but IMO it would require changes on both sides (i.e. this module but also the other)

Then, there is also a third module doing the same thing in OCA but with a different scope: https://github.com/OCA/account-payment/tree/14.0/account_payment_term_discount

So I don't see why we shouldn't have the different alternatives available on existing versions. I guess we could try to converge all the modules together (maybe for v15.0), but it would require some time to do it properly so that it answers everyone's needs... Maybe we can start by opening an issue and try to involve all the contributors?

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2021

So I don't see why we shouldn't have the different alternatives available on existing versions.

Sure, I'm not blocking anything :)

I guess we could try to converge all the modules together (maybe for v15.0), but it would require some time to do it properly so that it answers everyone's needs... Maybe we can start by opening an issue and try to involve all the contributors?

Good idea. Will you do it as it seems you have done some comparisons already ?

@grindtildeath
Copy link
Contributor Author

FTR: OCA/account-payment#462

"name": tax_line.name,
"account_id": tax_line.account_id.id,
"debit": tax_line.credit and line.amount_discount_tax or 0,
"credit": tax_line.debit and line.amount_discount_tax or 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"credit": tax_line.debit and line.amount_discount_tax or 0,
"credit": tax_line.debit and -line.amount_discount_tax or 0,

While working on mig to v14.0 I'm wondering if we shouldn't have this here (for vendor bills).

}
# Deduce tax amount from fin. disc. write-off
if fin_disc_write_off_vals.get("credit"):
fin_disc_write_off_vals["credit"] -= line.amount_discount_tax
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
fin_disc_write_off_vals["credit"] -= line.amount_discount_tax
fin_disc_write_off_vals["credit"] += line.amount_discount_tax

While working on mig to v14.0 I'm wondering if we shouldn't have this here (for vendor bills)

fix: invoice_date in priority for discount date
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 25, 2023
@github-actions github-actions bot closed this Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants