Add billable/billed flags to expenses

I need the billable/billed flags for a module I'm working on at the moment.
Now, since I don't have the time to wait for an update of this issue, I'm willing to write a patch by myself and to contribute it here.

But it'd be nice, if you could give me some inputs where I have to make changes so I dont' mess the module up. Wouldn't wanna write a patch which actually creates more bugs than new features.

Best place to look is how the billable / billed flags have been implemented on other parts of Storm.

I just made a patch with the billable and billed flags for expenses.

Hope, I din't miss anything.

I've just read through - seems sensible and complete.

Would be good for someone to test and mark rtbc if happy.

Just to note in terms of process, I'll commit bug fixes first - so may need rework if any of the bug fixes conflict with this patch (not sure).

Confirming patch in comment 4 works. Thanks Raphael :)

Using
* Fresh Drupal 6.27
* Storm 2.x-1.x-dev 2012-Oct-20

Thanks for testing.

Considering that we've started with the port to Drupal 7 now, we need to have a patch for D7 before D6.

Sorry for the inconvenience, but I'm sure you'll agree this is best practice for the longer term.

Same thing, make sense to me :)

Any volunteer to port this patch? I would be happy to contribute testing.

Since the D7 port is now in this module, I am moving it here.

Follow up from #1849014: Fix expenses form fields #7:

On the new feature, we need to discuss whether to put this in at this stage, using the old methodology for items like a list page etc, fields api etc.

My personal opinion is that at this stage, we should focus more on getting the existing working before adding features that we'll then have to change.

I think, we should implement this feature at this stage.
The changes are pretty minor (hook_schema, hook_form, hook_insert, hook_update, node view template, hook_views_data) and I think, it would be good to implement this before we port the invoice save function to D7.

I've been thinking about this again since that comment.

Definitely whilst the port is still very much in progress, then the focus should be on getting the basics working.

My concern on adding new fields roots from the fact that this will be data that will then need to be migrated to the new format. Obviously it is cleaner if we can just get stuff in the new format straight off.

On reflection, this change is probably fine at this stage. We have fields like this already, so migration will just be an extension from that. So fine to go ahead.

Any thoughts from anyone else?

I think adding in new features as part of the migration is good as long as it doesn't interfere with progress on the D7 port.

Ok, I should be able to provide a patch for this later today. I did most of the work for this already in the other issue.

I just wanted to note that I added this to the roadmap in the ongoing improvements section, but on the principle that it can be done much earlier (even now) if someone (you!) provides a patch!

Ok, this is the patch to add the billable/billed fields.
I think, everything should be included here.

I also checked the code with Coder & Coder Tough Love.

Patch applied cleanly, however when creating a new expense the following warnings show up...

Warning: substr() expects parameter 2 to be long, string given in pminvoice_get_invoice_number() (line 987 of /media/sf_sandbox/pm/sites/all/modules/pm/pminvoice/pminvoice.module).
Warning: substr() expects parameter 2 to be long, string given in pminvoice_get_invoice_number() (line 987 of /media/sf_sandbox/pm/sites/all/modules/pm/pminvoice/pminvoice.module).

THEN, when filling out the form and saving I get the following message(s)...

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'billable' in 'field list': INSERT INTO {pmexpense} (vid, nid, organization_nid, organization_title, project_nid, project_title, task_nid, task_title, task_stepno, ticket_nid, ticket_title, provider_nid, provider_title, expensedate, amount, tax1app, tax1percent, tax1, subtotal, tax2app, tax2percent, tax2, total, billable, billed) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23, :db_insert_placeholder_24); Array ( [:db_insert_placeholder_0] => 47 [:db_insert_placeholder_1] => 47 [:db_insert_placeholder_2] => 1 [:db_insert_placeholder_3] => test org 1 [:db_insert_placeholder_4] => 2 [:db_insert_placeholder_5] => test proj 1 [:db_insert_placeholder_6] => 5 [:db_insert_placeholder_7] => test task #1 [:db_insert_placeholder_8] => [:db_insert_placeholder_9] => 15 [:db_insert_placeholder_10] => test ticket [:db_insert_placeholder_11] => [:db_insert_placeholder_12] => [:db_insert_placeholder_13] => 1367448481 [:db_insert_placeholder_14] => 687 [:db_insert_placeholder_15] => 1 [:db_insert_placeholder_16] => 20 [:db_insert_placeholder_17] => 137.4 [:db_insert_placeholder_18] => 824.4 [:db_insert_placeholder_19] => 0 [:db_insert_placeholder_20] => 20 [:db_insert_placeholder_21] => 0 [:db_insert_placeholder_22] => 824.4 [:db_insert_placeholder_23] => 1 [:db_insert_placeholder_24] => 0 ) in pmexpense_insert() (line 739 of /media/sf_sandbox/pm/sites/all/modules/pm/pmexpense/pmexpense.module).

The first error comes from the pminvoice module. I didn't turn this on while testing this patch because it's not ported to D7 yet.

The second one seems to be a problem on your side. You have to reinstall the pmexpense module. Meaning, you have to uninstall the module first (drush pm-uninstall pmexpense) and then enable it again.
Otherwise the hook_install function doesn't get called and the pmexpense table in the database has no billable/billed columns.

Edit: The first error is really an error in pminvoice and has absolutely nothing to do with this patch or pmexpense in general.
So I recommend to just turn it off for the moment.

We need to implement hook_update_N to add these fields as well, so that an existing installation can be upgraded - we don't want to have to reinstall from scratch each time!

Yes, but does this make sense, why we are still in the dev stage and have no official release?

Yes, because anyone who tries to migrate from Storm in Drupal 6 to Project Management in Drupal 7 will effectively run every database update that has happened between the two.

That's 500 installations that I don't want to cause grief for!

(Yes, I hope that many people will rebuild on Drupal 7 rather than upgrade, but I think we still need to provide a working upgrade path).

Ok, then I guess I have to use pmexpense_update_7000?

mymodule_update_7000(): This is the required update for mymodule to run with Drupal core API 7.x when upgrading from Drupal core API 6.x.

Please use 7101.

We've started at 7100 being the upgrade to bring you to 7.x-1.x, then by using a new update rather than adding to the existing, we ensure anyone who is already following -dev is fine.

Added hook_update_N.

OK, reviewing based on http://groups.drupal.org/node/296793, which I hope will help us with swift, effective reviews! (Feedback on this very welcome).

#1: Testbot pass: Yes - passed
#2: Coder & CTL pass: No - 3 normal warnings, 6 minor warnings (although I see that a lot of other warnings have been fixed, thanks!)
#3: Readthrough: Yes - looks fine
#4: Functional Test: Not tested this yet.

So "needs work", based on #2.

I fixed all the coder warnings.

Most of them were just missing docblocks.

Patch applied cleanly. Ran update.php and this completed OK. Cleared cache.

Creating new billable expense produced no errors or warnings.

Editing existing nodes adding in billable and/or billed fields worked OK.

Changed expense view to include billed/billable fields (see attached) resulted in a new stunning view. (wow!!!)

Nice job. I'm going to love using the new D7 PM module for project management.

I believe this is now RTBC:

#1: Testbot passes: Yes, all green.
#2: Coder / CTL passes: 3 normal warnings, but I /think/ these are ones to ignore (will check in more detail).
#3: Readthrough: Yes, I think it looks fine
#4: Functional: Yes (#26).

Committed, thanks.