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

[IMP]hr_expense: Allow changes (or not) depending on the stage #32918

Closed

Conversation

jbm-odoo
Copy link
Contributor

Description of the issue/feature this PR addresses:

When an Expense report hits the "Approved" stage, the Accountant
can no longer edit the account / tax / analytic account fields.
However, this information can still be modified from the Expense
itself, impacting directly the related Expense report anyway. In
any case, it makes sense for an accountant to be able to edit
this type of information, as it is likely that an employee or a manager
will input the wrong accounts. On the other hand, once the Journal
entries are posted, the Expense report should not be editable anymore.

Desired behavior after PR is merged:

If the report is in the "Approved" stage
AND the user has at least the Accounting / Accountant access right level

The following fields should be editable
Taxes (tax_ids)
Bill reference (reference)
Account (account_id)
Analytic account (analytic_account_id)

f it is in the "Posted" stage (regardless of the access right level of the user):

The following fields should be in read only:
Taxes (tax_ids)
Account (account_id)
Analytic account (analytic_account_id)

The "Bill reference" (reference) field should still be editable (current behavior).

id=1965890

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

@C3POdoo C3POdoo added the RD research & development, internal work label Apr 24, 2019
@jbm-odoo jbm-odoo force-pushed the master-expense-edit-approved-jbm branch 2 times, most recently from 7ecf96b to 7ccf540 Compare April 24, 2019 13:45
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 24, 2019
@jbm-odoo jbm-odoo force-pushed the master-expense-edit-approved-jbm branch from 7ccf540 to cbebc9c Compare May 2, 2019 07:46
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label May 2, 2019
@jbm-odoo jbm-odoo force-pushed the master-expense-edit-approved-jbm branch from cbebc9c to 00bba4c Compare May 6, 2019 08:44
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label May 6, 2019
@jbm-odoo jbm-odoo force-pushed the master-expense-edit-approved-jbm branch from 00bba4c to bb33a01 Compare May 7, 2019 07:19
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label May 7, 2019
@jbm-odoo jbm-odoo force-pushed the master-expense-edit-approved-jbm branch from bb33a01 to 2199be3 Compare May 7, 2019 08:38
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label May 7, 2019
Copy link
Contributor

@RomainLibert RomainLibert left a comment

Choose a reason for hiding this comment

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

IMO this should use ir.rules instead of groups on some of the fields

@@ -28,6 +28,7 @@
<record id="group_account_user" model="res.groups">
<field name="name">Show Full Accounting Features</field>
<field name="category_id" ref="base.module_category_hidden"/>
<field name="users" eval="[(4, ref('base.user_root')), (4, ref('base.user_admin'))]"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why it is tempting to try and fix the access rights problem you get in your code by giving the group_account_user to user_root and user_admin, but this does not actually fix the problem.

  1. It will change the behaviour of the whole community accounting, which is not what the task is about
  2. It will "fix" the problem on runbot by hiding it under the carpet, but if you create a new user and give it the account_manager right, he will face the access right issue

I agree that the way groups and access rights are handled by the accounting module is a bit weird but we have to work with it being like that.

A proper fix for this issue is to check if the user has either group_account_manager or group_account_user instead of checking only the group_account_user

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed this group should not be granted to a user in the community version ;)

@RomainLibert
Copy link
Contributor

@tivisse This one can probably be merged

@tivisse tivisse force-pushed the master-expense-edit-approved-jbm branch from 2199be3 to 37e4107 Compare June 25, 2019 08:23
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jun 25, 2019
@tivisse tivisse force-pushed the master-expense-edit-approved-jbm branch from 37e4107 to bbcfdb4 Compare June 25, 2019 10:11
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Jun 25, 2019
Purpose
=======

When an Expense report hits the "Approved" stage, the Accountant can no
longer edit the account / tax / analytic account fields. However, this
information can still be modified from the Expense itself, impacting
directly the related Expense report anyway. In any case, it makes sense
for an accountant to be able to edit this type of information, as it is
likely that an employee or a manager will input the wrong accounts.

On the other hand, once the Journal entries are posted, the Expense
report should not be editable anymore.

Specifications
==============

For the Expense report (hr.expense.sheet model):

If the report is in the "Approved" stage AND the user has at
least the Accounting / Accountant access right level

The following fields should be editable
- Taxes (tax_ids)
- Bill reference (reference)
- Account (account_id)
- Analytic account (analytic_account_id)

If the user has a lower access right level => the fields listed above remain
in read only.

If it is in the "Posted" stage (regardless of the access right level of the user):

The following fields should be in read only:
- Taxes (tax_ids)
- Account (account_id)
- Analytic account (analytic_account_id)

The "Bill reference" (reference) field should still be editable (current behavior).

Based on the Expense report's stage and the access right level of the user,
the fields of the related Expenses should also be editable (or not).
@tivisse
Copy link
Contributor

tivisse commented Jun 25, 2019

@robodoo r+

@robodoo robodoo added r+ 👌 CI 🤖 Robodoo has seen passing statuses labels Jun 25, 2019
robodoo pushed a commit that referenced this pull request Jun 25, 2019
Purpose
=======

When an Expense report hits the "Approved" stage, the Accountant can no
longer edit the account / tax / analytic account fields. However, this
information can still be modified from the Expense itself, impacting
directly the related Expense report anyway. In any case, it makes sense
for an accountant to be able to edit this type of information, as it is
likely that an employee or a manager will input the wrong accounts.

On the other hand, once the Journal entries are posted, the Expense
report should not be editable anymore.

Specifications
==============

For the Expense report (hr.expense.sheet model):

If the report is in the "Approved" stage AND the user has at
least the Accounting / Accountant access right level

The following fields should be editable
- Taxes (tax_ids)
- Bill reference (reference)
- Account (account_id)
- Analytic account (analytic_account_id)

If the user has a lower access right level => the fields listed above remain
in read only.

If it is in the "Posted" stage (regardless of the access right level of the user):

The following fields should be in read only:
- Taxes (tax_ids)
- Account (account_id)
- Analytic account (analytic_account_id)

The "Bill reference" (reference) field should still be editable (current behavior).

Based on the Expense report's stage and the access right level of the user,
the fields of the related Expenses should also be editable (or not).

closes #32918

Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>
@robodoo
Copy link
Contributor

robodoo commented Jun 25, 2019

Merged, thanks!

@robodoo robodoo closed this Jun 25, 2019
@tivisse tivisse deleted the master-expense-edit-approved-jbm branch June 25, 2019 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants