Posted by greggles on September 22, 2008 at 8:29pm
| Project: | OG Project |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
If you allow issues to be assigned to other people it should ideally be limited to the users who are in the group/project for the issue.
This would be relatively simple to implement except that the audience can be changed and then the list of possible people in the assignee would change...So, it will need some fancy javascript action like the Project > Version box have.
Comments
#1
Ideally, you'd be able to control which "role" within the group had this behavior. If you think of each project on d.o as an OG (a direction I'd like to move), then people who use the module subscribe to the group as regular users, and maintainers of the module become the group admins. Maybe there'd even be a tier of "beta testers" or something -- folks who regularly help in the issue queue, etc, but don't actually have CVS access. It'd be cool if you could say "let maintainers and beta-testers assign issues to each other, but not end-users"...
#2
Here is the code for limiting the assignees to user who belongs to the project's group.
function og_project_project_issue_assignees(&$assigned, $node) {
$assigned='';
if (isset($node->project_issue['pid'])){
$pid = ($node->project_issue['pid']);
if (user_access('assign and be assigned project issues')) {
// All users are included if either anon or auth user has the perm.
$result = db_query("SELECT u.uid, name FROM {users} AS u JOIN {og_uid} as ogu ON(u.uid = ogu.uid) WHERE ogu.nid=%d",$pid);
while ($assignee = db_fetch_object($result)) {
$assigned[$assignee->uid] = $assignee->name;
}
}
}
}
#3
+1 to getting this implemented!
@maragnis: where does this code get implemented?
#4
This was my approach. (Probably a bit slower - but uses the existing list then filters it).
Place this in the project_issue.module at line 428 (works for Drupal 6):
$opts = array();$users = $form['issue_info']['assigned']['#options'];
$nid = $form['#node']->project_issue['pid'];
if (empty($users))
break;
foreach ($users as $uid => $name){
$result = db_fetch_object(db_query("SELECT COUNT(*) as count FROM {og_uid} WHERE nid = %d AND uid = %d", $nid,$uid));
if ($result->count || $name == 'Unassigned')
$opts[$uid] = $name;
}
$form['issue_info']['assigned']['#options'] = $opts;
#5
The code of #2 works well for me.
Just to correct : $assigned should not be set to '' cause it is not a string but an array.
$assigned=array(); is better (else it causes an error).
Remark (for the others) : to have this functionnal you just have to paste this code at the end of og_project.module.
Remark 2 : it works with vers 6.x
function og_project_project_issue_assignees(&$assigned, $node) {
$assigned=array();
if (isset($node->project_issue['pid'])){
$pid = ($node->project_issue['pid']);
if (user_access('assign and be assigned project issues')) {
// All users are included if either anon or auth user has the perm.
$result = db_query("SELECT u.uid, name FROM {users} AS u JOIN {og_uid} as ogu ON(u.uid = ogu.uid) WHERE ogu.nid=%d",$pid);
while ($assignee = db_fetch_object($result)) {
$assigned[$assignee->uid] = $assignee->name;
}
}
}
}
#6
Here's a patch based on the previous work.
1. I now include only active users.
2. Are we supposed to do check_plain on users or does project or FAPI handle that?
3. code style cleanups
4. I had to manually update the weight for the module to 3 as that didn't happen on its own for some reason (I installed via "drush en og_project").
#7
It seems the fix has a drawback as it empties the array, people without the permission to 'assign and be assigned' to an issue breaks the assignation that goes back to unassign.
I think we'll have to test and not empty the whole array but let the one who is indeed assigned by default, which is actually the first item in the array.
Example code :
$key=key($assigned);
$value=current($assigned);
$assigned=array();
$assigned[$key]=$value;
instead of just $assigned=array();
This is dirty code (we do the operation even if we write again on the $assigned array), so to be cleaned but it just shows what is wrong.
Other issue is that without the permission you can still unassign (but you cannot assign). At least with such change it does not loose the assignee.
edit : there is no check_plain in user_load() function, so I presume it is safe...
#8
Great points, eme. Could you provide it as a patch?
#9
And a new patch.
In this one we only alter the array if the user has the permission to alter it. It also preserves the first element as you suggested.
#10
Bah, I named the patch totally wrong in #9 but the contents are right for this issue.
#11
Even given the resolution at #962446: 1-1 requirement (for now) I think this patch is the wrong approach for a few reasons:
A) Why must we do this?
+ // Then we blank out the options and add back that one.+ $assigned = array();
+ $assigned[$current_key] = $current_value;
:( That means og_project would break the "Maintainers" tab on project nodes, for example. :( #880820: Add 'Maintain issues' checkbox to the project maintainers form would all be meaningless and confusing UI with this patch as-is. Why not just let og_project add options to the list but not be so disruptive and clobber everyone else implementing this hook?
B) There's still no way to optionally limit this to certain "roles" within the OG, right? If the plans for og_project on d.o go forward, this patch would be a disaster, since the idea is that each project OG would be the "fan club", not the co-maintainer inner circle. At the very least, we should have a global admin setting that controls this behavior site-wide. Maybe it could even be a per-project/OG setting? Best of all would be a set of radios: "( ) all members, (*) only group admins, ( ) no extra assignees" (or something). But, even if the 3-way radio is impossible via OG (I haven't looked in ages) we at least need a checkbox toggle for if we want to inject all members or not.
C) Currently, 'assign and be assigned project issues' is a global permission that lets you assign issues across all projects. I know it doesn't say so in the name, but this is a potential source of confusion we should address somehow. I don't believe silently limiting assignees to group members makes sense like this. OTOH, without a solution to (B) above, I can see why we'd want this permission check here. :/
D) I wonder if project_issue itself should enforce that the currently assigned user should always remain as an option. I think that's the source of the bug over at #958052: Issue automatically reassigned to 'Anonymous' if anonymous comment is left and would simplify this patch considerably. Basically, we'd move a few lines of code from project_issue_project_issue_assignees() into project_issue_assigned_choices(). That obviously shouldn't happen as part of this patch, but I'd rather that was fixed before this patch goes in as it currently stands.
#12
A) If it only adds options based on "in this group" then the list ends up being really really huge. I guess this patch should consider whether someone is in the group AND has the "maintain issues" checkbox?
B) That seems like a nice feature for someone else to add. This adds something valuable now and can be refined.
C) See B.
D) Probably so.
#13
@greggles:
A) I'm confused. The patch currently only adds options based on "in this group". The only reason you're seeing tons of options otherwise would be if everyone had the 'assign and be assigned project issues' permission (see C). If we're using the maintainer UI already, I don't see what this patch adds.
B) Maybe I wasn't clear. ;) Yes, the optionally limiting to roles within the OG can be a separate feature someone else can add. But I can't commit this patch to og_project without at least a global killswitch to disable this behavior.
D) okay, cool. Let's meet at #958052: Issue automatically reassigned to 'Anonymous' if anonymous comment is left and get that cleaned up ASAP.
I'll try to hash this out in IRC a bit to come up with a plan on (A) that doesn't suck. I have some ideas, but it'll probably be faster to converge on a workable solution in IRC than in here... stay tuned for an update.
#14
Initially when working with this code it took me a while to figure out how exactly the issue perms should work (and they need more tests, sadly) but It appears to be sourced from 4 distinct areas, correct me if I'm wrong.
Given how confusing this already is to someone who isn't aware of all the options, making another method that completely overrides all this is certainly attractive, but I think that more maintainable long term solution would work better.
What if OG Project just added a single row to the Project maintainers tab, for all members of the OG, and then instead of adding a 5th item to the list above it just becomes part of number 3? I think this could be the simplest and easiest way to configure the feature, but it may require some trickery on the backend or some patches to how project maintainer permissions are stored.
The other option if we you go with approach A from #12, is to alter the autocomplete of the project maintainers form to only list members of the current group. If OG is providing access control, and the group is private it should probably do this anyway (although it's a separate issue)