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.
over at http://drupal.org/node/270610 there's another use case for project_issue to automatically create comments on an issue. instead of custom hacking this code again, we really should abstract it so that it's reusable. what i'd like to see:
- 'project_issue_auto_close_user' variable should be converted to a more general 'project_issue_auto_comment_user'
- the user loading and safety checks should be abstracted into a reusable function
- as much of the code for actually inserting the comment as possible should be abstracted into another function, such as insert statement, updating node stats, etc
- sensible defaults should be provided for many of the fields necessary to create a comment, so they don't need to be constantly declared/passed to the function (ex. thread, users, format, etc)
- pid should be forced to 0 in the abstracted function, to ensure the flat comment threading that we need.
and maybe some other things i didn't think of ;)
Comment | File | Size | Author |
---|---|---|---|
#35 | followup.patch | 17.51 KB | hunmonk |
#34 | followup.patch | 17.93 KB | chx |
#32 | followup.patch | 17.72 KB | chx |
#27 | followup.patch | 16.72 KB | chx |
#25 | followup.patch | 16.74 KB | chx |
Comments
Comment #1
dwwSounds good to me.
Comment #2
solotandem CreditAttribution: solotandem commentedI am currently working on this.
I assume we are writing this for 5.x and it can be included in a port?
Comment #3
solotandem CreditAttribution: solotandem commentedTo test the patch, 1) I ran cron on a node that I manually back-dated and 2) I manually called project_issue_auto_comment on a node with values.
A couple of questions:
1. In refactoring the code as suggested, I left the while loop in project_issue_insert_auto_comment although it would seem to be unnecessary (as the query should only have one row).
2. Should the function project_issue_insert_auto_comment always return TRUE.
Comment #4
aclight CreditAttribution: aclight commentedA few general comments:
* This is minor, but I don't exactly like that auto-comment is being used as a replacement for auto-close, because I feel that, grammatically, they aren't interchangeable. For example, "%name does not have sufficient permissions to auto-comment issues." sounds rather awkward to me. I'm not sure right now what a better phrasing would be, however.
* I haven't yet tested any functionality.
* This patch is sort of difficult to review, since the way you've modified the functions ends up making a patch that looks like more code was changed than really was. Once these issues below are addressed I'll give it a test and a more thorough review.
we'd have
+ * Automatically comment an issue.
should be "comment *on* an issue" or "add a comment to an issue".Comment #5
solotandem CreditAttribution: solotandem commentedResponse to aclight's comments
1. Changed to use a query to get the sid values.
2. I did not find a refernece to project_issue_auto_close_user in the uninstall hook, but I added one project_issue_auto_comment_user. In the update_N function, is it necessary to copy the value of the current variable and then delete the current variable. I added an update statement to simply change the value if present.
3. Changed all references to auto-comment where it seemed appropriate.
4. Changed.
5. Changed.
6. Changed.
7. Changed.
8. Changed on all queries in the module (was this intended?).
9. Removed constant.
10. Rewritten as discussed in IRC.
11. Changed.
12. Now being called by project_issue_auto_close and is intended to be the API call for the remote access.
13. Changed _project_issue_insert_auto_comment.
New questions
In project_issue_validate_auto_comment_user, I changed the default value for project_issue_auto_comment_user from 'anon' to 0 (zero). I don't see a reason why not to and this simplifies other code.
Testing
I tested the code by inserting 10 issues with comments and manually running cron to call the auto-close routine. I also ran the cron after changing the project_issue_auto_close_seconds value and the auto-comment user. All tests passed.
Comment #6
aclight CreditAttribution: aclight commented@solotandem: can you please try to set up whatever you're using to generate your patch to not include the hunk at the top of each modified file like so:
I hear Eclipse tends to do this but as I don't use it I don't know how to prevent that from happening. If you're using
cvs diff -up
from the command line these shouldn't be included.Also, it looks like you might be basing your patch off of the DRUPAL-5--2 branch. It's probably best to roll it against HEAD, though at the moment I believe both HEAD and DRUPAL-5--2 should be identical.
Comment #7
aclight CreditAttribution: aclight commentedI still haven't had a chance to test or look at this in detail, but wanted to address some of your questions. I'm going to be out of town this weekend, but will try to find time to review this fairly soon.
Responses:
1: On second thought, maybe now is not the time to be doing this, because I think the way you've done it could potentially lead to just as many problems as hard coding the sid, since if the admin changes the name from "fixed" to something else, then the logic will get messed up.
I think the best way to do this would be to have an additional admin settings page on which the user could select one or more sets of states (to and from) and for each specify a time interval that should elapse before the "to" state is changed to "from". But doing this in this patch is probably a bad idea as well.
So maybe you should just go back to using a constant, like you did in the last patch, and then file an issue in the queue with this suggestion (I don't think there is one already).
2: I didn't realize project_issue_auto_close_user wasn't in the list of variables to delete in hook_uninstall. That was an omission. For the update function, I think that instead of directly changing the name of the variable by manipulating the db, we should instead do something like:
7: I think having the granularity of the time before closing an issue be day, instead of second, is sufficient and easier for an admin to calculate.
8: Normally I would say not to touch code outside of the scope of the patch, but it looke like it's just 1 or 2 additional lines affected by this change. I'm not sure why p was used instead of pi as the table alias originally, but typically in the project module we try to use an alias that makes it obvious which table is being aliased.
Other things:
14:
I'd like to see the function description here be a bit more descriptive.
Also, I think it would be better if this function accepted one parameter, a keyed array, instead of 3 parameters. I think this will give us better flexibility in the future. For example,
In this case, the function signature would look like:
15:
We might also want to think about returning something useful here. Perhaps the number of comments actually inserted? What is returned might depend on part on what kind of information the pift stuff would need to know.
I'm not sure off the top of my head, but I would guess it has something to do with the fact that if nothing is entered in the text box for user then comments should not be automatically closed. But I'd need to look at the code in more detail to say for sure.
Comment #8
dwwCool, thanks for working on this, folks! I haven't had time to closely read all the comments or review the patches. A few quick things I wanted to mention:
Re: 1. Yes, the status values should be constants for the purpose of this patch -- adding the flexible status stuff would make this too large to review/test. However, in another patch, we should fix that stuff so the status values aren't hard-coded -- there's already an issue for that: #27865: Remove hard-coded status options.
Re: 14. A keyed array seems better, since I could imagine wanting to programatically set other issue metadata for various reasons, not just the status. A co-worker once suggested a date-driven priority escalation on issues (not for d.o) where as time goes on, if it's still not closed, the priority increases. There are lots of other potential cases, so we should write this code to be flexible enough to handle them, not just the specific use-case that's motivating the change.
Anyway, time permitting, I'll take a closer look in the near future. Thanks again!
Comment #9
solotandem CreditAttribution: solotandem commentedThanks for the feedback.
@aclight: Regarding the code version, my understanding is this task was a version 5.x change that you would incorporate into the 6.x port you are working on. Please clarify.
Regarding the numbered items:
2. Using the variable_set and variable_del functions in this way does not add any rows to the $ret variable. When the DB update is run it displays "No queries" for this update. Is it important to provide feedback to the user on the queries run? That was why I used the update_sql statement as the other updates had done. What is the preferred approach?
14. I understand the point about flexibility but question the implementation. In IRC, we had agreed to allow for an array of nids to be passed to the function. If we still find this useful, the $comments array now becomes something like:
However, my understanding is the sole current use case for this code is the auto-close of issues after two weeks. This involves the same changes (i.e., inserting the auto-close comment and changing the issue status to closed) to multiple issues (= nids). The pending use case involving t.d.o would be similar -- large numbers of nodes with a given status would have status changed with a static comment added. For example: patch does not apply to head => needs work, or patches that pass tests => needs review (or whatever). Using the above $comments array would mean passing the same static message multiple times.
The use cases would seem to argue for two parameters: $nids = array of node ids, $changes = keyed array of comment properties to change (what you suggested minus the nid element). The same $changes would be applied to each issue node.
Regarding my new question with the second patch submission:
The current code stores uid if not anonymous, blank if disabled, and 'anon' if anonymous. I am suggesting the values be blank if disabled, and uid otherwise. I have this working now. It seems more consistent to me.
Comment #10
aclight CreditAttribution: aclight commentedA. HEAD is still the D5 version and will be for a bit longer at least.
Re:
2: Well, technically you're not really running any queries since you're just calling variable_set and variable_del, so I think it's fine that no queries were run is what is printed for the results of this update function.
14: Right now the only way this would be used is for closing issues, and soon for patch testing, but that doesn't mean that in the future we won't think of additional good uses for this. There has been discussion on d.o about automatically closing issues that have been set as active (needs more info) for > 2 weeks, and though I don't think that's a great idea it might be something we eventually do.
Furthermore, I can see that one day in the future, when project* has 10 or so steady contributors, and some nice sponsorship, that we might also make it possible to set an issue as being fixed from the cvs commit message, much like what can be done currently when using Trac. Or, we might eventually add in direct XML-RPC support in project_issue so that the cool kids who use IDEs that provide in IDE issue tracking support can do that as well. These may only be pipe dreams, but I think they are realistic use cases for this API we're working on.
So I really think using a fully keyed array is the way to go here. I'd like to take my suggestion a little further as well:
project_issue_auto_comment() takes a single keyed array as a parameter. Each key of the array is the same as the name of the field that ultimately goes into the $comment array. So, in other words, we could have possible keys of nid, uid, subject, comment, format, etc. Certain of these would be required, such as nid, and if that key did not exist for an element of $comments then that comment would not be inserted. Other keys, for example pid, would be ignored, even if they were in the array, since in this case pid must be 0. For keys that weren't in the keyed array, sensible defaults would be used.
By doing it this way, we allow for the greatest flexibility without (I think) too much complication. Basically, you'd:
a. check to make sure the required keys exist in a given element of $comments, and if not go to the next one
b. fill in the array with all of the default fields that are overwritable, eg. mostly things that are taken from $issue
c. overwrite any keys in that array with values provided in the element of $comments
d. overwrite any keys that must be a certain value (eg. pid and possibly uid) with whatever that value needs to be.
e. save the comment
I know this is slightly creeping the scope of this issue, but it wouldn't right for a project* issue to have no creep at all :)
As for how to store the auto comment user in the variable, I'd recommend that you try to ping hunmonk in IRC and see if he can remember why he did it that way, since I believe he wrote that code. Doing this the way you suggest seems to make pretty good sense, but I'd be surprised if he didn't also think to do it that way.
Comment #11
solotandem CreditAttribution: solotandem commentedThis is a debug patch as I left in some drupal_set_message statements to assist reviewers in evaluating the patch.
Regarding the numbered items:
7. Changed.
14. The auto_change function takes two parameters: $nids and $changes. This arrangement accommodates your "additional good uses" examples as well as the two concrete use cases -- auto-close and patch testing. As I understand your examples, each would most likely involve the changing of one issue at a time. The auto_change function will handle this. But by separating the nids from the changes, the function much more efficiently handles the two concrete use cases that involve making the same change (to issue status) and inserting the same comment for mass quantities of issue nodes. Using a "fully keyed array" would inflate the array with duplicate comment strings.
If my thinking is incorrect, please explain.
Also, I request input on which fields can be changed by someone calling this function and those that we should set. The code has comments indicating the elements that are and are not overrideable. This is a first cut on the separation of fields.
15. Changed to return number of changes made / comments inserted.
Other
Regarding the value stored in the (now named) project_issue_auto_change_user variable, I tried to ping hunmonk but got no reply. Will try again.
Comment #12
aclight CreditAttribution: aclight commentedOne note--a lot of your comment lines look rather long. While we typically don't wrap code lines in project*, and Drupal as a whole, it's best to keep lines of comments <~80 characters long for easier readability.
Re 14: I'm just not convinced that having multiple comment messages in the array passed in will cause a problem. How many comments do you expect will be changed in one call to the API function? My feeling is that in most cases the comment will be pretty short anyway, so I don't think we'll run into problems.
But, if we do decide that we want to prevent redundant comment messages from being passed in, I think there is a better way to do it. Instead of taking $nids and $changes as parameters, we take $changes and $messages as parameters. Then, instead of having $changes['comment'] = 'This is the comment.', we'd have something like $changes['message_id'] = 0 and then $messages[0] = 'This is the comment.' I still don't think this is necessary, but if so I feel that it's more self documenting and less confusing.
Comment #13
solotandem CreditAttribution: solotandem commentedThis is a debug patch as I left in drupal_set_message statements to assist reviewers in evaluating the patch.
Regarding the numbered items:
14. Changed as suggested in comment #12. Stubbed in the $messages array but did not implement. Is this acceptable?
For the auto-close and patch testing use cases, the implication of this design (at least the way I have coded it) is that inside the _auto_change function we now run the query n times, once for each record. Before we would run the query once and cycle through the n records. Doesn't this imply a negative performance hit?
Other
I shortened the comment lines to <~80 except where they were quotes of code. Is this acceptable?
For testing, the auto_close function builds an array with required and disallowed values. You can comment out one of the required values to test that no changes are made. I wrote a sql script to create issues and comments for testing. If anyone is interested in this, let me know and I will attach the file to another post.
Comment #14
aclight CreditAttribution: aclight commentedOne thing we need to consider here is whether or not the automatic comment creation should bypass the part of the code that sends an email to people subscribed to an issue. I believe that currently the auto close code does so, which is fine. However, I'm not certain that we'd want to skip sending out an email for issues where a patch does not apply, because otherwise those who primarily follow issues via email might never be aware of this.
Comment #15
catchIf we have automated testing running as it should, then those e-mails are likely to be as important as most others about an issue (compared to subscriptions at least) - file a patch, goes stale, gets marked to needs work - if I primarily followed issue via e-mail, which I don't, I'd want an update.
Comment #16
pwolanin CreditAttribution: pwolanin commentedSince this seems a bit stalled, I'm wondering what we really need for 5.x vs. 6.x. Previously testing.d.o was using a bot to leave comments about failing patches - is there some reason that can no longer work?
Comment #17
dwwFor the record, I think anything that makes the code harder to read/write in the name of optimization for a "problem" we don't even know we have is a bad move. I'd much rather pass in a simple array, keyed by nid, with an array of changes to make to each nid. So what if we're passing around some duplicate data? Code simplicity is vastly more important to me than that, especially since it seems like most of the time we're going to be calling this 1 issue at a time, anyway (e.g. if a patch no longer applies).
Issues like this are a good opportunity to make the project* code base more sane. Anything that adds complication for no obvious gain isn't going to work at all, and definitely won't get committed.
Thanks,
-Derek
Comment #18
boombatower CreditAttribution: boombatower commentedAfter looking at all this and listening to the conversations in IRC (some time ago) I don't see the usefulness of having an array containing all the changes for each issue on the basis of code simplicity or usefulness. The current use-cases that we have are as follows:
In either case the majority of the changes will be made to a large number of nodes with the same state. It would appear that is the nature of this whole idea to begin with, mass (batch) changes to issue states. The executing code will be performing one task to a large number of nodes, making the code perform any possible number of changes to any nodes in one execution or loops is much more complicated.
Having an array with changes for each node only forces the calling code to enter the same state data over and over and over, not to mention the memory footprint. In an attempt to get around that by separating the messages (largest memory component) into another array and referencing only adds one more level of complexity. Which from the comments above doesn't seem to be what is wanted.
Considering the current use-cases, general nature of the code, and future use-cases it would seem clear that batch editing a set of nodes to the same state is the most useful arrangement of this function. Other methods over-complicate the calling code and add to the memory requirements/load on the server.
I'm just not seeing how having less data with less arrays that fits the workflow of the calling code is more complex.
Comment #19
aclight CreditAttribution: aclight commentedOne use case you may not be considering (directly) is the potential for using this code to allow comments to be created remotely but not just the testing server. For example, perhaps if/when GHOP comes around again we'd make it possible to have comments posted on the Google code tracker also show up on our tracker. In that use case we wouldn't be doing the same thing to multiple issues, most likely.
I just don't know the scale of how many issues we expect to be changed at a time. If we're talking thousands of issues at once then you're probably right, we need to be really concerned about memory footprints, etc. But if we're talking 10-20 issues at once, I think code simplicity rules over performance. Maybe dww can run a db query on d.o and see roughly how many nodes are closed when the cron job runs. That might at least give us a sense of the scale we're talking about.
Comment #20
catchfwiw, testing.drupal.org probably has less than 1,000 patches total that it could test against either D6 or D7, and some of them will apply and not break any tests. Given the time to run all tests, I can't see it doing more than a handful of patches per hour - if it manages 4 x 24, that's 96/day (to test, not post about) - which will be quite enough once the backlog is cleared. So I don't reckon the performance issues of actually posting to issues are going to be the bottleneck.
Comment #21
boombatower CreditAttribution: boombatower commentedThe major point I was making is that the code either makes the same change to multiple nodes or only changes one node. The use-cases don't include change multiple nodes to different states.
Side note: I'm still not sure why having ($nodes, $changes) is more complicated as keeps being said over and over.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedbookmark
Comment #23
chx CreditAttribution: chx commentedIs this closer to "simpler"? I merely trimmed the patch above.
Comment #24
aclight CreditAttribution: aclight commentedIf nothing else the phpdoc needs to be corrected for project_issue_add_followup() given the changes in this patch. I'll keep this at CNR though so others will take a look.
Comment #25
chx CreditAttribution: chx commentedSo, I fixed the phpdoc. And aclight asked what did i change. Well, I created a function which adds a followup and does nothing else. Just one followup.
Comment #26
dwwYes, I think making the internal API in project_issue more simple is better. If the XML-RPC protocol between testing.d.o and d.o wants to send an array of nids and something interprets that to iterate over the array and invoke project_issue_add_followup() in a loop, that's fine. Just because the internal API is a certain way doesn't mean the network protocol has to work the same way.
That said, isn't t.d.o supposed to include a link to the test results for each patch? In that case, isn't the entire premise of the "we need an array of nids" argument moot? If the actual metadata is different for each followup, even if they have some overlapping data, there's no way to actually collapse all of it, anyway. Maybe I'm out of the loop on what kind of followups we were hoping to post. I suppose there's not much value in a link to the "results" that show the patch failing to apply.
That said, if we're going to be adding a new function for this, we should consider #218066-4: Prevent cross posting from reverting metadata fields as we design the new API.
Comment #27
chx CreditAttribution: chx commentedI am unsure what that issue has to do with this when the function takes an array of things to change... maybe renaming the argument to $followup was a mistake. Now it's back to $changes.
Comment #28
chx CreditAttribution: chx commentedWhat do I need to do to further the issue?
Comment #29
dwwSorry chx, I've been out of town for over 2 weeks, and am struggling to catch up on everything that piled in my absence. I hope to take a look at your recent efforts in the next few days. Stay tuned.
Comment #30
catchRe-rolled minus some lines which were left commented out.
Marked #198452: Enable test results on Drupal.org postponed based on this issue.
Comment #31
hunmonk CreditAttribution: hunmonk commentedpatch in #27 is the most recent i see, so this review is for that patch.
define('PROJECT_ISSUE_AUTO_CLOSE_DAYS', 14 * 24 * 60 * 60);
<-- if we're going to have that constant name, maybe we should do more likedefine('PROJECT_ISSUE_AUTO_CLOSE_DAYS', 14);
and then do the math when we use it?if ($followup_user = project_issue_get_$followup_user()) {
-- doesn't look right.$followup_username
,$followup_user
, and$followup_change_username
in function project_issue_settings_form, and i'm not sure what each does.db_query("INSERT INTO {comments} (cid, pid, nid, uid, subject, comment, hostname, timestamp, score, status, format, thread, users, name, mail, homepage) VALUES (%d, %d, %d, %d, '%s', '%s', '%s', %d, %d, %d, %d, '%s', '%s', '%s', '%s', '%s')", $comment);
-- this doesn't seem to match up w/ the way $comment was built.Comment #32
chx CreditAttribution: chx commentedI did a crap job and held up this important issue for another eight days. I am sorry. But, here is an another, cleaned up attempt. The only thing that was not changed is the project_issue_menu change, someone (me? not me? who knows) changed the table alias for {project_issues} to pi (it was p).
Comment #33
hunmonk CreditAttribution: hunmonk commentedalmost ready for testing. need to clean up these few things:
n.status = 1
in the WHERE clause that pulls the issues to auto close should be removed.Add a followup to one or more project issues.
still seems wrong in the doxygen for project_issue_add_followup() -- you can only pass changes for one issue at a time, right?$$comment['cid']
in comment insert query.variable_get('project_issue_followup_user', 0);
-- dww and i agree here that the default should be disabled (empty string)Comment #34
chx CreditAttribution: chx commentedComment #35
hunmonk CreditAttribution: hunmonk commentedi've committed the attached patch to 5.x-2.x and HEAD, after fixing these issues:
setting the issue back to active until this change is deployed on d.o
Comment #36
hunmonk CreditAttribution: hunmonk commenteddeployed on d.o
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.