Skip to content

Conversation

@goreki
Copy link

@goreki goreki commented Jun 29, 2025

This PR adjusts the circumstances under which a res.group becomes available for portal users:
In addition to the magic module category in the data.xml of this module, a boolean field 'portal' can be set on a group to make it selectable for portal users.
For backwards compatibility I have kept the module category, though.

This makes it also possible to put portal-groups into actual categories and then have them displayed in those categories on the res.users form view.

It addresses an old/stale/closed issue: #232

Not sure if actively pinging the main contributor is best practice or frowned upon, but since this module was initiated by you, maybe you want to take a look 😄
@hbrunn

@OCA-git-bot
Copy link
Contributor

Hi @hbrunn,
some modules you are maintaining are being modified, check this out!

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

please add a sentence or two about the new flag in the readme, also explaining in which cases you'd want the flag. when done, squash your commits

Comment on lines 55 to 62
arch_root = arch.xpath("//field[@name='groups_id']")[0]
if len(arch_root) and portal_groups:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
arch_root = arch.xpath("//field[@name='groups_id']")[0]
if len(arch_root) and portal_groups:
if not portal_groups:
return result
for arch_root in arch.xpath("//field[@name='groups_id']"):

no need to do the write below if we don't change anything, and arch.xpath("//field[@name='groups_id']")[0] raises if the xpath is empty for whatever reason

Copy link
Author

Choose a reason for hiding this comment

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

true. I did it differently and moved the write() section into the if-block.
I think I now also understood that the whole function is called twice during install/upgrade - and the first time with the 'install_filename' in the context, which creates a placeholder view in the super() function, which therefore leaves us with the empty xpath result.
you didn't encounter that problem beforehand, because you where for-looping over the xpath results --> not doing a single loop in that case.

I didnt try, but I would even go as far as saying that our whole function _update_user_groups_view() can simply be skipped at the very top by checking if the 'install_filename' is in the context

@goreki
Copy link
Author

goreki commented Jul 4, 2025

please add a sentence or two about the new flag in the readme, also explaining in which cases you'd want the flag. when done, squash your commits

thank you for all the feedback - I will integrate it as suggested during the weekend. Would you be okay with me adding myself as a contributor in the manifest?

When this PR is going to be merged, I will also take a look at migration to v17 and 18 (I see there is already a PR, which obviously does not have my adjustments included yet). I am not worried about any big changes - more about following the guidelines, so this will be another learning experience for me.

@hbrunn
Copy link
Member

hbrunn commented Jul 4, 2025

contributors go usually to https://github.com/goreki/server-backend/blob/16.0/base_portal_type/readme/CONTRIBUTORS.rst

…vided in this module (but it is still kept for backwards compatibility):

a boolean field 'portal' can be set on a group to make it selectable for portal users.
this makes it also possible to put portal-groups into actual categories
Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

thanks

@github-actions
Copy link

github-actions bot commented Nov 9, 2025

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.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale PR/Issue without recent activity, it'll be soon closed automatically.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants