[16.0][IMP] [account_statement_base account_reconcile_oca] make statements listed and editable #637

lk-eska · 2024-03-11T09:14:16Z

Currently there is no way to list or edit bank statement lines. this PR brings this feature to account_statement_base account_reconcile_oca modules.

OCA-git-bot · 2024-03-11T09:14:22Z

Hi @alexis-via, @etobella,
some modules you are maintaining are being modified, check this out!

etobella

Just a small comment and we can proceed

etobella · 2024-03-11T13:49:00Z

account_reconcile_oca/views/account_bank_statement_line.xml

@@ -325,11 +326,12 @@
        <field
            name="context"
        >{'default_journal_id': active_id, 'view_ref': 'account_reconcile_oca.bank_statement_line_form_reconcile_view'}</field>
-        <field name="view_mode">tree</field>
+        <field name="view_mode">tree,kanban</field>


Suggested change

<field name="view_mode">tree,kanban</field>

<field name="view_mode">kanban,tree</field>

etobella

The order is important, as the default loaded view is tree now

etobella · 2024-03-11T13:50:35Z

account_reconcile_oca/views/account_bank_statement_line.xml

        <field
            name="view_ids"
            eval="[(5, 0, 0),
-            (0, 0, {'view_mode': 'tree', 'view_id': ref('bank_statement_line_reconcile_view')})]"
+            (0, 0, {'view_mode': 'tree', 'view_id': ref('account_statement_base.account_bank_statement_line_tree')}),


Change the order, first the kanban please 😉

etobella · 2024-03-11T13:50:41Z

account_reconcile_oca/views/account_bank_statement_line.xml

        <field
            name="view_ids"
            eval="[(5, 0, 0),
-            (0, 0, {'view_mode': 'tree', 'view_id': ref('bank_statement_line_reconcile_view')})]"
+            (0, 0, {'view_mode': 'tree', 'view_id': ref('account_statement_base.account_bank_statement_line_tree')}),


Change the order, first the kanban please 😉

I just sent a fix.

pedrobaeza · 2024-03-11T14:14:18Z

I haven't checked yet, but statement lines shouldn't be editable once the statement is published (just in case this is favoring other behavior).

etobella · 2024-03-11T14:18:00Z

@pedrobaeza I had the same concern, but I tested it on runboat and an error was raised when editing reconciled moves 😃

pedrobaeza · 2024-03-11T14:20:14Z

Then isn't better to not allow to edit such reconciled entries?

lk-eska · 2024-03-12T03:09:03Z

Odoo doesn't allow you to change reconciled entries so there is no such concern.

The real problem is not having edit/delete capability on the statements. How to change or delete them if you imported wrong? How to set foreign currency and foreign currency amount? Is it possible to do it somewhere else?

pedrobaeza · 2024-03-12T07:14:40Z

Odoo doesn't allow you to change reconciled entries so there is no such concern.

What I mean is UX. According to what I'm understanding, the interface will let you change data in reconciled lines, but at the saving time, it will rise an error. What I'm talking about is to directly avoid to change such data for the reconciled entries.

lk-eska · 2024-03-12T10:06:53Z

How do we allow editing some of the lines conditionally? (on tree editable mode) We can improve UX if this is possible. It works on enterprise similarly by the way.

etobella · 2024-03-12T10:20:50Z

In 16 you should do: attrs="{'readonly': [('is_reconciled', '=', True)]}" on the view for each field

lk-eska · 2024-03-14T07:18:38Z

In 16 you should do: attrs="{'readonly': [('is_reconciled', '=', True)]}" on the view for each field

I added this to this PR.

etobella · 2024-03-14T07:33:40Z

Can you squash your commits and a use a proper commit message?

Thanks 😉

lk-eska · 2024-03-14T07:38:52Z

There should be "Squash & Merge" option for accepting a PR?

pedrobaeza · 2024-03-14T07:40:42Z

No, the bot can't squash, so it should be done manually.

etobella · 2024-03-14T15:52:16Z

I checked and with this new tree view, we can unlink the items. It is right that we can unlink lines that are not reconciled, but with this we were able to unlink reconciled lines. Can you add a validation on the unlink in order avoid this?

HaraldPanten · 2024-03-14T15:57:47Z

I checked and with this new tree view, we can unlink the items. It is right that we can unlink lines that are not reconciled, but with this we were able to unlink reconciled lines. Can you add a validation on the unlink in order avoid this?

Hi @lk-eska @pedrobaeza I think that it should be fixed before merging this PR. Deleting reconciled items in the tree view should be forbidden. If you want, we can manage this PR keeping you as author.

Let us know, please.

lk-eska · 2024-03-14T18:30:16Z

No, the bot can't squash, so it should be done manually.

I squashed them into one commit.

lk-eska · 2024-03-14T18:34:13Z

I checked and with this new tree view, we can unlink the items. It is right that we can unlink lines that are not reconciled, but with this we were able to unlink reconciled lines. Can you add a validation on the unlink in order avoid this?

Hi @lk-eska @pedrobaeza I think that it should be fixed before merging this PR. Deleting reconciled items in the tree view should be forbidden. If you want, we can manage this PR keeping you as author.

Let us know, please.

Deleting a statement line (unless it is reconciled with an invoice) is standard behavior in enterprise. This is not a bug but a feature and all enterprise users live with it without complaining. I think we are getting beyond the scope of this pull request. Not that I care about authorship but if you think this is required you can create your own PR fixing it once this gets merged.

etobella · 2024-03-14T18:35:36Z

@lk-eska It is already nice to delete not reconciled lines, but right now,we are able to delete reconciled lines.

lk-eska · 2024-03-14T18:39:44Z

I can't do it in runboat if it is reconciled with an invoice:

It can be deleted if it is reconciled manually and it makes sense.

etobella · 2024-03-14T18:47:23Z

But only with reconciled items with invoices

You can see here what happens... Obviously, this is not right

movie.mp4

lk-eska · 2024-03-14T18:53:16Z

But only with reconciled items with invoices

You can see here what happens... Obviously, this is not right
movie.mp4

It is deleting the generated account move so I see no problem here. Why bother unreconciling it when you can skip that step?

etobella · 2024-03-14T19:09:42Z

I understand that a reconciled move should never be deleted, as it is posted.

So, IMO it is not a desired behavior.

HaraldPanten · 2024-03-14T19:30:57Z

What about payments related to reconciled account moves like manual ones? For example like the ones that we create in the initial balance, or the ones coming from payrolls? Is that a desired behaviour?

lk-eska · 2024-03-14T20:15:36Z

What about payments related to reconciled account moves like manual ones? For example like the ones that we create in the initial balance, or the ones coming from payrolls? Is that a desired behaviour?

I haven't tested all possible use cases but I assume it allows deletion if it is not reconciled with another move. It should cover most of your questions. IMO, deleting is not a big deal if it is a standalone entry.

HaraldPanten · 2024-03-15T10:42:11Z

What about payments related to reconciled account moves like manual ones? For example like the ones that we create in the initial balance, or the ones coming from payrolls? Is that a desired behaviour?

I haven't tested all possible use cases but I assume it allows deletion if it is not reconciled with another move. It should cover most of your questions. IMO, deleting is not a big deal if it is a standalone entry.

Hi, I'm starting to think that you're right. I've tested V15 and the functionality is the same as here (I think). In V15 you can delete reconciled bank statement lines as well.

pedrobaeza · 2024-03-15T10:46:11Z

In V15 you can delete reconciled bank statement lines as well.

AFAIK, that's not possible. What are the steps you do for that?

lk-eska · 2024-03-15T10:57:56Z

I think there is a confusion when we say reconciled. Let me clarify:

reconciled with another entry such as invoice (deletion is not possible)
directly creating a new entry such as manual expense entry (deletion is possible)

HaraldPanten · 2024-03-15T10:58:49Z

These are my steps in V15:

Create a bank statement with a bank statement line of -15$.
Validate the statement.
Go to the reconciliation view.
Make a manual operation an send these -15$ to any expenses account.
Go to the operations view (see screenshot).

Select the reconciled line in the tree view and try to delete it from the tree view.

Users can delete it without any restriction.

@pedrobaeza @etobella I think it's not correct, but it seems it has been working like that for several years. WDYT?

HaraldPanten · 2024-03-15T11:03:49Z

I think there is a confusion when we say reconciled. Let me clarify:

reconciled with another entry such as invoice (deletion is not possible)

directly creating a new entry such as manual expense entry (deletion is possible)

Yep. That's it.

HaraldPanten

Manual operations can be deleted, but reconciled statement lines (with invoices or other payable/receivable accounts like customers/vendors in the starting balance) can't be deleted once they're reconciled.

IMHO, this pull request is functionally correct. LGTM.

pedrobaeza · 2024-03-15T11:23:33Z

OK, analyzing it, it seems congruent with the new v16 approach:

Statement lines can be created without statement.
You can add a new statement line in any moment, so deleting the line it's not limiting you forcing to create a new statement.

Anyway, I think we should add a protection over lines belonging to an statement, as if you remove a line, your balances won't fit and you won't be able to add the line in the same statement. What do you think? I'm thinking in statements imported from banks, where the transaction list should be unalterable at that level.

HaraldPanten · 2024-03-15T11:26:08Z

OK, analyzing it, it seems congruent with the new v16 approach:

Statement lines can be created without statement.

You can add a new statement line in any moment, so deleting the line it's not limiting you forcing to create a new statement.

Anyway, I think we should add a protection over lines belonging to an statement, as if you remove a line, your balances won't fit and you won't be able to add the line in the same statement. What do you think? I'm thinking in statements imported from banks, where the transaction list should be unalterable at that level.

I agree, but shouldn't we work in this new IMP/FIX in a new PR?

lk-eska · 2024-03-15T13:15:37Z

Can we merge this now?

Make listed and editable

Make them listed and editable

etobella · 2024-03-15T23:02:27Z

Yes, I added a proper commit message and splitted them between both modules (Sorry for making you merge them, because they were two modules and I didn't notice that)

For the next time, remember to use proper commits message https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#L1114-L1167

As you can see, I kept your authorship and no modification on the code

If it is ok for you the applied changes, I can proceed with merge

HaraldPanten · 2024-03-15T23:40:11Z

But I still see 2 different commits. Is that correct, @etobella ?

etobella · 2024-03-16T00:11:53Z

Yes, two modules are modified.

lk-eska · 2024-03-16T23:34:03Z

No problem @etobella ! You can merge it.

etobella

/ocabot merge patch

OCA-git-bot · 2024-03-16T23:35:29Z

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

OCA-git-bot · 2024-03-16T23:36:08Z

@etobella The merge process could not start, because command git fetch --quiet --force --prune https://github.com/OCA/account-reconcile 'refs/heads/*:refs/heads/*' failed with output:

fatal: unable to access 'https://github.com/OCA/account-reconcile/': gnutls_handshake() failed: The TLS connection was non-properly terminated.

etobella · 2024-03-17T00:09:48Z

Trying again

/ocabot merge patch

OCA-git-bot · 2024-03-17T00:10:48Z

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-637-by-etobella-bump-patch, awaiting test results.

OCA-git-bot · 2024-03-17T00:15:11Z

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

This was referenced Mar 11, 2024

[16.0] account_reconcile_oca currency exchange valuation handling issues #622

Closed

[16.0] Reconcile Button is Missing #615

Closed

etobella reviewed Mar 11, 2024

View reviewed changes

lk-eska force-pushed the 16.0-edit-statement branch from b346fde to 7de1c37 Compare March 14, 2024 18:17

HaraldPanten approved these changes Mar 15, 2024

View reviewed changes

pedrobaeza added this to the 16.0 milestone Mar 15, 2024

lk-eska added 2 commits March 15, 2024 23:58

[IMP] account_reconile_oca: Set tree views

74d4b47

Make listed and editable

[IMP] account_statement_base: Set tree views

df5a929

Make them listed and editable

etobella force-pushed the 16.0-edit-statement branch from 7de1c37 to df5a929 Compare March 15, 2024 22:58

etobella approved these changes Mar 16, 2024

View reviewed changes

OCA-git-bot added the approved label Mar 16, 2024

OCA-git-bot added the ready to merge label Mar 16, 2024

OCA-git-bot added the bot is merging ⏳ label Mar 17, 2024

OCA-git-bot merged commit 50c037b into OCA:16.0 Mar 17, 2024
7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Mar 17, 2024

	<field name="view_mode">tree,kanban</field>
	<field name="view_mode">kanban,tree</field>

[16.0][IMP] [account_statement_base account_reconcile_oca] make statements listed and editable #637

[16.0][IMP] [account_statement_base account_reconcile_oca] make statements listed and editable #637

Conversation

lk-eska commented Mar 11, 2024

OCA-git-bot commented Mar 11, 2024

etobella left a comment

Choose a reason for hiding this comment

etobella Mar 11, 2024

Choose a reason for hiding this comment

etobella left a comment

Choose a reason for hiding this comment

etobella Mar 11, 2024

Choose a reason for hiding this comment

etobella Mar 11, 2024

Choose a reason for hiding this comment

lk-eska Mar 11, 2024

Choose a reason for hiding this comment

pedrobaeza commented Mar 11, 2024

etobella commented Mar 11, 2024

pedrobaeza commented Mar 11, 2024

lk-eska commented Mar 12, 2024

pedrobaeza commented Mar 12, 2024

lk-eska commented Mar 12, 2024

etobella commented Mar 12, 2024 • edited Loading

lk-eska commented Mar 14, 2024

etobella commented Mar 14, 2024

lk-eska commented Mar 14, 2024

pedrobaeza commented Mar 14, 2024

etobella commented Mar 14, 2024 • edited Loading

HaraldPanten commented Mar 14, 2024

lk-eska commented Mar 14, 2024

lk-eska commented Mar 14, 2024

etobella commented Mar 14, 2024

lk-eska commented Mar 14, 2024 • edited Loading

etobella commented Mar 14, 2024

lk-eska commented Mar 14, 2024

etobella commented Mar 14, 2024

HaraldPanten commented Mar 14, 2024

lk-eska commented Mar 14, 2024

HaraldPanten commented Mar 15, 2024

pedrobaeza commented Mar 15, 2024

lk-eska commented Mar 15, 2024

HaraldPanten commented Mar 15, 2024 • edited Loading

HaraldPanten commented Mar 15, 2024

HaraldPanten left a comment • edited Loading

Choose a reason for hiding this comment

pedrobaeza commented Mar 15, 2024

HaraldPanten commented Mar 15, 2024

lk-eska commented Mar 15, 2024

etobella commented Mar 15, 2024

HaraldPanten commented Mar 15, 2024

etobella commented Mar 16, 2024

lk-eska commented Mar 16, 2024 • edited Loading

etobella left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Mar 16, 2024

OCA-git-bot commented Mar 16, 2024

etobella commented Mar 17, 2024

OCA-git-bot commented Mar 17, 2024

OCA-git-bot commented Mar 17, 2024

etobella commented Mar 12, 2024 •

edited

Loading

etobella commented Mar 14, 2024 •

edited

Loading

lk-eska commented Mar 14, 2024 •

edited

Loading

HaraldPanten commented Mar 15, 2024 •

edited

Loading

HaraldPanten left a comment •

edited

Loading

lk-eska commented Mar 16, 2024 •

edited

Loading