[15.0][IMP] sale_report_delivered: Allow salesman to have access to the sale delivery report in the same way as they have access to the sales report. #297
Conversation
|
Hi @sergio-teruel, |
pilarvargas-tecnativa
changed the title
|
Didn't you need specific record rules? |
pilarvargas-tecnativa
force-pushed
the
15.0-imp-sale_report_delivered-salesman_security
branch
from
bf93745 to
877a423
Compare
Yes, I was already forgetting... Thanks |
|
The |
pilarvargas-tecnativa
force-pushed
the
15.0-imp-sale_report_delivered-salesman_security
branch
2 times, most recently
from
f2bb2cb to
472aaed
Compare
Done! The tests were already saying that this rule was missing. |
| @@ -1,2 +1,3 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
| access_sale_report_delivered_manager,sale.report.delivered,model_sale_report_delivered,sales_team.group_sale_manager,1,1,1,1 | |||
There was a problem hiding this comment.
I'm thinking that you don't need this ACL line, and in fact, you should never grant delete permissions to _auto = False models.
There was a problem hiding this comment.
There was a problem hiding this comment.
Look at this:
Seeing this one, you don't need as well modify not create permissions, just reading ones.
There was a problem hiding this comment.
That's 18.0 though. On 15.0, it' similar to yours (but only with reading):
I would bet that this is not a change in the framework, but a cleaning...
Is it worth trying? You tell me. I don't want to block this.
There was a problem hiding this comment.
In 18.0, we should look at this:
https://github.com/odoo/odoo/blob/18.0/addons/sale/security/ir.model.access.csv#L49
We normally recommend developers to do the following:
- If this is the case in 15.0, it is always advisable to ‘mimic’ the version.
- If it is wrong or can be improved, we can always do a PR to Odoo to change it and then adapt our modules.
If set as is in 18.0 with only this record:
access_sale_report_salesman,sale.report,model_sale_report,sales_team.group_sale_sale_salesman,1,0,0,0,0,0
should work without problems and with one less line, but I understand that we should not make people too confused so that they don't lose the spirit of contribution.
@pilarvargas-tecnativa , if you consider it appropriate, make the change, otherwise it's fine.
There was a problem hiding this comment.
I think it's good to follow the Odoo line but leave the read-only permission for the salesman group. Pushed change
There was a problem hiding this comment.
If it is wrong or can be improved, we can always do a PR to Odoo to change it and then adapt our modules.
That's the question. It's not wrong. It's just that it seems not needed. Odoo is not going to accept that "improvement"/cleaning on stable versions, so if we detect that something can be done, the sooner, the better. If not, when migrating this module to 18, any contributor (being us or any other) won't probably do the change.
but I understand that we should not make people too confused so that they don't lose the spirit of contribution.
This is not a random contributor. This is our team, and thus, we can set higher quality standards between us.
…e delivery report in the same way as they have access to the sales report. TT51606
pilarvargas-tecnativa
force-pushed
the
15.0-imp-sale_report_delivered-salesman_security
branch
from
472aaed to
3d1fbf0
Compare
cc @Tecnativa TT51606
@sergio-teruel @carlosdauden please review