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

[14.0][IMP] account_reconciliation_widget: added option to filter all accont move lines #583

Conversation

ValentinVinagre
Copy link

Added option to filter all accont move lines from the date indicated in 'account_bank_reconciliation_start' field.

…nt move lines from the date indicated in 'account_bank_reconciliation_start' field.
@ValentinVinagre
Copy link
Author

@HaraldPanten

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Functional review. Tested in local environment and runboat.

@HaraldPanten
Copy link

@etobella that might be interesting for you 😉

@pedrobaeza
Copy link
Member

Not sure about this. Ignoring unreconciled entries doesn't seem the best strategy: they will still persist in open entries report, views, etc.

@HaraldPanten
Copy link

Not sure about this. Ignoring unreconciled entries doesn't seem the best strategy: they will still persist in open entries report, views, etc.

Hi @pedrobaeza , the objective of this improvement is only improving a filter in the reconciliations view (the filter already exists, we are not creating any extra feature). The usability is quite similar as it was in V12, for example, when the reconciliation view was in Odoo Community. It's not about "balancing" the ledger, it's just about filtering a view.

There are some situations where users are unable to reconcile account entry lines, such as when accounting is balanced (because payments have been entered manually introducing account moves), but users have not reconciled account move lines in several years.

In this case, the situation arises in which there are hundreds of thousands account move lines with pending reconciliation, so... when users start reconciling properly, this view is impossible to manage.

Do you know what I mean?

@pedrobaeza
Copy link
Member

My comments were just for the use case you are describing.

@pedrobaeza pedrobaeza added this to the 14.0 milestone Aug 31, 2023
@HaraldPanten
Copy link

My comments were just for the use case you are describing.

Yeap, but showing hundreds of thousands of old account move lines makes impossible to start working properly, reconciling the items as users should do (but haven't done). It helps for "tiding the accounting" when you need to start working well.

Not everyone makes "their homework". You know...

As I said, the same feature was in previous versions and it was never a problem.

@ValentinVinagre
Copy link
Author

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

If it was already present on odoo, I don't see it as a problem

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

You are mixing things. The filter in v12 is for payments (blue lines), and that filter is already present in this module. What you pretend is to filter everything, not only blue lines.

@ValentinVinagre
Copy link
Author

ValentinVinagre commented Sep 1, 2023

Sorry bad link in the last comment.
What happened is that when the module was transferred to oca, the date filter criteria was changed (as you can see in the link). In OCA it currently only filters based on the "domain_reconciliation" domain and not both domains as it was in Odoo originally.
So, we recover that functionality while respecting the OCA functionality.

https://github.com/odoo/odoo/blob/f7431b180834a73fe8d3aed290c275cc6f8dfa31/addons/account/models/reconciliation_widget.py#L526-L527

@etobella
Copy link
Member

etobella commented Sep 1, 2023

Well, After reading original code from odoo (v12) and the current code), I see we moved the filter from the total domain to only one part of it:

As you can see, the filter was applied to domain https://github.com/odoo/odoo/blob/12.0/addons/account/models/reconciliation_widget.py#L526-L528

On the following PR #443, the code was moved changing the behavior.

IMO, we should move the code as it was originally and keep part of the PR code: https://github.com/OCA/account-reconcile/pull/443/files#diff-00ca0391f0efd17a399a50f2dca6af6f06a32a2456f722c4f3a01b1edcdb4e91L771-L779

WDYT @pedrobaeza or did you move this code for a reason I am unable to see?

@HaraldPanten
Copy link

HaraldPanten commented Sep 1, 2023

With our changes we keep the current functionality, but we offer the other option (the original one). So... we are not "breaking" anything nor offering "new bad praxis" to the users.

@pedrobaeza
Copy link
Member

That domain is used only for blue lines:

Selección_018

And the same for 13.0:

https://github.com/odoo/odoo/blob/13.0/addons/account/models/reconciliation_widget.py#L72-L81

@etobella
Copy link
Member

etobella commented Sep 2, 2023

@pedrobaeza that's true, but that domain contains an OR https://github.com/OCA/account-reconcile/blob/14.0/account_reconciliation_widget/models/reconciliation_widget.py#L809

On #443, the condition was moved from all the domain to just the first part of that domain (the first condition of the OR). It should affect all of it.

@HaraldPanten
Copy link

HaraldPanten commented Sep 2, 2023

That's it, Enric 👍

@pedrobaeza
Copy link
Member

The intention since the beginning has been to apply to blue lines, although the code in some moments doesn't reflect it, and the fact that Odoo itself is not allowing this "masking" shows that it's a bad idea.

As said, you will hide these reconciliations from the reconciliation widget, but open items in account_financial_report will reflect them, the journal items view filtered by "Non reconciled" will show these elements, etc, so it's tricking only one part of the system.

@HaraldPanten
Copy link

The intention since the beginning has been to apply to blue lines, although the code in some moments doesn't reflect it, and the fact that Odoo itself is not allowing this "masking" shows that it's a bad idea.

As said, you will hide these reconciliations from the reconciliation widget, but open items in account_financial_report will reflect them, the journal items view filtered by "Non reconciled" will show these elements, etc, so it's tricking only one part of the system.

Hi Pedro, sorry but I don't agree. It was not since the beginning. At the beginning it was like a "copy" of the old reconciliation features in previous versions.

On the other hand, these entries will not appear in the General Ledger (account_financial_report) because users can set date filters there (for example from 01/01/2023 to 31/12/2023). Even the Trial Balance is correct. As I said, it's not about "balancing the accounting" but hiding lines in the view.

How would you reconcile more than 300.000 items from 5 years ago that can't be related to other moves? Payments have been entered manually with the correct amounts.

As I said, the account's credit/debit are balanced and they're correct. We are not creating a new feature but restoring an old one.

For me, saying that "Odoo itself is not allowing this" is not a real strong reason. Everyday we're adding new features in OCA that are not allowed in Odoo. Even we restore old features that work better than new ones (done by Odoo). And we (OCA) choose our own way of working that is different from Odoo point of view.

I really don't understand why shouldn't this feature be accepted. Users can choose the way they preffer to work. Hiding all lines (even if they're not blue) is not a must, but an option.

If you don't agree these changes, could you suggest any option in order to satisfy both parts?

THX

@pedrobaeza
Copy link
Member

You will find the same problem on v16 and the rest of the upcoming versions.

The way to deal with this is to create an extra account (in the Spanish CoA, 430099), that is not reconcilable, and load the balances there. They go to the same lines in MIS reports. You move those balances from 430999 to 430000 for those that need to still be reconciled.

@HaraldPanten
Copy link

You will find the same problem on v16 and the rest of the upcoming versions.

The way to deal with this is to create an extra account (in the Spanish CoA, 430099), that is not reconcilable, and load the balances there. They go to the same lines in MIS reports. You move those balances from 430999 to 430000 for those that need to still be reconciled.

Hi Pedro,

We'll apply this improvement in newer versions as well as we've been doing in all the modules migrations we did. By the way, It'll be only needed in V15. In newer versions (16, 17...) I think it'll not be needed, because if we get the idea of the new reconciliation methodology, you know that we've filters in the search bar.

Moving the balance to another account is not a solution. As I told you we can not make this account movements as we mustn't change the debit/credit of the accounts. They're correctly balanced and we must keep them as they are. It's a huge amount of data that can't be managed, even technically.

On the other hand, Enric found some changes in a previous PR. The condition was already there but it was removed when making visible payment/debit orders in reconciliation.

We are restoring the original functionality in a better way than it was before, so I can't understand your point of view.

@pedrobaeza that's true, but that domain contains an OR https://github.com/OCA/account-reconcile/blob/14.0/account_reconciliation_widget/models/reconciliation_widget.py#L809

On #443, the condition was moved from all the domain to just the first part of that domain (the first condition of the OR). It should affect all of it.

Regards.

@pedrobaeza
Copy link
Member

On OCB, this has been forbidden since v11, so it's not a restoration at all. I still don't want to introduce this option in the same module, as existing users (our customers) will use (and abuse) this new option. Please add a new module with this option, and add hooks if you need in this module.

@etobella
Copy link
Member

etobella commented Sep 5, 2023

IMO @pedrobaeza you removed an existent functionality on the PR I marked by moving the code (probably as a side effect)... If we put back the code (not the filter), it should work as usual and keep the functionality implemented on that PR. I don't get the problem in restoring a functionality that existed previously and was removed on a PR.

@HaraldPanten
Copy link

On OCB, this has been forbidden since v11, so it's not a restoration at all. I still don't want to introduce this option in the same module, as existing users (our customers) will use (and abuse) this new option. Please add a new module with this option, and add hooks if you need in this module.

Pedro, I still insist that it'd be an optional feature. It wouldn't be a mandatory option. We put a checkbox in order to filter by all the items. You can decide to use it or not.

We can hide it with a special permission group if you think it would be better. On the other hand, it's "dangerous" to allow unexperienced users managing global configuration 😝.

All we know several options that being given to users could create mass disasters, but we're not removing them.

@pedrobaeza
Copy link
Member

Putting that under an option doesn't make this as something valid to be added to a stable module. I'm very reluctant because this is a total bypass of good business practices, and I have direct cases with this that use the OCA module. Is it too much problem to add a new module account_reconciliation_widget_limit_aml or similar name?

@HaraldPanten
Copy link

Putting that under an option doesn't make this as something valid to be added to a stable module. I'm very reluctant because this is a total bypass of good business practices, and I have direct cases with this that use the OCA module. Is it too much problem to add a new module account_reconciliation_widget_limit_aml or similar name?

OK, we can do that and try to make the inheritance, but we have to know that it would not be blocked for not wasting our time.

THX.

@HaraldPanten
Copy link

Closing

@HaraldPanten HaraldPanten deleted the 14.0-imp-account_reconciliation_widget branch September 5, 2023 16:06
@pedrobaeza
Copy link
Member

Sorry for not being clear enough since the beginning. Adding as an optional module is perfect and I won't block it (I won't install it neither 😛). Or maybe yes, but only in a controlled case where the scope is very clear, not on my existing customers that have access to configuration, and discover later that they used it in an unintended way, and we have to deal with the consequences.

Do you understand my concerns?

@HaraldPanten
Copy link

Sorry for not being clear enough since the beginning. Adding as an optional module is perfect and I won't block it (I won't install it neither 😛). Or maybe yes, but only in a controlled case where the scope is very clear, not on my existing customers that have access to configuration, and discover later that they used it in an unintended way, and we have to deal with the consequences.

Do you understand my concerns?

Yes, it's OK.

This time was disapointing for us, but I know that you're trying to do your best for the Community.

As we say in Spain "it happens in the best families" 😂

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.

5 participants