Tests for Storm Expense

+++ ./stormexpense/stormexpense.module	2011-01-10 16:03:36.321121580 -0500
@@ -382,44 +386,50 @@ function stormexpense_form(&$node) {
+if (isset($node->project_nid)) {
+    $form['group1']['project_nid'] = array(
+      '#type' => 'select',
+      '#title' => t('Project'),
+      '#default_value' => $node->project_nid,
+      '#options' => $projects,
+      '#process' => array('storm_dependent_select_process'),
+      '#attributes' => array('onchange' => "stormticket_project_task_tickets(this, 'edit-organization-nid', 'edit-task-nid', 'edit-ticket-nid', true, '-')"),
+    );

This should be done like this so that on the node add form (where the $node variable is not yet fully populated) the field still shows up properly.

+++ ./stormexpense/stormexpense.module	2011-01-10 16:03:36.321121580 -0500
@@ -382,44 +386,50 @@ function stormexpense_form(&$node) {
+  $form['group1']['project_nid'] = array(
+    '#type' => 'select',
+    '#title' => t('Project'),
+    '#default_value' => isset($node->project_nid) ? $node->project_nid : NULL,
+    '#options' => $projects,
+    '#process' => array('storm_dependent_select_process'),
+    '#attributes' => array('onchange' => "stormticket_project_task_tickets(this, 'edit-organization-nid', 'edit-task-nid', 'edit-ticket-nid', true, '-')"),
+    );

Also, one of the exceptions seems to imply calling a function from the (not enabled) storm ticket module. Is stormticket a dependency of stormexpense? If so, it needs including in the test, if not, there is a bug in stormexpense.

Powered by Dreditor.

Thanks for the update. A couple more comments.

+++ ./stormexpense/stormexpense.module	2011-01-13 13:45:35.931102000 -0500
@@ -385,37 +389,41 @@ function stormexpense_form(&$node) {
-  $tree = _stormtask_get_tree($node->project_nid);
-  $tasks = _stormtask_plain_tree($tree);
-  $tasks = array(0 => '-') + $tasks;
-  $form['group1']['task_nid'] = array(
-    '#type' => 'select',
-    '#title' => t('Task'),
-    '#default_value' => $node->task_nid,
-    '#options' => $tasks,
-    '#process' => array('storm_dependent_select_process'),
-    '#attributes' => array('onchange' => "stormticket_task_tickets(this, 'edit-organization-nid', 'edit-project-nid', 'edit-ticket-nid', true, '-')"),
-  );
+  ¶
+  if (isset($node->project_nid)) {
+    $tree = _stormtask_get_tree($node->project_nid);
+    $tasks = _stormtask_plain_tree($tree);
+    $tasks = array(0 => '-') + $tasks;
+    $form['group1']['task_nid'] = array(
+      '#type' => 'select',
+      '#title' => t('Task'),
+      '#default_value' => (isset($node->task_nid)) ? $node->task_nid : NULL,
+      '#options' => $tasks,
+      '#process' => array('storm_dependent_select_process'),
+      '#attributes' => array('onchange' => "stormticket_task_tickets(this, 'edit-organization-nid', 'edit-project-nid', 'edit-ticket-nid', true, '-')"),
+    );
+  }

It is not impossible to think of a situation where project_nid is not set, but we still want to set a task. Consider the node add form.

Therefore, we can't have the entire form item inside the if, it needs to be more specific.

+++ ./stormexpense/stormexpense.module	2011-01-13 13:45:35.931102000 -0500
@@ -536,9 +544,9 @@ function stormexpense_insert($node) {
+    $node->vid, $node->nid, $node->organization_nid, $node->organization_title, $node->project_nid, (isset($node->project_title)) ? $node->project_title : NULL, ¶
+    (isset($node->task_nid)) ? $node->task_nid : NULL, (isset($node->task_title)) ? $node->task_title : NULL, (isset($node->task_stepno)) ? $node->task_stepno : NULL, $node->ticket_nid, (isset($node->ticket_title)) ? $node->ticket_title : NULL, ¶
+    (isset($node->provider_nid)) ? $node->provider_nid : NULL, $node->provider_title, $node->expensedate, $node->amount, $node->tax1app, $node->tax1percent, ¶

Trailing whitespace behind all of these lines.

(Not introduced by this patch, but is something that could do with fixing)

+++ ./stormexpense/stormexpense.module	2011-01-13 13:45:35.931102000 -0500
@@ -583,18 +591,30 @@ function _stormexpense_beforesave(&$node
-  $node->project_title = $p->title;
+  if (!empty($p)) {
+    $node->project_title = $p->title;
+  }

I think for the functions within ..._beforesave, we should use the ... ? ... : ... syntax in order that we have explicitly set each part of $node.

Powered by Dreditor.

+++ ./stormexpense/stormexpense.test	2011-01-13 12:33:24.511100000 -0500
@@ -0,0 +1,38 @@
+    // Create a team

Also, copy and paste error!

Powered by Dreditor.

Patch for review.

I don't seem to be improving things here...

Looks good, thanks.

Committed to 2.x.

Testing on 1.x.

#17: storm_expense_simple_test.patch queued for re-testing.

Committed to 1.x.