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][ADD] module account_move_update_analytic #497

Merged

Conversation

remi-filament
Copy link

@remi-filament remi-filament commented Nov 16, 2022

Add new module allowing to update analytic account and tags on posted moves
(Note that although model has changed, this is now allowed in v16)

Moved to this repo from OCA/account-financial-tools#1485

@rafaelbn @fclementic2c I have mode module here as suggested, also I have added the handling of analytic tags.

Let me know your thoughts !

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.

Hello @remi-filament

Tested, it works. Maybe I should test it deeper 😄

Doubts about module name:
account_move_update_analytic vs account_move_analytic_update

And finally a re-flexion trying to realize the best UX for accountants and financial responsible that I have recorded 😃

https://www.loom.com/share/a8c9edbb3bac493084f871e2df33a6a6

What do you think @remi-filament @fclementic2c ?

I'm just asking to know your opinion and thougts in order to get the best UX for the module.

We can of course if we agree contribute not only reviewing.

After that obviusly I will approve and merge!

Thank you @remi-filament for this great module and contribution!!!! ❤️

cc @moduon @Shide (please review tech+UX) MT-1606

Copy link

@Shide Shide left a comment

Choose a reason for hiding this comment

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

Super really great work!

Some things can be tweaked but are minor fixes.
The most important for me is the use of parent_state instead of parent.state and adding the button in the account.view_move_line_tree_grouped view

This is a code review without using the module on runboat

account_move_update_analytic/views/account_move_view.xml Outdated Show resolved Hide resolved
{"analytic_account_id": self.new_analytic_account_id.id or False}
)
if self.user_has_groups("analytic.group_analytic_tags"):
self.line_id.write(

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the or [] but had to keep the [(6,0,xx)] otherwise it does not empty the field if you remove analytic tags

<field name="company_id" invisible="1" />
<field
name="new_analytic_tag_ids"
domain="['|', ('company_id', '=', False), ('company_id', '=', company_id)]"
Copy link

Choose a reason for hiding this comment

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

Same as above

Copy link

@Shide Shide left a comment

Choose a reason for hiding this comment

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

The missing view in view_move_line_tree_grouped

@remi-filament
Copy link
Author

Hi @rafaelbn sorry for the delay in coming back to you !
Very busy weeks over here, thank you for taking time to review it and even record a video !
I get your point about having the updates made by accountant in general ledger or accounting menus, however I have a different view because :

  1. With French regulations, it is not allowed to edit an invoice once it has been posted (you can only refund and create a new one), therefore in Odoo when configuring FR company, you cannot edit account_move_lines once posted, nor can you bring an invoice back to draft
  2. My customers are not accountant, but rather want to go back to their invoice to correct analytic account when they get discrepancy analyzing margins on projects

I guess we can still add button on account_move_line tree views.
Also I did not manage to make the column hideable but I should look again, maybe it is only because I forgot to give a name to column...

Thanks !

@remi-filament remi-filament force-pushed the 14.0-add-account_move_update_analytic branch from 4a844fe to aa61459 Compare December 1, 2022 15:11
@remi-filament
Copy link
Author

Thanks a lot @Shide for your comments, I updated accordingly !
I made a separate commit to that you can see the additions, however I can squash these into one if OK for you

@rafaelbn as proposed by @Shide I added the button in view_move_line_tree_grouped as well which was if I understood correctly what you were asking for !

Also I did not manage to use show/hide to hide the column with the button, nor adding a title to that column, I am not sure it is even feasible ?

Let me know
Best Regards

@rafaelbn
Copy link
Member

rafaelbn commented Dec 2, 2022

Thank you @remi-filament !

I found a BUG, check (2 min): https://www.loom.com/share/a274be3a321a4ae3986ee705cf7e94ea

@rafaelbn
Copy link
Member

rafaelbn commented Dec 2, 2022

With French regulations, it is not allowed to edit an invoice once it has been posted (you can only refund and create a new one), therefore in Odoo when configuring FR company, you cannot edit account_move_lines once posted, nor can you bring an invoice back to draft

@remi-filament , Spain is similar, I mean, and invoice is not accounting even Odoo made same model. Change an account is not change an invoice, invoicing means more amount and taxes, accounting go in other hand. Futhermore, the change is not allowed for full invoice just for income or outcome (expenses).

Accounting Reclassification si common.

For example, when we invoice and send it in real time to administration you are not sending accounts, just amount base, tax, vat, types... the accounting is later, usually once a year.

I'm working for multi-nationals companies with monthly closing accounting, they are really strict, but accountants sometimes fails and they have to reclassify accounting before closing (and should be really fast), and this is legal if you don't change taxes or amounts that you previously have sent.

Do you see the differences?

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.

Please check this. I found a BUG, check (2 min): https://www.loom.com/share/a274be3a321a4ae3986ee705cf7e94ea

@remi-filament
Copy link
Author

Hi @rafaelbn
Thanks for reporting the issue, of course we should not break anything.
I can reproduce on runboat but not on my local instance with only this module installed from account-analytic OCA repo.
Also, I am not updating account_move_line model code, so I would not understand how my module could imply the bug you see.
So I suppose the issue comes from some other module from that repo...

I went quickly over the code of all modules in this repo but cannot find the culprit...

Also, regarding your other point, I am not used to update these fields as an accountant but I guess the addition of the button on account move line grouped tree view is what you expected ?
FYI, in France with hash enabled on customer invoices, you cannot update even account once the invoice is posted (you get the following error : You cannot edit the following fields due to restrict mode being activated on the journal: debit, credit, account_id, partner_id.), which is why I never was allowed to do it !

@remi-filament
Copy link
Author

I managed to find the culprit I think, when uninstalling account_analytic_tag_default the expected behavior is back.
Probably because of the compute of account_analytic_tag_ids here :

def _compute_analytic_tag_ids(self):

@rafaelbn
Copy link
Member

rafaelbn commented Dec 9, 2022

Hello @remi-filament ,

Also, regarding your other point, I am not used to update these fields as an accountant but I guess the addition of the button on account move line grouped tree view is what you expected ?

No by the moment.

On monday we will return to this PR to get it reviewed and merge next week 👍

Thank you @remi-filament !! 🙏

@Shide
Copy link

Shide commented Dec 12, 2022

Thanks a lot @Shide for your comments, I updated accordingly ! I made a separate commit to that you can see the additions, however I can squash these into one if OK for you

@rafaelbn as proposed by @Shide I added the button in view_move_line_tree_grouped as well which was if I understood correctly what you were asking for !

Also I did not manage to use show/hide to hide the column with the button, nor adding a title to that column, I am not sure it is even feasible ?

Let me know Best Regards

I think it's not possible right now.

@Shide
Copy link

Shide commented Dec 12, 2022

@remi-filament I have one question:

Why not make the field not-readonly and allow the user edit directly from the tree lines?
Is there any limitation that forces us to show a wizard? Because the wizard only allows to modify one line at a time.

The multi selection + inline tree editing allows to edit all the lines with the same analytic accounts at once.

@remi-filament
Copy link
Author

Hi @Shide thanks for looking again into it !

Why not make the field not-readonly and allow the user edit directly from the tree lines?
Is there any limitation that forces us to show a wizard? Because the wizard only allows to modify one line at a time.

Actually, the problem is not only updating analytic account or tags on account_move_lines, this could indeed be done as you suggest, however you need to update / delete / create account_analytic_lines and/or account_analytic_tags which are related to these account_move_lines.
We could still have a function with a depends that recreate lines if account_move_line is updated but I think it is better to get a wizard (+ you also have a second verification with the wizard, since you need to click on 2 buttons instead of only editing and Save).

I agree though that we could go further and make a mass edit wizard that would update analytic account and analytic tags on multiple lines at once, but this was not my initial use case, and I like to know that I am changing from this account to this new account which I think is more difficult to get if you change multiple lines at once, so that would need more thinking about UX !

@Shide
Copy link

Shide commented Dec 14, 2022

Hi @Shide thanks for looking again into it !

Why not make the field not-readonly and allow the user edit directly from the tree lines?
Is there any limitation that forces us to show a wizard? Because the wizard only allows to modify one line at a time.

Actually, the problem is not only updating analytic account or tags on account_move_lines, this could indeed be done as you suggest, however you need to update / delete / create account_analytic_lines and/or account_analytic_tags which are related to these account_move_lines. We could still have a function with a depends that recreate lines if account_move_line is updated but I think it is better to get a wizard (+ you also have a second verification with the wizard, since you need to click on 2 buttons instead of only editing and Save).

I agree though that we could go further and make a mass edit wizard that would update analytic account and analytic tags on multiple lines at once, but this was not my initial use case, and I like to know that I am changing from this account to this new account which I think is more difficult to get if you change multiple lines at once, so that would need more thinking about UX !

Ok, then It's fine for me 😄

Copy link

@Shide Shide left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Thanks!🙂 👍

@fclementic2c we are going to migrate to v15 if you would like to test it for any improvement, you are really welcome

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-497-by-rafaelbn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5a7515f into OCA:14.0 Dec 14, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b503816. 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.

4 participants