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 ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Sounds good to me.

solotandem’s picture

Version: 6.x-1.x-dev » 5.x-2.x-dev
Assigned: Unassigned » solotandem

I am currently working on this.

I assume we are writing this for 5.x and it can be included in a port?

solotandem’s picture

Status: Active » Needs review
FileSize
11.05 KB

To 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.

aclight’s picture

Status: Needs review » Needs work

A 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.

  1. We can't necessarily count on the sid of FIXED to be 2 nor on the sid of CLOSED to be 7. Site admins can add, delete, etc. different statuses in the project_issue settings page, so we don't want to hard code these. I know the current code is hard coded, but if we're abstracting all of this we should do it properly.
    +define('PROJECT_ISSUE_STATE_FIXED', 2);
    +define('PROJECT_ISSUE_STATE_CLOSED', 7);
    
  2. You're adding one variable and deleting another variable, but you haven't added the later to hook_uninstall so that it's deleted with the module is uninstalled. If we're getting rid of the project_issue_auto_close_user variable, you'll also want to copy that value over to project_issue_auto_comment_user and then delete project_issue_auto_close_user. This should be done in an update_N function in project_issue.install.
    -  $uid = variable_get('project_issue_auto_close_user', 'anon');
    +  $uid = variable_get('project_issue_auto_comment_user', 'anon');
    
  3. If you're changing the name of the variable, you need to make sure to change it everywhere. You need to change #title here as well as #default_value.
    -  $form['project_issue_auto_close_user'] = array(
    +  $form['project_issue_auto_comment_user'] = array(
         '#title' => t('Auto-close user'),
         '#type' => 'textfield',
         '#default_value' => $auto_close_username,
    
  4. In cases where you notice coding style mistakes and you're changing the line anyway, I think it's ok to go ahead and fix the code style at the same time. For example, we need a space between the if and the ( here:
    +    if(!$auto_user) {
    
  5. This comment needs to be updated
         // Safety check -- selected user must still have the correct permissions to auto-close issues.
    
  6. Many of the functions you've added are either lacking phpdoc or it's formatted incorrectly. For example, project_issue_get_auto_comment_user() needs an explanation of the return value. project_issue_auto_comment() has phpdoc but we don't put the parameter and the description on the same line (though there are counterexamples of this I'm sure, but that's old code for the most part that just hasn't been updated). So instead of
    @param integer $nid The node ID to auto comment.
    

    we'd have

    @param $nid
      $nid The node ID to auto comment.
    
  7. I think that while we're messing with this code, we need to eliminate the hard coded time in project_issue_auto_close() and add that as an administrative option on the project issue settings page.
  8. For the various db queries, let's alias the {project_issues} table as pi, not p. I know that it currently is aliased as p, but I don't think that makes any sense and we might as well improve readability of the code if we can.
  9. You added the PROJECT_ISSUE_STATE_FIXED constant but then you don't even use it. But as I said earlier, I think we need to not hard code those state values, even as constants.
      +    $result = db_query('SELECT p.nid FROM {project_issues} p INNER JOIN {node} n ON n.nid = p.nid WHERE n.status = 1 AND p.sid = 2 AND n.changed < %d', time() - 14 * 24 * 60 * 60);
      
  10. This line is very obtuse. I don't see how the old way this was done is any worse, and I think it makes it more obvious what is going on.
    
    
      +      $clear_cache = $clear_cache || project_issue_insert_auto_comment($issue->nid, $sid, $comment);
      
  11. + * Automatically comment an issue. should be "comment *on* an issue" or "add a comment to an issue".
  12. I don't see that you're actually calling this function. Am I missing something or is this cruft?
      +function project_issue_auto_comment($nid, $sid, $message) {
        
  13. It's not immediately clear to me which function should be the public API function that other modules, etc. should call and which functions are private functions that should not be called by other modules. Perhaps we can clarify this both in phpdoc and by renaming any private functions to be _project_issue_.....
solotandem’s picture

FileSize
18.39 KB

Response 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.

aclight’s picture

@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:

-// $Name:  $
+// $Name: DRUPAL-5--2 $

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.

aclight’s picture

I 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:

/**
 * Replace the project_issue_auto_close_user variable with
 * project_issue_auto_comment_user, if the former exists.
 */
function project_issue_update_5208() {
  $ret = array();
  $uid = variable_get('project_issue_auto_close_user', NULL);
  if (isset($uid)) {
    variable_set('project_issue_auto_comment_user', $uid);
    variable_del('project_issue_auto_close_user');
  }
  return $ret;
}

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:

+/**
+ * Automatically add a comment to an issue.
+ *
+ * @param $nids
+ *   The node IDs to auto comment.
+ * @param $sid
+ *   The project issue status ID to set on each node.
+ * @param $message
+ *   The message text.
+ */

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,

$comments = array(
  'nid' => 100,
  'sid' => 4,
  'message' => t('This issue was automatically closed after 2 weeks of no activity.'),
);

In this case, the function signature would look like:

+function project_issue_auto_comment($comments = array()) {

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.

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.

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.

dww’s picture

Cool, 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!

solotandem’s picture

Thanks 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:

$comments = array(
  100 => array(
    'sid' => 4,
    'comment' => t('This issue was automatically closed after 2 weeks of no activity.'),
  );
);

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.

aclight’s picture

A. 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.

solotandem’s picture

FileSize
21.21 KB

This 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.

aclight’s picture

One 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.

solotandem’s picture

FileSize
22.84 KB

This 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.

aclight’s picture

One 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.

catch’s picture

If 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.

pwolanin’s picture

Since 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?

dww’s picture

For 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

boombatower’s picture

After 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:

  • Built in project use for closing issues after two weeks. In this case a large number of nodes are all changed to the same state with the same message upon execution of cron.
  • Remote testing server builds list of patches that don't apply well, or that fail tests and marks them in batches accordingly. We definitely do not want to make hundreds, even thousands, of calls to the server with individual changes, or even a giant array of changes for each node as that just makes a huge packet.

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.

aclight’s picture

One 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.

catch’s picture

fwiw, 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.

boombatower’s picture

The 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.

Anonymous’s picture

bookmark

chx’s picture

Assigned: solotandem » chx
Status: Needs work » Needs review
FileSize
16.79 KB

Is this closer to "simpler"? I merely trimmed the patch above.

aclight’s picture

If 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.

chx’s picture

FileSize
16.74 KB

So, 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.

dww’s picture

Status: Needs review » Needs work

Yes, 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.

chx’s picture

Status: Needs work » Needs review
FileSize
16.72 KB

I 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.

chx’s picture

What do I need to do to further the issue?

dww’s picture

Sorry 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.

catch’s picture

Re-rolled minus some lines which were left commented out.

Marked #198452: Enable test results on Drupal.org postponed based on this issue.

hunmonk’s picture

Status: Needs review » Needs work

patch in #27 is the most recent i see, so this review is for that patch.

  1. there's commented out code in various places that needs to be removed.
  2. define('PROJECT_ISSUE_AUTO_CLOSE_DAYS', 14 * 24 * 60 * 60); <-- if we're going to have that constant name, maybe we should do more like define('PROJECT_ISSUE_AUTO_CLOSE_DAYS', 14); and then do the math when we use it?
  3. if ($followup_user = project_issue_get_$followup_user()) { -- doesn't look right.
  4. i see $followup_username, $followup_user, and $followup_change_username in function project_issue_settings_form, and i'm not sure what each does.
  5. the default value of 'project_issue_followup_user' looks like it would always be an empty string, which doesn't seem right.
  6. description on 'project_issue_followup_user' looks wrong.
  7. i think the description on 'project_issue_auto_close_days' could be made more clear.
  8. PROJECT_ISSUE_AUTO_CLOSE_DAYS is declared in seconds, but the form is asking for the value in days, and function project_issue_auto_close is also expecting that constant in days.
  9. doxygen for function project_issue_add_followup() says 'An array of keyed arrays of project issue fields to change.', but this doesn't seem accurate.
  10. 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.
  11. there's a change in function project_issue_menu(), but i'm not clear what it is from reading the patch.
chx’s picture

Status: Needs work » Needs review
FileSize
17.72 KB

I 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).

hunmonk’s picture

Status: Needs review » Needs work

almost ready for testing. need to clean up these few things:

  1. staet -> state in project_issue_auto_close_days form element description
  2. talked it over w/ dww, and we feel the n.status = 1 in the WHERE clause that pulls the issues to auto close should be removed.
  3. 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?
  4. specified -> specifies in the doxygen for for project_issue_add_followup()
  5. $$comment['cid'] in comment insert query.
  6. variable_get('project_issue_followup_user', 0); -- dww and i agree here that the default should be disabled (empty string)
chx’s picture

Status: Needs work » Needs review
FileSize
17.93 KB
hunmonk’s picture

Assigned: chx » hunmonk
Status: Needs review » Active
FileSize
17.51 KB

i've committed the attached patch to 5.x-2.x and HEAD, after fixing these issues:

  1. db update needed an extra check to convert possible 'anon' value to 0
  2. no default $comment['hostname'] value defined
  3. tightened up the validation checks surrounding the anonymous user
  4. Auto-change -> Auto-followup for consistency
  5. caste default value of auto-close days to an int for simple validation purposes
  6. ensure $comment['project_info'] exists before trying to merge into it

setting the issue back to active until this change is deployed on d.o

hunmonk’s picture

Status: Active » Fixed

deployed on d.o

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.