Posted by miro_dietiker on October 25, 2009 at 2:00pm
| Project: | OG User Roles |
| Version: | 6.x-4.1 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
Since "Default role for new group administrators" and "Default role for new group members" are not configurable per group, i don't see any reason to store them in the ogur table.
Much more i would recommend to create the assignment adhoc.
As long as ogur doesn't offer any way to rebuild user assignments, results are wrong after any update.
Attached patch considers adhoc default and admin default role based on configuration and group context.
| Attachment | Size |
|---|---|
| ogur_adhoc.patch | 1.43 KB |
Comments
#1
I do not see a bug report here.
Please also make sure to read http://drupal.org/coding-standards, your code needs work either way. However, before continuing with coding, please provide a proper bug report first. What did you do? What did you expect? What did happen? What did not happen?
#2
The initial bug report is:
Make groups, add some admins
Add an ogur role "Group X Admin". Make it default group administrator. Previously added group admins will lack the default group admin role. Also OGUR doesn't provide a way to rebuild default group roles (leading to heavy consistency problems).
I don't see any need for Admin and Member roles to be persisted. If we assign them adhoc, they won't have a chance to get wrong.
OK, the patch lacks some coding-standards. But it should allow you to understand the thoughts. I'll work another iteration on it, but i kindly ask for some feedback/inputs about these concepts and thoughts.
#3
Let's start with a test.
#4
I think this patch makes sense, although I'm not sure why this isn't covered by the existing code already. We therefore definitely need tests (since there are already some, most likely that's merely a copy-n-paste action with adjustments -- should also test the combination of default roles and default admin roles and custom assigned roles).
#5
Hm. Additionally, this seems to slightly clash with #534880: Improve maintenance handling of default group member/admin roles
If I'm not mistaken, then we currently store records for default [admin] roles for new members/admins. This patch basically changes the behavior to process them dynamically, without any database records.
That other issue identified the bug that when changing the default role settings, existing members/admins don't automatically get the new configuration - because the records are not updated. D'oh? ;)
I'm not sure what the perfect way forward is, but my immediate gut feeling is that we want to stop writing records for default roles and only apply them dynamically...?
Thoughts?
#6
Either stop writing -- or update... ;-)
It might make sense to write records if you're up to being able to query for those roles... (e.g. Views integration...)
This would make sense for administrative lists. But without batch updates the base is broken and i suggest degradation to the dynamic case.
Greets - Miro
#7
As of now, I did not really understand the use-case for #729256: Views integration yet.
Additionally, even if we would replace the current administrative UI of OGUR with a view, then we most likely wouldn't display the default user roles either.
Thus, I'd still be in favor of a dynamic approach.
#8
Decision taken. #534880: Improve maintenance handling of default group member/admin roles has been marked as duplicate.
#9
This will be very useful to my site, thank you for looking in to this.
#10
Firstly, I would just like to say this is a great little module.
I ran into the problem where all my existing groups and group users where not assigned the appropriate default roles. This patch sounded like it fixed my problem, but it did not work for me. So I started working on making the patch work for my needs.
Because of my projects requirements, adhoc default role assignment would not work. I am also not in favor of it as it will prevent me from implementing the ability to revoke user roles, even if it is the default group role. This makes sense to me since ogur is allowing for privileged users to set the default group role. Therefore the role set would be optional/dynamic as it could be any of the configured roles, so it would only make sense to add the ability to revoke the default role per a user as required even if it is the default role.
Please note this patch contains all my improvements/changes including a few that are not directly related to this issue. Sorry I did not break this patch up into smaller issues, but I don’t really have the time at the moment.
- One minor thing I only thought of after creating this patch was that maybe none default roles should be deleted instead of revoked to save on database size.
I hope somebody finds this useful.
#11
This patch looks totally useful and even has a high chance of getting committed, but we absolutely have to keep the patch for this issue focused and split all of those other changes into separate issues. Only changes directly related to applying default roles should be kept in this patch here. Especially the status column is highly debatable, but discussing it doesn't belong into this issue.