timetracking records not taking fixed price from organisation, project or task.

@tchurch, please do post the files here.

I would strongly encourage you to venture into the world of patching though - it is really useful to be able to see the specific change that has been made. There is a good drupal specific guide about how to do it here - http://drupal.org/patch/create.

The testbot needs the status to be 'needs review' to look at the patch...

The testbot is probably complaining because the patch diffs that one file, rather than being relative to the root directory - basically it doesn't know where to find the file. This can be got around by diffing the whole storm directory.

However, when I tried to apply the patch manually, I got errors with the first hunk. Are you sure that the patch is against a clean copy of the file?

Were any of your other patches applied before making this patch?

What happens if you get a clean -dev version and apply the patch. Does it work for you then?

OK - testbot is happy, so i've gone through the code in some more detail now. Please don't be offended if we pass the code back and forth a couple of times like this. I know it's annoying, but for things like coding style (see http://drupal.org/coding-standards), it's really helpful for people who later come to make changes to the module if the whole lot follows a similar style.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -692,13 +692,28 @@ function stormtimetracking_storminvoice_
+    global $use_fixed, $use_hourly, $fixed_price, $hourly_rate;

Please could we avoid using globals whereever possible? (Simply to avoid any conflicts with other modules etc.) Perhaps these could be returned by the function?

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -692,13 +692,28 @@ function stormtimetracking_storminvoice_
+    ¶

Please use two spaces rather than a tab.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -692,13 +692,28 @@ function stormtimetracking_storminvoice_
+    	$new_invoice->items[$count]->amount = $node->billing_duration * $hourly_rate;
+    	}

Tab indentation - please use two spaces instead.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -692,13 +692,28 @@ function stormtimetracking_storminvoice_
+    	$new_invoice->items[$count]->amount = $fixed_price; ¶
+    	}

Tab indentation.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -725,41 +740,53 @@ function stormtimetracking_views_api() {
- * Try to obtain an hourly rate from a timetracking node. To do this we will 
+ * Try to obtain an hourly rate or fixed price for the new invoice item for a timetracking node. To do this we will ¶

Could we rebalance the lines now? Perhaps split at the sentence break?

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -725,41 +740,53 @@ function stormtimetracking_views_api() {
+ *   Variables are set to global.

As above.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -725,41 +740,53 @@ function stormtimetracking_views_api() {
+	global $use_fixed, $use_hourly, $fixed_price, $hourly_rate;

As above.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -725,41 +740,53 @@ function stormtimetracking_views_api() {
+        	$found = TRUE;

Tab indentation.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-13 19:12:46.419917700 +0100
@@ -725,41 +740,53 @@ function stormtimetracking_views_api() {
+  if ($found == TRUE) {
+  break;
+  }

Indentation.

In general though, seems like quite logical changes.

Thanks for your efforts in getting the patch done. Hope that the experience with patching will help you too!

Powered by Dreditor.

One more code style bit, and then I'd just like to apply and test it. Also happy if someone else were to test applying the patch / the functionality and report back / mark rtbc.

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-16 19:02:22.546875000 +0100
@@ -725,41 +734,56 @@ function stormtimetracking_views_api() {
+	$rate_array = array('use_fixed' => FALSE, 'use_houlry' => FALSE, 'fixed_rate' => 0, 'hourly_rate' => 0);

Tab indentation. Spelling error on 'use_houlry'.

Powered by Dreditor.

Not quite...

+++ stormtimetracking/stormtimetracking_fix735694.module	2010-03-16 19:02:22.546875000 +0100
@@ -725,41 +734,56 @@ function stormtimetracking_views_api() {
+  $rate_array = array('use_fixed' => FALSE, 'use_houlry' => FALSE, 'fixed_rate' => 0, 'hourly_rate' => 0);

Spelling error - 'use_houlry'

Powered by Dreditor.

Code looks good to me now. Will try to test today.

Testing this - I wonder whether we're going down the right direction with it.

Created a fixed price project, and timetracking from that.

Without the patch - autoinvoice entry shows "1 hour unbilled work...." zeros in price.

With patch - autoinvoice entry shows "1 hour unbilled work..." and cost of project in price.

Wondering whether we're trying to solve the problem the wrong way around - I think what we should actually be working on to get what you want is the ability to add an entire project / task / ticket to an invoice at once. Then you'd get a line for the fixed cost of project, then zeros for the unbilled time.

Am I getting this correctly?

Always separate patches if possible.

OK so we here we have two distinct ways of using the fixed price in the project / task - you're using it as fixed per timetracking, and the other way is fixed per 'item' with all timetrackings included.

Perhaps we could find a way of separating these so that it is as expected for all users?

Sorry, I've been a bit lax about getting back on this one.

My issue is that there seem to be a couple of interpretations of 'fixed':
- The project/task/ticket is a fixed price, so all timetrackings as part of that are unbilled and zero cost.
- Each timetracking is fixed price, based upon the price agreed per project.

The former is what it is set up for at the moment, the latter is what you want.

Especially, I think the concept of seeing 'unbilled' followed by a non-zero price a bit confusing.

I had been meaning to set in and code something for this but haven't got around to it yet. What needs to happen though is something that will allow for both those cases above. I'm happy to hold the next snapshot release if this issue seems to be making progress.

I like the idea of a 'fixed xxx' mode, whereby the xxx is the type of entity that is that price.

Great.

There is already a feature request for the project/task/ticket billing: #567558: Extend auto-billing to other types of Storm node.

Ok, there are some really minor stuff below - but my main question is - will this patch give a situation where a non-zero price is put onto the invoice but with the description of "unbilled"? For me, that is quite confusing. I'll admit I haven't tested it yet (limited internet access this week), but from my read through I think it will show that.

+++ stormattribute/stormattribute.install	29 Mar 2010 14:22:07 -0000
@@ -948,3 +948,11 @@ function stormattribute_update_6110() {
+  $ret[] = update_sql("INSERT INTO {stormattribute} (domain, akey, avalue, weight, isactive) VALUES ('Price mode', 'fixed_timetracking', 'Fixed TimeTracking', 0, 1)");

Lets use "Timetracking" rather than "TimeTracking"

+++ stormattribute/stormattribute.install	29 Mar 2010 14:22:07 -0000
@@ -948,3 +948,11 @@ function stormattribute_update_6110() {
+
+  return $ret;  ¶
+}

Trailing whitespace.

+++ stormtimetracking/stormtimetracking.module	29 Mar 2010 14:22:08 -0000
@@ -692,17 +692,30 @@ function stormtimetracking_storminvoice_
+    if ($rate_array['use_fixed'] == TRUE) {
+      $new_invoice->items[$count]->amount = $rate_array['rate_to_use']; ¶

Trailing whitespace.

+++ stormtimetracking/stormtimetracking.module	29 Mar 2010 14:22:08 -0000
@@ -692,17 +692,30 @@ function stormtimetracking_storminvoice_
+    if ($rate_array['use_fixed_timetracking'] == TRUE) {
+      $new_invoice->items[$count]->amount = $rate_array['rate_to_use']; ¶

Trailing whitespace.

Powered by Dreditor.

OK. As I said before, my web access is a bit limited this week, but will test it out and feedback.

The db update needs a doxygen style comment, as this really helps when updating via drush.

But more importantly, the invoice line needs a bit of clarifying still - we have "01 Apr 10: 0.0333333 hours work at 10000 per hour on Fixed TT mode". I don't think it should be 10000 per hour in one of the fixed modes.

Committed, thanks for you work pushing this.