[14.0][ADD] partner_risk_insurance_security #403

toita86 · 2024-10-22T10:39:02Z

Currently, anyone can view and edit the Credit Insurance tab added by the module partner_risk_insurance.

Implementation: Created a group to restrict access to the fields added by the aforementioned module.

OCA-git-bot · 2024-10-22T10:39:06Z

Hi @Daniel-CA, @sergio-teruel, @omar7r, @Tardo,
some modules you are maintaining are being modified, check this out!

pedrobaeza · 2024-10-22T11:56:01Z

This must be an extra module, as it's modifying current behavior heavily. The reason to not protect it is because not everyone can edit contacts, and those who can, needs a lot of times to touch these amounts (they are salesmen providing the information).

pedrobaeza · 2024-10-22T14:09:25Z

A usual suffix is _security for this kind of things.

toita86 · 2024-10-22T14:15:18Z

A usual suffix is _security for this kind of things.

Thanks for the info!

francesco-ooops

Functional ok!

aleuffre · 2024-11-06T10:59:11Z

partner_risk_insurance_security/models/res_partner.py

+        store=False,
+    )
+
+    @api.depends("user_id")


issue: The field is not stored, so you don't even need to have this @api.depends at all, since it'll be recomputed every time anyway.

You can have the @api.depends if you want the field to be recomputed based on specific conditions, in addition to it being recomputed almost every time you look at the record, though.

In this case, you're asking Odoo to recompute the field based on the field user_id of res.partner, i.e. Salesman. In other words, when you change "Salesman" on a contact, recompute the value of this field.

I think what you're looking for is @api.depends_context('uid')

aleuffre · 2024-11-06T11:00:22Z

partner_risk_insurance_security/models/res_partner.py

+            partner.is_full_access_user = (
+                self.env.user
+                in self.env.ref(
+                    "partner_risk_insurance_security.group_full_access_credit_policy_state"
+                ).users
+            )


suggestion: I think self.env.user.has_group("partner_risk_insurance_security.group_full_access_credit_policy_state") is simpler to write and read, and probably also slightly better in terms of performance.

aleuffre · 2024-11-06T11:03:09Z

partner_risk_insurance_security/models/res_partner.py

+
+    @api.depends("user_id")
+    def _compute_is_full_access_user(self):
+        for partner in self:


issue: You're not using any field of the partner. You don't need to recompute the value of the field for every partner, you can compute it once and then assign it to all.

Even better, you can assign it all at once for every partner:

self.is_full_access_user = ....

aleuffre · 2024-11-06T11:05:01Z

partner_risk_insurance_security/views/res_partner_view.xml

+        <field name="inherit_id" ref="base.view_partner_form" />
+        <field name="priority" eval="50" />


issue: The fields you're modifying do not exist in base.view_partner_form. I think you want to inherit from partner_risk_insurance.view_partner_form.

suggestion You can also drop the priority IMO, if you don't need it for anything in particular.

Sorry, I realize it was ambiguous. By "drop" I meant "remove" the line where you specify the priority entirely.

Anyway, non-blocking.

aleuffre · 2024-11-06T11:05:20Z

partner_risk_insurance_security/views/res_partner_view.xml

@@ -0,0 +1,66 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<odoo>
+    <!--Add risk insurance tab in partner form. -->


chore: leftover comment from copy-pasting.

aleuffre · 2024-11-06T11:06:51Z

partner_risk_insurance_security/views/res_partner_view.xml

+           <xpath expr="//field[@name='company_credit_limit']" position="attributes">
+            <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath expr="//field[@name='insurance_credit_limit']" position="attributes">
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath
+                expr="//field[@name='risk_insurance_requested']"
+                position="attributes"
+            >
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath
+                expr="//field[@name='risk_insurance_grant_date']"
+                position="attributes"
+            >
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath
+                expr="//field[@name='risk_insurance_coverage_percent']"
+                position="attributes"
+            >
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath expr="//field[@name='risk_insurance_code']" position="attributes">
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath expr="//field[@name='risk_insurance_code_2']" position="attributes">
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath expr="//field[@name='credit_policy_state_id']" position="attributes">
+                <attribute
+                    name="attrs"
+                >{'readonly': [('is_full_access_user', '=', False)]}</attribute>
+            </xpath>
+            <xpath expr="//field[@name='credit_policy_state_id']" position="after">
+                <field name="is_full_access_user" invisible="1" />
+            </xpath>
+        </field>


question: can you add readonly directly on the <page name="credit_insurance position="attributes"> element instead of adding it on each field inside it?

I have tried to do

<xpath expr="//page[@name='credit_insurance']" position="attributes"> <attribute name="attrs">{'readonly': [('is_full_access_user', '=', False)]}</attribute> </xpath>

but it didn't work. I don't think that is possible to use the readonly attribute on a page

Fair enough, thanks for checking!

aleuffre · 2024-11-06T11:09:33Z

partner_risk_insurance_security/security/ir.model.access.csv

+id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
+access_full_access_credit_policy_state,res.partner,base.model_res_partner,partner_risk_insurance_security.group_full_access_credit_policy_state,1,1,1,1


issue: access to res.partner is already managed by other security groups, and is not strictly needed for this module to work. I'm not sure this specific groups needs to grant full CRUD access to res.partner.

My instinct would be to remove this file entirely.

aleuffre · 2024-11-06T11:13:56Z

Also, not strictly needed for such a simple module IMO, but always nice to add a test, IMO.

aleuffre

Code review, LGTM

aleuffre · 2024-11-08T11:05:08Z

partner_risk_insurance_security/tests/test_partner_risk_insurance_security.py

+                        0,
+                        [
+                            self.env.ref(
+                                "partner_risk_insurance_security.group_full_access_credit_policy_state"  # noqa: B950


suggestion(non-blocking):

Suggested change

"partner_risk_insurance_security.group_full_access_credit_policy_state" # noqa: B950

"partner_risk_insurance_security."

"group_full_access_credit_policy_state"

Thank you for the suggestion!

carlosdauden

Code LGTM

Let's wait a bit before merging to see if the suggestions are heeded.

OCA-git-bot · 2024-11-08T11:10:01Z

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

carlosdauden · 2024-11-08T11:56:15Z

Thanks, ready to merge

pedrobaeza · 2024-11-08T11:57:09Z

/ocabot merge nobump

OCA-git-bot · 2024-11-08T11:57:16Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-403-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2024-11-08T12:02:30Z

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

toita86 force-pushed the 14.0-IMP-partner_risk_insurance branch 3 times, most recently from a3f9d30 to b6084af Compare October 22, 2024 11:51

toita86 force-pushed the 14.0-IMP-partner_risk_insurance branch 3 times, most recently from f710520 to 2df0fbd Compare October 22, 2024 14:03

toita86 changed the title ~~[IMP] partner_risk_insurance: Added group for field access~~ [ADD] partner_risk_insurance_restriction Oct 22, 2024

toita86 changed the title ~~[ADD] partner_risk_insurance_restriction~~ [14.0][ADD] partner_risk_insurance_restriction Oct 22, 2024

toita86 changed the title ~~[14.0][ADD] partner_risk_insurance_restriction~~ [14.0][ADD] partner_risk_insurance_security Oct 22, 2024

toita86 force-pushed the 14.0-IMP-partner_risk_insurance branch 5 times, most recently from 024076f to 7a77449 Compare October 29, 2024 08:34

francesco-ooops approved these changes Oct 29, 2024

View reviewed changes

aleuffre suggested changes Nov 6, 2024

View reviewed changes

toita86 force-pushed the 14.0-IMP-partner_risk_insurance branch 2 times, most recently from c0fd637 to db00a4b Compare November 7, 2024 17:57

aleuffre approved these changes Nov 8, 2024

View reviewed changes

OCA-git-bot added the approved label Nov 8, 2024

carlosdauden approved these changes Nov 8, 2024

View reviewed changes

OCA-git-bot added the ready to merge label Nov 8, 2024

[ADD] partner_risk_insurance_security

5ad4e88

toita86 force-pushed the 14.0-IMP-partner_risk_insurance branch from db00a4b to 5ad4e88 Compare November 8, 2024 11:27

OCA-git-bot added the bot is merging ⏳ label Nov 8, 2024

OCA-git-bot merged commit e762825 into OCA:14.0 Nov 8, 2024
7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[14.0][ADD] partner_risk_insurance_security #403

[14.0][ADD] partner_risk_insurance_security #403

toita86 commented Oct 22, 2024 •

edited

Loading

OCA-git-bot commented Oct 22, 2024

pedrobaeza commented Oct 22, 2024

pedrobaeza commented Oct 22, 2024

toita86 commented Oct 22, 2024

francesco-ooops left a comment

aleuffre Nov 6, 2024 •

edited

Loading

aleuffre Nov 6, 2024

aleuffre Nov 6, 2024

aleuffre Nov 6, 2024

aleuffre Nov 8, 2024

aleuffre Nov 6, 2024

aleuffre Nov 6, 2024

toita86 Nov 7, 2024

aleuffre Nov 8, 2024

aleuffre Nov 6, 2024

aleuffre commented Nov 6, 2024

aleuffre left a comment

aleuffre Nov 8, 2024

toita86 Nov 8, 2024

carlosdauden left a comment

OCA-git-bot commented Nov 8, 2024

carlosdauden commented Nov 8, 2024

pedrobaeza commented Nov 8, 2024

OCA-git-bot commented Nov 8, 2024

OCA-git-bot commented Nov 8, 2024

		<field name="inherit_id" ref="base.view_partner_form" />
		<field name="priority" eval="50" />

		id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
		access_full_access_credit_policy_state,res.partner,base.model_res_partner,partner_risk_insurance_security.group_full_access_credit_policy_state,1,1,1,1

	"partner_risk_insurance_security.group_full_access_credit_policy_state" # noqa: B950
	"partner_risk_insurance_security."
	"group_full_access_credit_policy_state"

[14.0][ADD] partner_risk_insurance_security #403

[14.0][ADD] partner_risk_insurance_security #403

Conversation

toita86 commented Oct 22, 2024 • edited Loading

OCA-git-bot commented Oct 22, 2024

pedrobaeza commented Oct 22, 2024

pedrobaeza commented Oct 22, 2024

toita86 commented Oct 22, 2024

francesco-ooops left a comment

Choose a reason for hiding this comment

aleuffre Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre commented Nov 6, 2024

aleuffre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdauden left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Nov 8, 2024

carlosdauden commented Nov 8, 2024

pedrobaeza commented Nov 8, 2024

OCA-git-bot commented Nov 8, 2024

OCA-git-bot commented Nov 8, 2024

toita86 commented Oct 22, 2024 •

edited

Loading

aleuffre Nov 6, 2024 •

edited

Loading