GHOP #36: Auto-generate issue content

for the record, i'd be all in favor of changing the project_issue API to make this task easier, if that would help. however, i'd rather see this code in devel_generate.inc, (along with whatever changes to project_issue* we'd want to make it easier), than directly in this profile. therefore, i'm moving this issue into the devel issue queue so we can work on it there. if moshe would prefer a separate devel_generate_project.inc (for projects, issues, follow-ups, and releases), that'd be fine with me, too.

however, this (IMHO, critical) bug in the D5 installer that drupal isn't fully bootstrapped when you hit hook_profile_final() is the source of much pain and suffering. frankly, we should get to the bottom of that, file a critical bug against D5 with a patch, and hope that the core committers can be persuaded to fix this before 5.2.

i'd actually prefer that this ship as a module in the project module suite. devel can't be home to everyone's generate scripts. we'll do core object generation but not votingapi, actions, og, etc ...

that said, i'd love to see this done

some generate scripts in project* sounds very reasonable to me. i just didn't want it specific to the d.o testing profile...

we now have ifac. hope that followups are now easier to automate and not harder ... devel_generate can already create comments if we don't care about those comments actually changing project metadata.

belongs in project_issue queue

I've been doing some work as part of GHOP on this task and have been trying to get to the bottom of exactly why everything besides pid gets ignored. After a very long day of stepping through lots of code (being new to Drupal doesn't help) I somehow managed to get it to work. Currently more of a hack then a fix, but hopefully this will point us in the right direction. I'll be studying this some more tomorow night.

Index: form.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/form.inc,v
retrieving revision 1.174.2.11
diff -u -r1.174.2.11 form.inc
--- form.inc	26 Jul 2007 19:16:45 -0000	1.174.2.11
+++ form.inc	1 Dec 2007 04:37:28 -0000
@@ -150,6 +150,7 @@
  * drupal_execute('story_node_form', $values, $node);
  */
 function drupal_execute($form_id, $form_values) {
+  global $form_values;
   $args = func_get_args();
 
   $form_id = array_shift($args);
@@ -243,7 +244,7 @@
   // in earlier recursive calls.
   array_push($saved_globals, array($form_values, $form_submitted, $form_button_counter));
 
-  $form_values = array();
+  // $form_values = array();
   $form_submitted = FALSE;
   $form_button_counter = array(0, 0);
 

Here it is, creates random issues. :)

Sorry, re-uploaded with unix style line endings.

Great start, thanks. A few problems:

A) This should be a new project_issue_generate.module that lives in a project_issue/generate directory, not just part of the drupal.org testing install profile. It's possible other devel sites will want random issue content, not just ones using this install profile. This is more and more how the generate-random-XXXXX stuff is done. Ideally, there'd be an API function project_issue_generate_issues($num), in addition to an admin UI with a simple form (just like devel's core generate stuff for nodes, users, etc).

B) I don't understand what all this _drupalorg_testing_drupal_execute() code is for. Seems like a hack and a fork of similar code in core. Why can't you just use drupal_execute() itself? This doesn't look like a viable solution.

C) I'm a little worried about loading all the users into RAM like this. If you auto-generated 100,000 users, then ran this code, you'd probably start to hit the PHP/httpd memory limits. ;) Perhaps you should just limit it to the first N users, where N is another argument to the function, which defaults to 100 or something.

D) We also need random issue comments generated, since those can alter the issue meta data. Unfortunately, we can't just reuse devel's existing comment-generation functionality to fully test this stuff, since then we'd only see random comment text, but none of the issue settings getting altered as each comment comes in. See what I mean?

Thanks again!
-Derek

@dww: I agree with A and C from #12 above.

B) I don't understand what all this _drupalorg_testing_drupal_execute() code is for. Seems like a hack and a fork of similar code in core. Why can't you just use drupal_execute() itself? This doesn't look like a viable solution.

You're exactly right--it's a hack. When jvaill originally took this task (and even when webchick did this much earlier), both of them ran into the problem that drupal_execute() won't work to create project nodes. It will generate a project node, but none of the data for the node (ie. title, body, metadata) is filled out. This is pretty much useless. Because of this, he dropped the task.

In IRC, webchick and I were talking with jvaill and we both agreed that it would be acceptable (at least for GHOP purposes) if he forked the code in project_issue_generate.module. I took a look at drupal_execute also, and I wasn't able to figure a way to generate issues either without resorting to hacking it. If you have any suggestions that would be great, but I'm not sure it's possible to complete this task without forking drupal_execute. Webchick agreed that even if you wouldn't accept a patch with such a solution, that if the patch was otherwise acceptable that jvaill would get credit for the task.

D) We also need random issue comments generated, since those can alter the issue meta data. Unfortunately, we can't just reuse devel's existing comment-generation functionality to fully test this stuff, since then we'd only see random comment text, but none of the issue settings getting altered as each comment comes in. See what I mean?

Since jvaill had so much trouble with the first part of this task, I told him that auto-generating comments would be extra credit, and not required. Would you commit a patch that just generated the issues and then set this issue back to active so that later a comments generating patch could be created?

Ok, I think I did most of the changes requested . . It now lives as a seperate module along with an admin panel and a very simple api. I've changed the way everything works, instead of caching all the common items I now simply build a SQL query for every item. Obviously this might add some overhead but from the tests I conducted it really didn't seem that bad, it also allowed me make the code cleaner and resolve the issue of loading all users into memory first. As for the _drupal_testing_drupal_execute(), I agree it is a pretty nasty hack, but we really couldn't find another solution (other then resolving to raw SQL queries). Also, even though generating the metadata on comments wasen't really supposed to be part of the GHOP task, I've went ahead and added it anyways.

It isn't *entirely* tested yet but I was hoping to get some eyes on it before I go to bed... So please take a look at it and let me know what you think.

Also I apologize in advance for the patch, project_issue_generate.* belongs inside your project_issue/generate folder and the other file is a patch to your drupalorg_testing.profile file.

This is mostly looking great. I'm too tired to do a really careful review or test it, but this is definitely promising. I'm still bummed about the drupal_execute() madness, and would really like to understand that before we commit this code, but I know this is turning into a pretty nasty task for a measly tee-shirt. ;) So yeah, I can be flexible about that...

Some minor gripes on a quick skim of the code:

A) project_issue_generate.info -- this module should actually go into the "Development" package, as per http://drupal.org/node/193104

B) why is // if ($may_cache) { commented out in project_issue_generate_menu()?

C) why are the menu callbacks for generating issues and comments set to this access level: user_access('administer users')?

D) typo: "aswell"

E) I know I'm the one that suggested loading everything into RAM might be trouble, but this approach with all the individual DB queries scares me. Granted, the idea is you only run this on a test site, and only infrequently, so the performance shouldn't matter that much. But, this generates *A LOT* of queries. ;) I'm not sure that's such a good idea. All I was asking for was a limit on the size of some of the arrays you were creating with the previous approach. This is increasing by a few orders of magnitude the number of individual queries being run. Seems like a good way to bring a DB server to its knees, especially on a shared hosting account that limits DB traffic and penalizes users that are doing too many queries...

F) I understand there's treachery for the drupal_execute() madness, but there are no comments explaining it at all. :( It's a nearly exact cut/paste of the code, but I don't know what was changed, or why. If we're going to keep this hack in here at all, we should definitely add some comments why.

G) We don't use the closing ?> in our code (for good reason -- search for a pointer if it's not already explained in the coding guidelines).

H) (no idea if this is real, haven't tested, but a brief look at the code suggests it might be a problem) isn't it possible for the random issues to create invalid results? for example, what if there's access control on the site, so most users only have "access own project issues" permission? couldn't users comment on other users issues when they normally wouldn't be allowed to do so? or is that going to hit form validation errors and spew warnings if you tweaked the permissions? same thing with "assigned". currently, there are restrictions in the code on how issues can be assigned by new comments. it might be confusing if users run a test site to generate issues/comments and end up with results that are impossible by using the module normally.

anyway, sorry if i'm being too harsh in my review, but that's my job. ;) webchick or aclight can come in here to slap some sense into me and say "c'mon dww, give 'em a break, it's only a lousy tee-shirt!". i'm just very careful with code related to project*, since it's mission critical for drupal.org itself...

I also don't have time for a thorough review right now, but here are some additional comments and some replies to dww's comments:

A. You need to use UNIX line endings. I know you're using windows--what text editor are you using. Maybe we can help you figure out what setting you need to change to force UNIX line endings (ie. "\n").

B. Webchick just recently enabled .zip attachments (I think) but you shouldn't attach patches as .zip files. You should attach a patch that includes all files in the patch. I'm not sure if you're creating patches with CVS or just with patch. If you're using patch to create patches, I don't know if it's possible to make a patch that contains new files. But if you're using CVS, it's possible. I have only done this using the cvs command line tool, but here's how I do it:

1. Edit /var/www/drupal/sites/all/modules/project_issue/generate/CVS/Entries and add a line like the following for each new file you want to be included in the patch to the end of the Entries file:
   

   /project_issue_generate.inc/0/New file//
   

2. cd /var/www/drupal/sites/all/modules
3. cvs diff -NuRp project_issue/ > project_issue_generate_issues.patch
   You can check out http://drupal.org/patch/create for more information. There must be instructions for TortioseCVS somewhere in the handbook (if you can 't find any, let me know--that might be another good GHOP task for someone).

As for dww's comments, I agree with all of them. I think limiting the pool of users that will be used to generate projects is a good idea. I think you could, for example, limit to 100 users, but randomize which 100 users (probably just randomly pick a uid where you start your LIMIT statement, so like:

<pseudocode>
$first_user = random uid that is > 0 and <= $num_users - $num_users_to_fetch
$sql = "...... LIMIT $first_user, $first_user + $num_users_to_fetch"
</pseudocode>

It might also be possible to use an SQL statement to pull 100 random UIDs, but if so I don't know how off the top of my head.

Regarding dww's last point about access control modules and invalid issues, that's something good to keep in mind. If this is a problem with your current patch, that's probably something that should be fixed. Off of the top of my head, the only thing I can think of to get around that is to randomly pick a user, then check the permissions of the user to see if that user would be able to generate an issue for the given project, and if not then try again. Depending on how the user permissions are set up, you could end up having only a few users, or even no users, (out of the 100 that you picked as possible uids) that could create an issue. So you'll need to have some kind of timeout or escape block that gives up if you get in such a situation so that the process doesn't go on forever.

I'll likely be in #drupal-ghop today to answer any questions, etc., if your classes get cancelled and you want to code. Just ping me.

As dww said, nice work on this. I think we're getting pretty close to a very nice solution!

Ok, I finally got a chance to test this out, and it rocks.

Apart from the things mentioned above, there are a few other problems:

A. In project_issue_generate.inc, line 71, :

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

has an extra period before /issue.inc and should be

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

B. project_issue_generate needs to be listed in the array of contributed modules that are automatically enabled in drupalorg_testing.profile.

C. When comments are auto generated, there is no message set indicating that they were generated correctly.

D. The drupalorg_testing.profile patch probably should be generated as a separate issue in that projects issue queue and the patch for that module should go there. Personally, I'm fine with creating two patches attached to this issue, and then once the project_issue_generate stuff gets committed we can just create a quick issue at the drupalorg testing profile with that patch attached. But if someone else thinks that we really need to keep these completely separate, speak up.

I tested this on a clean install of the drupalorg testing profile and, after fixing the above things, this absolutely rocks. There are still a few issues to clear up, as mentioned here and above, but this is definitely good work and we're on our way to getting automatic issue and comment generation. This will make the drupalorg testing profile much more useful, especially when working on the project_issue views conversion.

Keep up the great work.

I maintain both project_issue and the d.o testing install profile. I'm happy to handle all of it in this single issue, and if it's easier for jvaill to generate them as separate patches, that's great, but don't split them up on my account. ;)

Otherwise, yes, this is great. I'm *so* happy to see this getting done!!!

Thanks,
-Derek

Attached is all the files bundled up . . sorry for lack of a proper patch again.

Afaik all of the issues are fixed except for the permissions. I've been trying to make this work properly with permissions since last night but just can't get it to work properly.

For starters, I thought that grabbing the users with an inner join to the roles table would do it. Turns out not all users have roles in the roles table. This causes problems because there are special roles (ie : "authenticated") that don't get set within the roles table. Things like these have been leaving me hanging.

Then there's the statuses permissions.. this is where it really gets tricky. Each status is a different permission.. The statuses could be spread out between different roles and a user could belong to none or even all of these roles. I just haven't been able to come up with a feasable way of making this work.

Hopefully someone can give me some pointers, that would be great.

first pointer: you can attach multiple files to a single issue comment. please do that instead of zip files, since that way people can review the files directly on the site (from their phone, perhaps), without downloading and uncompressing the .zip, etc. thanks.

certainly d.o, and probably many sites, give wide open access to create issues in all status values for all authenticated users, and open access to view everything to anonymous. so, if the very first release of the issue/comment generator only created valid results for that configuration, it'd already be a huge help.

the next version could add support for a more well-defined set of roles/permissions, and generate random content for 2 or 3 users from each role, within their specific access to issues (own vs. all), and status values.

thanks,
-derek

first pointer: you can attach multiple files to a single issue comment. please do that instead of zip files, since that way people can review the files directly on the site (from their phone, perhaps), without downloading and uncompressing the .zip, etc. thanks.

This is partially my fault. I was talking with jvaill last night in IRC and he was having problems making a patch and I suggested he just zip them and post that so at least I had something to take a look at.

I still forget myself that we can attach multiple files. I've unzipped them all and attached them directly. No changes at all to the files themselves.

I took a look at the work so far and got a little closer to getting this to work. Unfortunately, it doesn't quite work yet.

The access control stuff of this is pretty confusing to test (whether the statuses are being set in a way that the user has permission to). Right now, issues are being set to states that they shouldn't be. It's probably something stupid that I'm not noticing.

Also, I'm not sure that the assigned to is working right. Issues should only be assignable to the author or to unassigned, but I'm not sure that's actually happening.

Anyway, jvaill can take a look at this and see if it makes sense, and perhaps you can figure out where my bug is. But I think this code should be fine performance wise and memory wise.

I've pasted the functions below that have changed from jvaill's files in #21. If you think I've left something out, let me know.

From issue.inc:

/**
 * Return information about project issue state values.
 *
 * @param $sid
 *   Integer state id to return information about, or 0 for all states.
 * @param $restrict
 *   Boolean to determine if states should be restricted based on the
 *   permissions of the current user or $account if that parameter
 *   is provided.
 * @param $is_author
 *   Boolean that indicates if the current user is the author of the
 *   issue, which can potentially grant a wider selection of states.
 * @param $current_sid
 *   The current integer state id for the issue.
 * @param $defaults
 *   Boolean to request the states used for default issue queries.
 * @param $account
 *   Account of a user to pass to user_access() for access checking.
 *   This parameter will have no effect unless $restrict is also
 *   set to TRUE.
 *
 * @return
 *   An array of states (sid as key, name as value) that match the
 *   given filters, or the name of the requested state.
 */
function project_issue_state($sid = 0, $restrict = false, $is_author = false, $current_sid = 0, $defaults = false, $account = NULL) {
  static $options;

  if (!$options) {
    $result = db_query('SELECT * FROM {project_issue_state} ORDER BY weight');
    while ($state = db_fetch_object($result)) {
      $options[] = $state;
    }
  }

  foreach($options as $state) {
    if ($restrict) {
      // Check if user has access,
      // or if status is default status and therefore available to all,
      // or if user is original issue author and author has access,
      // or if the issue is already in a state, even if the user doesn't have
      //   access to that state themselves.
      if (user_access('set issue status '. str_replace("'", "", $state->name), $account)
          || user_access('administer projects', $account)
          || ($state->sid == variable_get('project_issue_default_state', 1))
          || ($state->author_has && $is_author)
          || ($state->sid == $current_sid) ) {
        $states[$state->sid] = $state->name;
      }
    }
    else if ($defaults) {
      if ($state->default_query) {
        $states[$state->sid] = $state->name;
      }
    }
    else {
      $states[$state->sid] = $state->name;
    }
  }
  return $sid ? $states[$sid] : $states;
}

From project_issue_generate.inc:

function project_issue_generate_issues($num) {
  require_once(drupal_get_path('module', 'devel') .'/devel_generate.inc');

  $projects = _project_issue_generate_get_field('projects');
  $categories = _project_issue_generate_get_field('categories');
  $priorities = _project_issue_generate_get_field('priorities');
  $users = _project_issue_generate_get_field('users');
  $loaded_users = array();
  for ($i = 0; $i < $num; $i++) {
    srand((double) microtime() * 1000000);

    $project = $projects[array_rand($projects)];
    $components = unserialize($project->components);
    $issue = array();
    $issue['pid'] = $project->nid;
    $issue['category'] = $categories[array_rand($categories)];
    $issue['component'] = $components[array_rand($components)];
    $issue['priority'] = array_rand($priorities);
    $issue['title'] = _project_issue_generate_random_field($pid, 'title');
    $issue['body'] = _project_issue_generate_random_field($pid, 'body');

    // The user must be chosen before status (sid) so that we can make sure
    // that the status is set such that the user would have permission to
    // set the status as such.
    $account = user_load(array('uid' => $users[array_rand($users)]->uid));
    $issue['name'] = $account->name;
    $permitted_sids = _project_issue_get_permitted_sids($account);
    $issue['sid'] = array_rand(_project_issue_get_permitted_sids($account));
    if (!isset($issue['sid'])) {
      unset($issue);
      continue;
    }

    // Currently, project_issue nodes can either be unassigned or assigned
    // to the user creating the project_issue node (or in the case of comments
    // the user creating the comment).
    $possible_assignments = array(0, $account->uid);
    $issue['assigned'] = $possible_assignments[array_rand($possible_assignments)];

    _project_issue_generate_drupal_execute('project_issue_node_form', $issue, array('type' => 'project_issue'));
  }
}

function _project_issue_generate_get_field($field, $pool_size = 100) {
  require_once(drupal_get_path('module', 'project_issue') . '/issue.inc');

  switch($field) {
    case 'projects':
      $projects = array();

      $result = db_query('SELECT nid, components FROM {project_issue_projects}');
      while ($project = db_fetch_object($result)) {
        $projects[] = $project;
      }

      return $projects;

    case 'categories':
      $categories = array_keys(project_issue_category());
      return $categories;

    case 'priorities':
      $priorities = project_issue_priority();
      return $priorities;

    case 'users':
      // Determine what role ids have permission to create project_issue nodes.
      $users = array();
      $allowed_roles = user_roles(FALSE, 'create project issues');

      // If any authenticated user can create project_issue nodes,
      // then there is no need for an INNER JOIN in our query.
      // Otherwise, the query needs to INNER JOIN on the users
      // table so that only users with roles that are allowed to
      // create project_issue nodes are selected.
      if (isset($allowed_roles[DRUPAL_AUTHENTICATED_RID])) {
        $join = '';
        $where = '';
      }
      else {
        $join = 'INNER JOIN {users_roles} ur ON u.uid = ur.uid';
        $where = "WHERE ur.rid IN (". implode(', ', array_keys($allowed_roles)) .")";
      }
      $sql = "SELECT u.uid FROM {users} u $join $where ORDER BY RAND() LIMIT %d";
      $result = db_query($sql, $pool_size);
      while ($user = db_fetch_object($result)) {
        $users[] = $user;
      }
      return $users;

    case 'statuses':
      $statuses = array();

      $result = db_query('SELECT sid FROM {project_issue_state}');
      while ($status = db_fetch_object($result)) {
        $statuses[] = $status->sid;
      }

      return $statuses;
  }
}

function _project_issue_get_permitted_sids($user) {
  static $permitted_states;

  if (!isset($permitted_states)) {
    $permitted_states = array();
  }

  if (isset($user->uid)) {
    if (isset($permitted_states[$user->uid])) {
      return $permitted_states[$user->uid];
    }
    else {
      $states = project_issue_state($sid = 0, TRUE, TRUE, 0, FALSE, $user);
      $permitted_states[$user->uid] = $states;
      return $states;
    }
  }
  return array();
}

Other comments about the work so far:
A. Since we're keeping the scope of this issue to generating project_issue nodes only (and not comments on project_issue nodes), you need to pull out the comment generation stuff for now.

B. It doesn't look like you've addressed the point about commenting on why we're forking drupal functions (made by dww in comment #15, point F).

C. I think there are several cases in _project_issue_generate_random_field() that are no longer necessary.

Hm....actually, I think the code above is OK. It turns out I had my access permissions set differently than I thought I did.

Tested it and seems to be working perfectly !

I cannot thank you *ENOUGH* for this !

Attached is the current patch with all of these changes minus the comment generation stuff.

Proper patch.

Unix line endings this time ^o.

Updated with latest issue.inc revision.

Hopefully last patch !

Yay!!!

Nice work. I've tested this with a fresh drupalorg_testing.profile install and project_issues were created correctly upon installation.

Furthermore, issues created later on from the admin pages were created properly.

Issues are not created by users that do not have permissions to create issues. Likewise, statuses are appropriate to the permission of the user creating the issue.

Setting this to RTBC. I hope you enjoy your T-shirt. You sure earned it!

Woot! Tested myself, and committed to HEAD and DRUPAL-5 with these tiny modifications:

1. Make sure project_issue_generate.inc exists (i.e., someone has the very latest project_issue code) before we try to use it.
   

   function _drupalorg_testing_create_issues() {
     $file = drupal_get_path('module', 'project_issue_generate') .'/project_issue_generate.inc';
     if (file_exists($file)) {   // <== 
       require_once($file);
       project_issue_generate_issues(50);
     }
   }
   

2. Put project_issue_generate.* in a "generate" subdir of modules/project_issue
3. s/dummy/random/ in the description in project_issue_generate.info

Great work, thanks!
-Derek

I just asked chx if he had any bright ideas about the drupal_execute() hacks in here. He pointed out that the issue form is usually a multi-page form. So, if you're trying to execute the form and don't specify that we're on page #2, the form builder only thinks you want to see the project id as a form element, nothing else. This matches perfectly why pid was saved but nothing else. Should be relatively easy to fix this, then we can remove the evil hacks in here.

This almost entirely works. All we needed to do was pass the pid to the $node object in the form builder, by way of the final array of arguments to drupal_execute(). The only problem is I got errors on the 'assigned' stuff ("An illegal choice was detected") so something fishy is happening there. Commenting out that line and everything else is working. Anyone have a chance to look into the assigned stuff?

This patch seems to work.

Not quite. That patch never assigned issues to anyone since you left part of it commented out. Try this.

code looks good. works as advertised.

Committed to HEAD and DRUPAL-5. Thanks.