GHOP #79: Auto generate project_issue comments

There it goes... :)

Sweet, thanks! Haven't tested, but a quick review of the patch:

A) This should just be a patch against the existing project_issue/generate directory. No need for a separate module just for generating comments. We only split these two tasks since the whole thing was getting too complicated, not because these are fundamentally separate operations. ;) I'd still keep a separate function for generating the comments, but I'd probably just extend the existing menu item for generating stuff, and have 2 boxes (number of issues, number of comments) on the form. Then, the submit handler just invokes both the issue-generator and the comment-generator. Make sense?

B) code style: $comment=array(); needs spaces around the '='. same on the very next line in the patch.

C) I'd don't immediately understand what this outer test is for: if (rand(0,10)) { ... Can you either add a comment to the code for why that's necessary, or just remove it?

D) the logic overall doesn't quite make sense to me, either (maybe i'm just being dumb since i'm tired). why do we go through mucking with the $issues array, then construct the comment separately? why not just populate the $comment array directly with our various random selections?

E) db_query("UPDATE {comments} SET uid='{$account->uid}' WHERE cid='{$newcid}'"); this needs to use DB placeholders to ensure everything is properly escaped to prevent SQL injection. in this case, there's no actual vulnerability, but you should always be in the habit of using db_query() as designed. see the docs for details, or just ask if this isn't clear.

F) drupal_set_message(t('Your <em>comment</em> has been created.')); -- in general, it's a bad idea to put HTML directly into t() like this. you can just use a t() placeholder, and it'll get theme('placeholder') applied to it automatically. like so:

drupal_set_message(t('Your %comment has been created.', array('%comment' => t('comment')));

It's a little more to type, but it's better for sites that want to implement theme('placeholder') themselves, and it makes it slightly easier to translate since the t('comment') part is already translated elsewhere...

G) No need to fork/duplicate function _project_issue_generate_get_field() -- see point (A) above.

H) It'd be nice if some of the comments had no body, just meta data changes, so it'd be cool to wrap that in a rand() check, too, to see if we want to populate that field or not.

This is going to be a great when it's done... Thanks!

p.s. same story with the patch against the d.o testing profile -- just stuff the call to project_issue_generate_comments() into _drupalorg_testing_create_issues() instead of making it a whole separate function. thanks!

Uhm... You're right. I did a lot of modifications, including some aclight suggested me in IRC. See it again, I hope you like more than in the first time. :)

D) the logic overall doesn't quite make sense to me, either (maybe i'm just being dumb since i'm tired). why do we go through mucking with the $issues array, then construct the comment separately? why not just populate the $comment array directly with our various random selections?

If I don't keep everything in $issues array, in the next time I comment in a post, if rand(0,1)==0 I'll back that field to what it was in the beginning, not let it like it is. (I don't know if I was clear, anything you can ask)

Forgot to set status ;)

But another thing: sometimes (completely randomly) the comments are not generated and I get this message: "An illegal choice has been detected. Please contact the site administrator."
Do you know what is this and how can I fix it?

A) hrm, it seems i wasn't clear with this:

I'd still keep a separate function for generating the comments, but I'd probably just extend the existing menu item for generating stuff, and have 2 boxes (number of issues, number of comments) on the form. Then, the submit handler just invokes both the issue-generator and the comment-generator. Make sense?

I liked the fact that there was a separate function specifically for generating the comments, I just didn't like having a whole separate module for it. So, it'd be good if the comment generation went back into a separate project_issue_generate_issue_comments() function, so that you can separately generate issues or comments. But, the admin UI to generate just asks for both numbers on a single form, and the submit handler will invoke each function with the appropriate argument.

B) Ok, I see what you're saying now about updating $issues and then generating the comment based on what changed. All that really accomplishes is that if we decided (randomly) not to alter a given field, that we use the existing value. I guess that works. However, can you please add a comment about this in code itself? The alternative would be to *always* take a random value for each of the issue metadata things (perhaps except the project), and then if the comment randomly selects the existing value, it didn't change, otherwise, it does. see what i mean? doesn't matter if the random comments move an issue around in a strange order of status values, or back and forth between critical/normal/minor all the time...

C) Always use {} around your if clauses, even if it's just a single line:

+    if (rand(0,1))
+      $comment['comment'] = devel_create_content();
+    else
+      $comment['comment'] = '';

This is very error prone, since if anyone adds another line and forgets to do the {}, you get totally unexpected behavior. Plus, the {}s helps keep it more readable IMHO.

D) It'd be nice if it randomly changed the component within the same project, instead of only altering the component if it's switching projects.

E) code style (spaces around '='): n.nid=p.nid

F) More code style: $issues[]=$result;

There it goes, again.
I hope no mistakes now...

After applying the patch:

Parse error: parse error in .../modules/project_issue/generate/project_issue_generate.inc on line 129

Uhm.. it's probably because your PHP version. Anyway, here is one more patch. :)
I won't post the profile patch, you can still use this: http://drupal.org/files/issues/drupalorg_testing_1.patch

lol
The upload didn't work =/

I don't know why I had a blank generatecomments.patch... anyway, there it goes now (i hope) lol

Definitely getting closer! ;)

A) you're not prefixing the DB table in this query:

+    $result = db_query("SELECT MAX(cid) AS cid FROM comments");

should be:

+    $result = db_query("SELECT MAX(cid) AS cid FROM {comments}");

B) it'd be nice if the drupal_set_message() said "200 comments created" instead of "your comments have been created".

C) minor code style gripe: $number would be a better argument name than $num

D) similarly, $form['issues'] and $form['comments'] would be clearer than $form['num'] and $form['cnum'].

E) I just noticed, do we really want/need to re-seed the random number generator on each loop iteration? I'm not sure, I'm open to an argument why this is important, but it seems wasteful to me. It's not like we're doing monte carlo simulation or cryptography where we *really* need good random numbers. ;)

F) You don't want to put quotes around integers in your SQL queries:

+    db_query("UPDATE {comments} SET uid='%d' WHERE cid='%d'", $account->uid, $newcid);
+    db_query("UPDATE {project_issue_comments} SET assigned='%d' WHERE cid='%d'", $account->uid, $newcid);

G) Actually, these queries scare me. It's not clear why you're manually messing with this stuff. I see your comment about it in the code, but have you tried to debug this? It'd be vastly better to understand why it doesn't work directly via the drupal_execute() than do a bunch of effort to directly manipulate the DB after the fact...

H) It's not clear to me why this works without a full path:

require_once('project_issue_generate.inc');

I) Code style:

drupal_get_path('module', 'project_issue') . '/issue.inc');

no space between '.' and the string literal, so it should be:

drupal_get_path('module', 'project_issue') .'/issue.inc');

Just so you know, I'm being so detailed in the reviews because that's what it's like in the Drupal community. ;) We take our code very seriously, and even the tiniest little code style problems can cause a delay getting a patch into core. So, the more you're used to the fine-toothed comb going over your work, the easier it'll be to contribute in the future, since you'll just automatically write it up to standards in the first place, and no one will be able to find the slightest flaw in your code. ;)

Thanks!
-Derek

Everything corrected, again. :)
I've put a comment to explain your (G).

Ok, getting very close now. ;) I think I'm down to 2 complaints -- 1 minor, 1 more substantial:

A) [minor] Previously, you re-seeded the random number generator on every iteration. Now, you don't seed it at all. ;) Can we compromise? How about once at the top of each generate function? ;)

B) [major] Ok, I'm really not down with how we're handling the "assigned" field here. The logic in your patch is actually buggy:

    // We can't determine the user ID, because if so a normal user could
    // hack the system by passing a UID by post in comment_form.
    drupal_execute('comment_form', $comment, array('pid' => '', 'nid' => $issues[$k]->nid));

    // We can't assign it before because it causes "an illegal choice".
    if (rand(0,1)) {
      // dww: NOTE: we only randomly update our issue array that it was assigned to the new user here.
      $issues[$k]->assigned = $account->uid;
    }
    $result = db_query("SELECT MAX(cid) AS cid FROM {comments}");
    $newres = db_fetch_object($result);
    $newcid = $newres->cid;
    // dww: however, you're always setting the comment to be owned by the current user.
    db_query("UPDATE {comments} SET uid=%d WHERE cid=%d", $account->uid, $newcid);
    // dww: and, you're ALWAYS setting the comment to be trying to assign the issue to the current user here.
    db_query("UPDATE {project_issue_comments} SET assigned=%d WHERE cid=%d", $account->uid, $newcid);
    // dww: and you never update the issue itself to be assigned to this user...
  }

So, what you have is buggy. You randomly update the array of issues to think it was assigned, but then *always* update the comment to try to assign, and never update the issue to know it was re-assigned. :(

However, all of this is madness. ;)

First of all, it seems like you should always at least be able to set $comment['uid'] to the current user, so that the comment itself thinks it was generated by the right user, no?

Honestly, I'd rather punt on messing with who the issues are assigned to than doing this. The root of the problem here is that the form builder for the comment/issue form thinks the only valid choices for assigned are the current uid of the global $user object, or "unassigned". That's why you get the validation errors and "illegal choice has been detected" stuff. I can imagine a few possible solutions to that problem (e.g. via hook_form_alter()), but I'm not sure it's worth dragging this GHOP task into that complication right now. Also note that the functionality of the 'assigned' field is currently very limited and hopefully going to go through some changes in the near future, anyway: http://drupal.org/node/4354

So, for this patch, let's just do the following:
- set $comment['uid'] appropriately based on the random user we're generating the comment as
- set $comment['assigned'] to 0 (unassigned)
- rip out everything else related to assigned from this patch

We can review, test and commit that as a first step. Then, we can work on a followup patch to cleanly deal with the assigned field. Sound good?

Obrigado!
-Derek

Sounds great, and the code looks better without the assignment stuff. :) Here is the new patch: I've put the srand before the loop and I've set $comment['uid'].

When I set $comment['assigned'] to 0 I was still receiving some "illegal choices"... so I just didn't create $comment['assigned'], ok?

Ready to be committed?
(please say yes... :D lol)

Thanks for the patience of reviewing million of times... :)
Tiago

Committed to HEAD. ;)

There were a small handful of very minor problems left, and I didn't think the code comments were all that clear, but English isn't your first language so I figured it was easier to just clean it up myself in a follow-up commit:

http://drupal.org/cvs?commit=91507

Great job on this, thanks!

p.s. See http://drupal.org/node/201367 about adding support to reassign issues when generating the random comments. again, it'd be great if you were willing to work on that, but I'm considering that separate from this GHOP task. Thanks.