[16.0][IMP] dms_field: Propagate groups from field template #441

SirPyTech · 2025-08-18T10:18:24Z

With this PR, the groups declared in child directories of the field template are propagated to the directories created in the target record.

I have also included a couple of small refactorings, in their own commit.

Let me know what you think!

victoralmau

Code review OK

By default, the value inherit_group_ids is True, so until now, it was always set to True in child directories. Changing this IMO has no side effects and allows for more flexibility in more complex cases (where a subdirectory of a template has different permissions than others).

victoralmau · 2025-08-18T10:32:05Z

dms_field/tests/test_dms_field.py

+        new_child_dir = partner.dms_directory_ids.child_directory_ids.filtered_domain(
+            [
+                ("name", "=", child_dir.name),
+            ]
+        )
+        self.assertRecordValues(
+            new_child_dir,
+            [
+                {
+                    "inherit_group_ids": child_dir.inherit_group_ids,
+                    "group_ids": child_dir.group_ids.ids,
+                }
+            ],
+        )


Suggested change

new_child_dir = partner.dms_directory_ids.child_directory_ids.filtered_domain(

[

("name", "=", child_dir.name),

]

)

self.assertRecordValues(

new_child_dir,

[

{

"inherit_group_ids": child_dir.inherit_group_ids,

"group_ids": child_dir.group_ids.ids,

}

],

)

subdirectory_1 = partner.dms_directory_ids.child_directory_ids.filtered(

lambda x: x.name == self.subdirectory_1.name

)

self.assertFalse(subdirectory_1.inherit_group_ids)

self.assertEqual(subdirectory_1.group_ids, access_group)

subdirectory_2 = partner.dms_directory_ids.child_directory_ids.filtered(

lambda x: x.name == self.subdirectory_2.name

)

self.assertTrue(subdirectory_2.inherit_group_ids)

self.assertIn(self.group, subdirectory_2.group_ids)

IMO this is more readable and checks that both subdirectories have the right data.

Thanks for the quick review!
This needed some tweaking, but I accepted your suggestion overall, please check

victoralmau · 2025-08-18T10:42:41Z

I think all these changes can be combined into a single commit: [IMP] dms_field: Propagate groups from field template

The title of the PR should be changed to something like: [16.0][IMP] dms_field: Propagate groups from field template

SirPyTech · 2025-08-18T11:03:08Z

I think all these changes can be combined into a single commit: [IMP] dms_field: Propagate groups from field template

I prefer to highlight which commits do not change the behavior ([REF] ones) from the other ones, and to keep each commit focused on their change, so I'd like to keep them all.

The title of the PR should be changed to something like: [16.0][IMP] dms_field: Propagate groups from field template

Right, added more details in the PR title, I'm not a fan of including the version in the PR title though.
If you need that for filters, note that PRs can be filtered by version 16.0 using base:16.0 filter (see https://github.com/OCA/dms/pulls?q=is%3Apr+is%3Aopen+base%3A16.0).

pedrobaeza · 2025-08-18T11:04:21Z

@SirPyTech the version has to be in the PR title, not in the commit message.

SirPyTech · 2025-08-18T11:13:07Z

The title of the PR should be changed to something like: [16.0][IMP] dms_field: Propagate groups from field template

Right, added more details in the PR title, I'm not a fan of including the version in the PR title though.
If you need that for filters, note that PRs can be filtered by version 16.0 using base:16.0 filter (see https://github.com/OCA/dms/pulls?q=is%3Apr+is%3Aopen+base%3A16.0).

@SirPyTech the version has to be in the PR title, not in the commit message.

Why does it "have to be" there?
If that's used for filtering the PRs, the dedicated filter base already exists as I wrote above

pedrobaeza · 2025-08-18T11:24:06Z

Current convention is for seeing in a glance which is the target branch from the pull request list or the received notification mails, without having to navigate to the PR itself to see it. Please make reviewers' life easier.

SirPyTech · 2025-08-18T15:01:29Z

Current convention is for seeing in a glance which is the target branch from the pull request list or the received notification mails, without having to navigate to the PR itself to see it. Please make reviewers' life easier.

I am a reviewer too and I have never needed the version in the title because I'm using the base filter, and I think that should be the way for searching PRs of a specific version; in the rare occasions I needed more, I used GitHub's API.

Could you please propose this convention to the Guidelines in https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst?
The contributors already have so many guidelines to follow, it would be nice if they were at least all written somewhere.

When this new convention is in the guidelines, maybe the OCA bot could help enforcing it 🤖 and you wouldn't need to ask all the contributors to edit their PR's title.

pedrobaeza · 2025-08-18T15:08:49Z

There's no section in guidelines for that, and I'm not going to lose more time in that bureaucracy given the result in other attempts, but if someone asks you for that, and that would ease the review, please do it as I do. If not, then we will be reluctant to review your PRs. It's that simple.

victoralmau · 2025-08-20T06:26:41Z

My comment is still pending #441 (comment)

I think all these changes can be combined into a single commit: [IMP] dms_field: Propagate groups from field template

IMO, all changes can be combined in a single commit; having several in this case adds nothing.

SirPyTech · 2025-08-20T07:31:13Z

My comment is still pending #441 (comment)

I think all these changes can be combined into a single commit: [IMP] dms_field: Propagate groups from field template

IMO, all changes can be combined in a single commit; having several in this case adds nothing.

Why is this case special?
By the way, I already answered that one in #441 (comment):

I prefer to highlight which commits do not change the behavior ([REF] ones) from the other ones, and to keep each commit focused on their change, so I'd like to keep them all.

It happens to have different opinions 🤷 I think having different commits makes more sense and is more aligned with the guidelines:

Only make a single commit per logical change set.

(from https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message)

Rewriting a method to allow override is a logical change set, and that deserves a commit on its own.
Changing the behavior of a method is another logical change set too, and that deserves a commit on its own.

Would you agree with a compromise solution of having one [REF] commits that includes the existing [REF] commits, and one [IMP] commit?

aleuffre

Code and functional review, LGTM, we've successfully built an implementation based on this.

Regarding the more formal disputes: the separate [REF] commits seem cleaner to me, and also make reviewing and future forward porting easier.

On the other hand, if there's strong maintainer preference for a single commit, I think in this specific case the changes are small enough that I wouldn't want to block the PR over this disagreement.

OCA-git-bot · 2025-08-26T19:00:02Z

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

github-actions · 2025-12-28T12:26:10Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

SirPyTech · 2025-12-29T08:18:47Z

@OCA/document-maintainers can someone have a look?
This is a Beta module and the PR has 2 approvals from 4 months ago, so it should be good for merge.
Thanks!

pedrobaeza · 2025-12-29T08:22:06Z

dms_field/models/dms_access_group.py

+                [
+                    item._get_domain_for_item_from_dms_field_ref(item.dms_field_ref),
+                    [
+                        ("id", "!=", item.id),


Remove the trailing comma for having these 3 lines into 1.

Thanks for having a look!
I usually have every item in the domain in their own line so that changes to each item do not highlight the whole domain as changed in diffs, but in this domain that only has one item it probably makes little sense 😅

P.S.: Thanks for merging too!

Yeah, but at the end, diff viewers like the one in GitHub are already highlighting the specific part of the line that is changed, and the probability that this will be changed is very low.

pedrobaeza

/ocabot merge patch

OCA-git-bot · 2025-12-29T09:02:16Z

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

OCA-git-bot · 2025-12-29T09:05:35Z

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

pedrobaeza requested a review from victoralmau August 18, 2025 10:20

SirPyTech marked this pull request as ready for review August 18, 2025 10:21

victoralmau suggested changes Aug 18, 2025

View reviewed changes

SirPyTech changed the title ~~Propagate groups from field template~~ [IMP] dms_field: Propagate groups from field template Aug 18, 2025

SirPyTech force-pushed the 16.0-imp-dms_field-propagate_groups branch from 0b3a905 to df49a7f Compare August 18, 2025 10:54

SirPyTech requested a review from victoralmau August 18, 2025 11:03

pedrobaeza changed the title ~~[IMP] dms_field: Propagate groups from field template~~ [16.0][IMP] dms_field: Propagate groups from field template Aug 18, 2025

pedrobaeza removed the request for review from victoralmau August 18, 2025 15:08

SirPyTech requested a review from victoralmau August 18, 2025 15:13

aleuffre approved these changes Aug 22, 2025

View reviewed changes

victoralmau approved these changes Aug 26, 2025

View reviewed changes

OCA-git-bot added approved ready to merge labels Aug 26, 2025

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 28, 2025

SirPyTech force-pushed the 16.0-imp-dms_field-propagate_groups branch from df49a7f to b69584e Compare December 29, 2025 08:14

pedrobaeza added this to the 16.0 milestone Dec 29, 2025

pedrobaeza reviewed Dec 29, 2025

View reviewed changes

[REF] dms_field: Allow to override generated group domain

e123320

SirPyTech added 2 commits December 29, 2025 09:58

[REF] dms_field: Allow to override child directory values

d613785

[IMP] dms_field: Propagate groups from template

6c2fe17

SirPyTech force-pushed the 16.0-imp-dms_field-propagate_groups branch from b69584e to 6c2fe17 Compare December 29, 2025 08:59

pedrobaeza approved these changes Dec 29, 2025

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Dec 29, 2025

OCA-git-bot merged commit d97c691 into OCA:16.0 Dec 29, 2025
7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Dec 29, 2025

victoralmau mentioned this pull request Jan 2, 2026

[18.0][IMP] dms_field: Propagate groups from field template #454

Open

aleuffre deleted the 16.0-imp-dms_field-propagate_groups branch January 2, 2026 14:17

-        new_child_dir = partner.dms_directory_ids.child_directory_ids.filtered_domain(
-            [
-                ("name", "=", child_dir.name),
-            ]
-        )
-        self.assertRecordValues(
-            new_child_dir,
-            [
-                {
-                    "inherit_group_ids": child_dir.inherit_group_ids,
-                    "group_ids": child_dir.group_ids.ids,
-                }
-            ],
-        )
+        subdirectory_1 = partner.dms_directory_ids.child_directory_ids.filtered(
+            lambda x: x.name == self.subdirectory_1.name
+        )
+        self.assertFalse(subdirectory_1.inherit_group_ids)
+        self.assertEqual(subdirectory_1.group_ids, access_group)
+        subdirectory_2 = partner.dms_directory_ids.child_directory_ids.filtered(
+            lambda x: x.name == self.subdirectory_2.name
+        )
+        self.assertTrue(subdirectory_2.inherit_group_ids)
+        self.assertIn(self.group, subdirectory_2.group_ids)

Uh oh!

[16.0][IMP] dms_field: Propagate groups from field template #441

[16.0][IMP] dms_field: Propagate groups from field template #441

Conversation

SirPyTech commented Aug 18, 2025

Uh oh!

victoralmau left a comment

Choose a reason for hiding this comment

Uh oh!

victoralmau Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

SirPyTech Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

victoralmau commented Aug 18, 2025

Uh oh!

SirPyTech commented Aug 18, 2025

Uh oh!

pedrobaeza commented Aug 18, 2025

Uh oh!

SirPyTech commented Aug 18, 2025

Uh oh!

pedrobaeza commented Aug 18, 2025

Uh oh!

SirPyTech commented Aug 18, 2025

Uh oh!

pedrobaeza commented Aug 18, 2025

Uh oh!

victoralmau commented Aug 20, 2025

Uh oh!

SirPyTech commented Aug 20, 2025

Uh oh!

aleuffre left a comment

Choose a reason for hiding this comment

Uh oh!

OCA-git-bot commented Aug 26, 2025

Uh oh!

github-actions bot commented Dec 28, 2025

Uh oh!

SirPyTech commented Dec 29, 2025

Uh oh!

pedrobaeza Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

SirPyTech Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

pedrobaeza Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

pedrobaeza left a comment

Choose a reason for hiding this comment

Uh oh!

OCA-git-bot commented Dec 29, 2025

Uh oh!

Uh oh!

OCA-git-bot commented Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants