Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.merlin and I were chatting about how it'd be handy if we could assign issues to each other in the queues today, and I since I found myself taken with insomnia tonight, I ended up writing an initial draft of a patch to project_issue that should theoretically permit exactly that.
I don't know if this is something that's been discussed at all, discussed and shot down, or whatever, but since I've got the code, I figured I might as well post it. Please note that my local sandbox environment for project* is woefully inadequate, so this cold code that's entirely untested.
I'm happy to continue working on the code to get it into shape as needed if there's a desire for this. As of now, the only glaring thing that seems like it might be missing is I haven't taken steps to ensure that when someone is assigned to an issue that they hadn't previously commented on, no steps are taken to add that issue to the assignee's 'My Issues' queue.
Most of what I've done is some minor tweaking of a few functions. The biggest revamp is in mail.inc's project_mail_notify(), but those changes aren't actually all that significant - most of what I've done is just reorder the function into two halves, where the first half handles steps shared by both 'notify' and 'assignee' emails, whereas the second half is responsible for sending the emails themselves.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 253825_13_drupalorg.patch | 2.52 KB | hunmonk |
| #12 | 253825_12_drupalorg.patch | 2.77 KB | aclight |
| #8 | 253825_8_drupalorg.patch | 1.35 KB | sdboyer |
| #7 | 253825_829548_project_issue.patch | 1.14 KB | sdboyer |
| project_issue.patch | 16.32 KB | sdboyer |
Comments
Comment #1
aclight CreditAttribution: aclight commentedTo answer your first question, this is part of a larger issue of issue subscriptions in general. But more specifically, various aspects of this specific issue have been discussed in several other issues, including the following:
#34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment
#237294: When I submit an issue the assigned person does not get a notification email.
#4354: Select from various users for assigning issues
As for your local project sandbox environment, do this. Checkout a fresh D5 copy, co the Drupal.org testing installation profile, and co the various modules you'll need for that profile. If you go to the d.o testing profile project page, click on the link to documentation for instructions and a bash script you may want to use (with local modifications) to do some of this automatically. Last, but still important, you should first apply my patch at #231640: Auto-generate some release nodes to automatically generate release nodes for you. Also, you could apply (and review while you're at it :)) patches from #240546: project release type vocabulary should be multiple select and #232023: Project API terms are generated in the wrong order if you want your test site to be even more accurate. Once you've downloaded all of these, and patched the profile, *then* you can actually install the site and you'll have a fairly accurate test site.
Now, on to the real issue here :)
My understanding is that we've pretty much finished adding features to the D5 version of the project issue module. I've already started a D6 port of the project module and will be doing project_release and project_usage shortly. Project issue won't be far behind, and unless this all goes in before I start the port I'd rather not have this hanging over my shoulder.
Stepping back, I'm not convinced that this is the best way to solve the problem. I think there are 3 real issues here that are all connected:
1. What user names are listed in the "Assigned" field. Should certain users (eg. project maintainers) have additional username options in that field compared to a regular user?
2. Who gets email notifications when an issue is commented on? Can people subscribe to issues without actually following up in the issue. Should a user who is assigned an issue *always* get emails? If not, how do we determine when they get them?
3. What issues should show up in the "My issues" list. And, on a related note, what projects should show up on the "My projects" part of that page?
There are many different use cases and so I don't think hard coding any particular use case into the project_issue module is a very good idea. It might be desirable to have a few use cases in mind that can be turned on/off by either a site administrator or possibly a project maintainer on a per-project basis.
But before we get further into actually coding, I think we need to fix subscriptions in project issue first. The current system works but is pretty inflexible and fairly messy. There's been discussion of using another contrib module for this, such as the subscriptions module or the notification framework modules, but AFAIK nobody has really evaluated our options very thoroughly, but we'd love to get input from anyone who has done this.
I should really take most of what I wrote above and put it into a wiki page or something on g.d.o so that this can be discussed more. For now I guess I'll mark this as postponed. I certainly agree that the workflow right now is sub-optimal, especially for large projects like panels and views, but I don't think it makes sense to keep putting band aids on the problem while avoiding restructuring those parts of the module to better suit our needs.
It'd be nice if dww or hunmonk could chime in here and let me know if I'm off base.
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedI would just like to comment that even an interim solution here would be useful. Right now, there is simply no way for sdboyer to mark Panels issues for my attention, and vice versa, and this is a glitch; it means that one or both of us has to keep manual lists of issues if we're going to have the other work on them, when it would be great if we could say "This is something the other should work on." I don't think doing this has much real impact, and yes, it is not a full solution, but it is something that could ease a problem right now because a real solution is probably at least a year out.
Also, I think aclight is focusing on some minor changes in the patch, when the real purpose of this patch is to allow people with CVS access to a project to assign the issue to other people in that same group.
Comment #3
sdboyer CreditAttribution: sdboyer commentedWell, if nothing else, I owe you a few patch reviews on those links simply because you wrote such an extensive response so quickly :)
It doesn't seem to me that you're off base at all with your response - let me hit the issues you've raised in the order you've raised them:
1. This was my core concern initially, and as I was working on this patch, some thoughts occurred to me: co-maintainers being able to assign issues to one another is one of a VARIETY of different relationships that could, and perhaps even should, be in place when it comes to determining who all appears in that dropdown list. While invoking a hook that allows other modules to determine members of the assignees list is kind of a step in the right direction, it's also a bit of a 'band-aid' in the wrong direction. Really, I don't think the issue at hand with the assignees list is so much about exposing that list, pure and simple, to outside modules so that they can add their own people as it is about developing a richer, more pluggable system that is aware of a broader logic behind who appears in that list, why they're there, and what different actions should be taken depending on who gets selected. Adding additional users to the dropdown list without taking any other related action is essentially significant only with respect to the database - that is, it is not connected to any of the various mechanisms that project* uses to facilitate communication between PEOPLE. However, solving those problems was clearly beyond the scope of the patch (and something I hoped/figured you all were thinking about, so wasn't about to work on without talking to y'all first), so I generally followed the 'hard-coding' logic that is currently used in generating issue emails.
2 & 3. Yep, these are the same questions re: participation and notification flexibility that I had pounding in my head while writing the code, and I think they're the heart of the issues that underlie what you raised in your first point. tbh, I was surprised that there weren't more (read: any) checks against individual mail & subscription preferences throughout the entire scope of the code I worked through. I think there's a real theoretical question here, one that smacks more of project management techniques and even sociology than it does of computer science: given that there are virtually innumerable ways in which we could construct a notifications system and that a good system will define an architecture that users are able to control with a simple interface, what's the optimal way of constructing that system so that the various axes of options we provide to the user a) are immediately intuitive to the user, b) play well with each other even when they become combinatorially complex, and c) ultimately allow for a user experience that flows from there preference selections wherein the tools and information provided from the system are no more, and no less, than what they need for their particular purposes.
Holy grail much? :)
There are aspects to the current code that I think pave the way for a considerably more flexible system; in particular, the currently-in-place system of marking particular nids as having emails to dispatch (project_issue_set_mail_notify()), then performing those operations as a final step in the page call is good. I have yet to play around with subscriptions or notifications (is there a plan to integrate with one or the other, by the way? hopefully that's not too much of a hot button...), but I imagine that they both use a similar system for managing the dispatching of emails. At least, that's how I'd write it. I also think that the changes I made to project_issue_set_mail_notify() are the type of direction we could be thinking in - rather than simply passing a nid and that being the only information available to the actual mailing operation, we pass some $ops along with that instruct the mailer to deal with the information in that nid differently and dispatch different types of emails accordingly.
Just spitballing here, but I think the optimal route separates the process into three distinct phase:
1. A 'designation' phase, which wouldn't need to be that much more sophisticated than what project_issue_set_mail_notify() does right now. It'd be called during one or another of the project_issue FAPI-related functions, and would be responsible for highlighting what all nids need to have emails sent, and what the nature of those emails should be.
2. A 'construction' phase, more along the lines of project_mail_notify(), where the information from the 'designation' stage is taken and a set of emails are constructed according to the parameters that were set during designation. We'd want to keep all the email data inside a structured array, and avoid the inclusion of formatting markup as much as possible.
3. A 'modification/dissemination' phase. This is where the flexibility happens - we've got a fully-built email/set of emails in our structured array, along with information about who those emails are to be sent to. We foreach() through the list of users and nab their notification preferences, then operate on copies of the default email array and construct the emails for each individual. At that point it ought to just be text processing, so there shouldn't be any intensive db queries going on, so there shouldn't be much of a performance hit. Also, this would be an ideal place for one hook, at least: prior to running through the array of users, we invoke a hook that allows us to make modifications to the structured array, with awareness to the type of email via an $op parameter. This ought to mean that people are fully able to control the structure & appearance of their own project* emails via external modules implementing the hook without interfering in project*'s own process at all.
So yeah, that's...a slightly more exhaustive idea :) I'll be interested to hear your thoughts!
Comment #4
aclight CreditAttribution: aclight commentedIf this is truly the main goal, I don't see a problem with adding this functionality. I think it should go in the drupal.org module and would just implement hook_project_issue_assignees() and add any co-maintainers to the list if the current user was himself a co-maintainer.
Such functionality could also be useful for the project issue module itself, but it's a bit more complicated. Currently, the only way to have co-maintainers of a project is to use the cvslog module and set up a project to be connected to a CVS repository. But see also #69556: move "cvs access" tab into project, make it generic for plans on how this could be improved and generalized. So knowing who the co-maintainers are is difficult without having the cvslog module. We're trying to break away from hard coding things into project* that rely on the cvslog module, so that's why I think this should go in the drupal.org module.
But, the main content of sdboyer's patch was not implementing hook_project_issue_assignees(), which was actually not implemented at all in the patch. The main focus was changing who gets emails on an issue and what those emails look like. And that's the part of the project issue module that I'd like us to avoid changing until we have a well thought plan that we can follow. And, as merlinofchaos pointed out, it could be a while before that happens.
@merlinofchaos: I know that you use email to monitor your issue queues, so I didn't suggest just tackling the issue of who issues are assigned to initially. But if just that alone would be helpful, I don't see a problem getting that up and running, provided a) someone writes such a patch, and b) dww/hunmonk (or someone with power to implement said patch) agrees. I'm not sure how helpful it is to have an issue assigned to you when you don't also get an email about that, however. Maybe you've subscribed yourself to all views issues and use your mail client to filter everything to which you are not assigned out? If so, I can see this being useful.
@sdboyer Re: plans for the future:
As far as I know, there are no specific plans to use any notification/subscription framework/module. Personally I haven't ever used any of those types of modules, and have no basis on which to make a comparison or determine how fit they are to replace the current subscriptions functionality in the project issue module.
What we want to avoid doing is reinventing the wheel here. Project* has done that enough over the years (just look at the project browsing code, which is like a mini views with very little of the flexibility). I haven't looked at actions and triggers, but maybe we can use them to our advantage for some of this functionality once we get a D6 port finished. Your ideas on what the ideal system would do look good, at first glance, but not if we have to do all of this ourselves in project issue again. If you have the time/desire to help evaluate the available options, we'd be more than grateful for the help.
Comment #5
sdboyer CreditAttribution: sdboyer commented@aclight (in general)
I didn't implement a new hook_project_issue_assignees(), but I did modify it. Unfortunately, it's dependent on the very thing that you're trying to avoid - one of the cvslog tables, the cvs_project_maintainers table (I think that's what it's called) - but I definitely did change the implementation of the hook. I actually didn't change the appearance of the default email at all - just reordered the project_mail_notify() function so that I didn't have to write a special separate function for the assignee emails, and so that there wasn't redundant code. Nothing about the appearance of the issue emails themselves should be different; from the perspective of those emails, the code reworking was purely aesthetic. Or at least, it was intended to be. Those're toes I have no desire to step on.
I imagine there's probably a way to figure out who's an approved cvs maintainer without the use of that table...but it certainly wouldn't be as easy. I couldn't find it; honestly, I was expecting there to be a function in cvslog itself that'd return an array of form uid => name for all the maintainers of a given project.
@aclight (re: @merlinofchaos):
What you've described is actually exactly what this patch is intended to do - no more, no less. It adds the people who are currently approved cvs co-maintainers on a given project to the list of available assignees, then, if one maintainer assigns another maintainer to an issue, it generates a special email (with a different subject and different body) in addition to the normal email and sends it to the assignee. I wrote it that way because I had the very thought you raised there - just how useful is it to get assigned to an issue if you don't get an email notification about it?
@aclight (re: me)
Wise statements all around, I can't stand reinventing the wheel. I'm going to be going neck-deep on subscriptions/notifications for a project pretty soon, so I'll keep these questions in mind as it regards project_issue. Actions and triggers are also probably a good way to go here, and are another area I need to learn more about. If nothing else, it makes sense to me that project* ought to model good developer behavior when it comes to not reinventing the wheel whenever possible, and I'm happy to make sure my contributions do so. So sure, I'll commit to working on this end of things once the D6 port is complete.
Comment #6
merlinofchaos CreditAttribution: merlinofchaos commentedaclight: It's useful because I can very easily create a search filter on assignee. So I can find views assigned to me *very* easily, with or without email notification. Being able to have important issues come to me in a single search is like gold. =)
Comment #7
sdboyer CreditAttribution: sdboyer commentedAs promised, here's the sweet-and-simple version that does nothing more than make a small tweak to project_issue_project_issue_assignees(). Nothing's changed about my inability to test it just yet, and it's quite possible that my fun with array_flip(array_merge(array_flip(...))) doesn't work quite as intended, but the basic idea is still valid, I think.
All this patch does is add about ten lines of code that allow maintainers of a given project to assign issues to their co-maintainers.
Comment #8
sdboyer CreditAttribution: sdboyer commentedThis is me, reading the instructions on the box. lol.
Patch is now against drupalorg.module.
Comment #9
aclight CreditAttribution: aclight commentedI haven't tested this yet, but see some immediate problems.
The $node object that's passed to hook_project_issue_assignees() is the issue node object, not the project node object. So you need to enclose the block that does the db query in
if (!empty($node->pid)) {...}to prevent any query from happening if $node->pid is 0 or not set, which would be the case if the user is at /node/add/project-issue.You also need to change $node->nid to $node->pid as your parameter to db_query() in the two queries.
I'm not sure we even need two queries here. Why can't we just take out the first query and do this:
That should select any maintainers of the project other than the current user (since the current user is automatically added in project_issue_project_issue_assignees()). I also don't see why you need to do the array acrobatics. Can't you just check if each user returned from the db query is already in $assigned, and if not then add a row to the array?
As I said, I haven't tested the code or my suggested changes, so it's possible I'm overlooking something.
Comment #10
sdboyer CreditAttribution: sdboyer commentedI doubt you've missed anything, as my patch is just as cold-coded and untested as it was when I rolled it.
I actually tried to solve the node/add/issue a different way in my initial patch by generating two slightly different emails if the issue was new and had been assigned by one co-maintainer to another, versus if it was an existing issue being reassigned. However, the use case for the former is so narrow, and is so easily solved by just an immediate blank follow-up comment to reassign that it's really not worth the trouble, I don't think.
It may well be a total waste to have the two queries. The query you've written is almost exactly the same as the second one I have in there, of course, and the only purpose of the first query is to determine whether or not we should continue on to the more complex JOIN query. I did it because of the quite possibly erroneous way that I instinctively think about optimizing db queries: it seems to me that performing the simple, non-joining query as a conditional would be a way of preventing the slightly more complex JOIN query from running unless it absolutely has to. Given that this is a routine that'd be running all day and all night on d.o, I figured we might be saving a bit by avoiding the latter query.
When I think about it rationally, I seriously doubt that there are any appreciable gains to be had here, even if my logic IS correct. It's really just the product of a kneejerk thing I do while coding. So I suspect that scrapping the conditional query is the way to go; it can probably just be replaced with
if (!empty($node->pid)) {...}.Comment #11
aclight CreditAttribution: aclight commentedI'm certainly no db expert, but I think the general goal is to minimize the number of queries whenever possible, as long as doing so doesn't require extremely complex queries with lots of joins. I don't think a query with one join is going to cause any problems. Plus, it makes the code simpler and easier to follow, IMO.
Comment #12
aclight CreditAttribution: aclight commentedI think this patch does the job a little better.
Note that at the moment when you change the project an issue is assigned to (from a comment), the assigned select box does not update the options available, as it should. I'm working on a patch to the project_issue module at the moment that'll take care of that. But that bug doesn't directly impact this issue, so this patch can be reviewed independent of that issue.
Comment #13
aclight CreditAttribution: aclight commented#254416: Change of project in comment does not update assigned options is the issue for the project_issue module dealing with updating the assigned select box when the project is changed from a comment.
Comment #14
merlinofchaos CreditAttribution: merlinofchaos commentedI give aclight's patch a +1 on visual review. I'm not, unfortunately, in a position to test project.module patches.
Comment #15
NancyDru#298181: Assign to owner on DO marked as duplicate of this.
Comment #16
hunmonk CreditAttribution: hunmonk commentedlatest patch didn't apply. re-rolled and did a quick test on a local install -- seems to work as advertized. i think we might be able to optimize things a bit better, but i don't have time to dig into it right now.a
Comment #17
dwwSorry, I completely forgot about this issue since I had never participated until now. ;) Reviewed the patch, simplified the code, tested locally, committed to HEAD, and deployed on d.o. Thanks everyone, and enjoy!
Happy holidays from Germany. ;)
Cheers,
-Derek
Comment #19
dwwGabor asked if this code really needs to live in d.o module. Probably not -- it could safely live in cvs.module, and it's probably better there, anyway. A more generic solution in project or versioncontrol API can come later, but there's no reason this functionality is d.o-specific.
Comment #20
Gábor HojtsyWill work on this later as part of the upgrade.
Comment #21
Gábor HojtsyComment #22
jpetso CreditAttribution: jpetso commentedFor the record, I'd really appreciate if project maintainership moved out of cvs.module (or versioncontrol_project, for that matter). It also won't map at all to DVCS-style branch management, and issues like this one show that the scope of project maintainership is wider than just commit access.
Comment #23
Gábor HojtsyWell, my point was that drupalorg module has raw queries to get people who have CVS access to modules. I was saying this data should be available from CVS module (or whatever VCS module we are using), instead of doing raw queries in drupalorg module. So we can tie the data to where it is also needed. I am not sure we need this whole function in cvs module, my point was not to reinvent the wheel from the start in drupalorg module.
Comment #24
sdboyer CreditAttribution: sdboyer commentedThis isn't the place to have the discussion, so I'm just noting for posterity's sake, but it's not entirely accurate to say that it won't map at ALL to dvcs branches.
Comment #25
Gábor HojtsyHrm, we keep using cvs module for now, and what we ported perfectly works, so not going to mess with it as part of the upgrade. Will look a this later once we move off of cvs module. So this should be considered closed as part of the upgrade.