When a timetracking record is added to an invoice, it would be good to have a link between the two.

So that you can go from one to the other.

I.e. customers can check which invoice contains the timetracking record.

CommentFileSizeAuthor
#39 738020_3.patch8.28 KBtchurch
#33 738020_2.patch4.07 KBtchurch
#29 738020_2.patch4.04 KBtchurch
#20 738020.jpg102.74 KBtchurch
#17 738020.patch4.02 KBtchurch

Comments

juliangb’s picture

juliangb commented
Version: 6.x-1.31 » 6.x-1.x-dev

The data is already being stored to go from invoice to timetracking, so it is just a case of editing theme_stormtimetracking_view() to show a link.

The other direction will need some additional data.

tchurch’s picture

tchurch commented

I've checked further in the data. I can see a link from the timetracking record to the invoice record but not the invoice_item record.
I added 2 timetracking records to the same invoice but only 1 is linked from the invoice table.

I assume this would be a bug and it should be linking timetracking to invoice_item?

juliangb’s picture

juliangb commented

OK sounds like it wasn't extended when we added the functionality to add to an existing invoice.

Not a bug as such considering it has never been there - but something it would be good to add.

Feel free to have a play!

aacraig’s picture

aacraig commented

I actually think that a deeper problem exists, besides the cosmetic issue of a link between an invoice item and the timetracking item that it's created from.

Currently, if I add a timetracking item to an invoice, that item's data is copied into the invoice item table.

This (redundant?) copying of data means that if I ever update the original timetracking item, the corresponding entry in the invoice won't be changed -- the data has gone out of sync.

While this is arguably desired behavior after an invoice has been issued and paid, it's still counter-intuitive. If a user adds a timetracking to an invoice, it's reasonable for them to expect that link to continue over time.

I propose that the invoice module store just the item id of the timetracking item.

tchurch’s picture

tchurch commented

I'm not so sure.

Invoice items can also be created manually. They don't have to come from timetracking (and in the future other Storm node types; see #567558: Extend auto-billing to other types of Storm node).

Personally I would think that timetracking records (and others later) shouldn't be edited once they were added to an invoice. This can create inconsistencies (authors shouldn't be allowed to edit/delete records after invoicing; see #801374: Allow deleting/editing of timetracking record if not already billed.).

Also invoice item details (like price and description) are calculated when added to the invoice.

Really, this feature is a 'nice to have' to allow people to jump between the invoice item and the related node and visa versa.

aacraig’s picture

aacraig commented

I suppose that's a matter of your own internal workflow. I don't see anything wrong with adjusting a timetracking before an invoice has been paid. This is a reasonable use case, especially if (as in my case) there is an auditing step before the invoice is sent to the client. As it stands now, I have to make the same edit in two places if a discrepency is found -- or if one of my interns 'accidentally' puts their Facebook time on the books ;)

I agree that an item may come from more than one place, but that doesn't mean you can't store the relevant data for each type of node in its own table and store the associated id in the invoice.

juliangb’s picture

juliangb commented

I'm with tchurch. It may not suit your workflow (nothing can suit everyone), but I think it would cause more problems if invoices were changed after issue, possibly without the owner realising.

tchurch’s picture

tchurch commented

I'm not a legal expert but I'm quite sure that invoices are important documents. Once they are produced they should not be changed. I think it's like that in the UK anyway.

I hate to say this (as it might mean more work) but I have a few ideas.

1) A new tick box on the invoice level "Sent to customer" or something like that.
With extra permission/code (maybe) to allow:
- add new items only to invoices not sent.
- new views filter option (if it can be done) so that views can only show invoices sent.
- maybe a bulk update option to allow changing this flag for multiple nodes?

2) Some kind of resync (where am I going ?!?)
Once there is a link between Storm node and the invoice item, if the node (i.e. timetracking record) was changed, it could be re-synced (an update) of the original invoice item but only if the invoice has not been sent.

If we do go for these, it might take a while as there's a lot of dependencies between pending issues at the moment (for me anyway).

I may be way off base with these ideas and it might make things more complex for people who won't want to use the functions (maybe some default settings in admin?).
Just throwing the idea out there.

aacraig’s picture

aacraig commented

You're absolutely correct that an invoice is a sensitive document, and it's just for this reason the data between the source timetracking and the invoice must be in synch.

What if you've invoiced a timetracking item and then later set the timetracking's billed flag to false?

tchurch’s picture

tchurch commented

I agree. I have the similar issue with a customer of mine using Storm.
I've had to remove delete permissions from them, just to make sure that they don't delete things they shouldn't (dependencies).
Unfortunately I can remove edit permissions (not viable).

I would like to build on the dependencies and permissions so that (examples):

a) users cannot edit/delete timetracking if already billed (even if they're the author).
b) users cannot edit/delete invoices if already "sent" (maybe we could use published/published for this instead).
c) decide if users can see unpublished content in the list (maybe add this column to the lists too).
d) check over dependencies (e.g. so that users cannot delete projects if they have tasks etc...)

plus lots more...

I think we should stay on issue here and open new issues for the other items to discuss them further.
I can see some kind of 'sync' option being handy for all Storm types (after #567558: Extend auto-billing to other types of Storm node).

I start opening new issues for the other points made here (by me and aacraig) and then we can comment on them separately.
Otherwise I think we'll all get confused (I know I do).

aacraig’s picture

aacraig commented

I think you're on the right track.

Could you please reference the new issues here so they're easy for us to find? I'd like to contribute to the conversation (and eventually to the code base!).

tchurch’s picture

tchurch commented

The following issues have been created:

#835360: Flag - invoice "sent to customer".
#835366: Resync Storm node with invoice item
#835368: Viewing of unpublished nodes
#835370: Check dependencies between node types

Also the following issues are linked with discussions here:

#801374: Allow deleting/editing of timetracking record if not already billed.
#567558: Extend auto-billing to other types of Storm node

tchurch’s picture

tchurch commented
Title: Link timetracking record to invoice » Link timetracking record to invoice item

Slightly changed title.

tchurch’s picture

tchurch commented
Assigned: Unassigned » tchurch

I'd like to work on this for 1.35

I would update it so that there is a link between the node (timetracking or other) to the invoice item record when billed.

I would also like to add a link on the invoice view page to link back to that node.

Any thoughts.

juliangb’s picture

juliangb commented

Might be just worth clarifying the scope - is this just timetrackings or other nodes that can be added to invoices too?

Seems like it would be worth generalising, but as much as anything else, knowing will help know what to expect when reviewing the patches...

tchurch’s picture

tchurch commented
Title: Link timetracking record to invoice item » Link node record to invoice item

Title changed to generalise the change.

It would cover any item added to an invoice. I would add a link back to it's original node.

e.g.
- You add a project to an invoice so the link would go back to the project.
- You add a timetracking record, the link would go back to that record.

If it was a manual invoice item then no link would be displayed.

tchurch’s picture

tchurch commented
Status: Active » Needs review
StatusFileSize
new 738020.patch4.02 KB

First try.

juliangb’s picture

juliangb commented

Looks good, but this will probably break #848444: Multiple Select for Teammembers - any chance we could try to get that in first as it should be a major usability improvement?

tchurch’s picture

tchurch commented

I'll look at testing #848444: Multiple Select for Teammembers.
I'm not sure where the clash could be though.
I have both installed on my test area at the moment.

tchurch’s picture

tchurch commented
StatusFileSize
new 738020.jpg102.74 KB

I've attached a screen shot of the invoice edit page. It shows a new field for weight. Users can put a number in there.
The smaller the number, the higher on the list (like all other Drupal weight fields).

This field already existed in the DB. I've added it to the edit page.

Any comments?

juliangb’s picture

juliangb commented

I confused invoices and teams. Therefore, ignore #18.

Seems good.

Although I think you've posted #20 on the wrong issue?

tchurch’s picture

tchurch commented

You're right. #20 was wrong.
I confusing my issues. :)

It should be #887638: Invoice Items not in correct order

juliangb’s picture

juliangb commented
Status: Needs review » Reviewed & tested by the community

RTBC for the patch in #17, assuming separate patches if we want to do this for any node type added to an invoice.

tchurch’s picture

tchurch commented
Component: Storm Timetracking » Storm Invoice

This patch should work for all Storm node types. If it's OK I can commit it and move on.

juliangb’s picture

juliangb commented

No patch...

tchurch’s picture

tchurch commented

Sorry, the patch in #17 already should work for all Storm nodes.

juliangb’s picture

juliangb commented
Status: Reviewed & tested by the community » Needs review

OK, I see.

A few questions...

+++ storminvoice/storminvoice.theme.inc	26 Sep 2010 08:57:08 -0000
@@ -344,7 +344,7 @@ function theme_storminvoice_view($node, 
+  $s .= "sit.total, ifnull(sit.src_nid,0) as src_nid FROM {storminvoice_items} sit WHERE sit.invoice_vid=%d ORDER BY sit.weight ASC";

What is wrong with src_nid coming out as NULL, then checking against NULL instead of 0?

It doesn't really matter either way, but perhaps it simplifies the code.

+++ storminvoice/storminvoice.theme.inc	26 Sep 2010 08:57:08 -0000
@@ -364,6 +364,12 @@ function theme_storminvoice_view($node, 
+    if ($invoice_items[$key]['src_nid'] != 0) {
+      $invoice_items[$key]['src_nid'] = l(t('Link'), 'node/'. $inv_item['src_nid']);
+    }

If I'm right in understanding, this adds the word link at the end of the invoice item.

Why don't we just make the description a link?

Also, would there be an advantage to utilising the src_vid instead of / as well as src_nid so that the link goes to the correct revision of the source item?

+++ storminvoice/storminvoice.theme.inc	26 Sep 2010 08:57:08 -0000
@@ -364,6 +364,12 @@ function theme_storminvoice_view($node, 
+    else {
+      unset($invoice_items[$key]['src_nid']);
+    }

What does the else{} section achieve?

Powered by Dreditor.

tchurch’s picture

tchurch commented

Some answers:

1) setting NULL to 0 - this was to treat NULL and 0 the same. I've removed this and put the extra check on line 367 instead. Might make is easier to understand.

2) make description the link - good idea. Have changed it.

3) unset variable - this is needed otherwise the next part (line 376, making the table) adds the src_nid to the page.

4) regarding the nid/vid question, I've played around with this. It seems that the vid doesn't matter for this. No matter what revision it has, the link always uses the nid and it finds the correct revision.

I'll post a new patch soon.

tchurch’s picture

tchurch commented
StatusFileSize
new 738020_2.patch4.04 KB

new patch

juliangb’s picture

juliangb commented 
+++ storminvoice/storminvoice.theme.inc	3 Oct 2010 18:00:50 -0000
@@ -364,6 +364,10 @@ function theme_storminvoice_view($node, 
+    if ($invoice_items[$key]['src_nid'] != 0 OR $invoice_items[$key]['src_nid'] != NULL) {

Should this be AND?

Powered by Dreditor.

tchurch’s picture

tchurch commented

I don't think so.
If it's not zero OR not null then it has a valid number.

If it's AND then only one will ever be true and will never display the link.

juliangb’s picture

juliangb commented

Imagine passing in src_nid = 1 (or any positive integer)
TRUE OR TRUE = TRUE
TRUE AND TRUE = TRUE

Imagine passing in src_nid = 0
FALSE OR TRUE = TRUE
FALSE AND TRUE = FALSE

Imagine passing in src_nid = NULL
TRUE OR FALSE = TRUE
TRUE AND FALSE = FALSE

Unless I've made a mistake or misunderstanding, I think we want the results from AND.

tchurch’s picture

tchurch commented
StatusFileSize
new 738020_2.patch4.07 KB

I think you might be right.

New patch.

juliangb’s picture

juliangb commented 
+++ storminvoice/storminvoice.module	3 Oct 2010 18:55:23 -0000
@@ -548,6 +548,16 @@ function storminvoice_form(&$node) {
+    $form['group4'][$i]['first']['items_'. $i .'_src_nid'] = array(

I may have missed the reason behind this - but what would be wrong with using $form['group4'][$i]['src_nid']? (Simplicity)

Powered by Dreditor.

tchurch’s picture

tchurch commented
Status: Needs review » Needs work

I have to admit, I don't know.

But I copied it from the description and weight field handling.

I've done a little bit more testing on it and found that adding a second item to an invoice doesn't take the right nid.
It's using the previous one (or the one of it's project).

I'll check further. Maybe this is the reason but then I should see the same error with the description too, I think.

juliangb’s picture

juliangb commented
Status: Needs work » Reviewed & tested by the community

Actually, I think I remember. The structure of the array only controls positioning, and the name of the last element controls the name of the element that stores the data, therefore, the last element must be unique.

I wonder if 'first' would be better as 'src', but thats fairly irrelevant, and I think the patch in #23 is ready to be committed.

tchurch’s picture

tchurch commented
Status: Reviewed & tested by the community » Needs work

OK. I still have to fix the "add to existing" error I mentioned in #35.

I think I need to make a few small changes to all the other modules (where it adds to invoice) to set the value but I'll have to do that tomorrow.

Back to NW.

tchurch’s picture

tchurch commented

Did a quick test on timetracking and it worked. Will deliver a new patch tomorrow.

tchurch’s picture

tchurch commented
Status: Needs work » Needs review
StatusFileSize
new 738020_3.patch8.28 KB

New patch. Hopefully this should work (and also adds the code in preparation for #695976: Add billable/billed flags to expenses)

juliangb’s picture

juliangb commented

What is the connection to 695976?

tchurch’s picture

tchurch commented

The only link is that I've added the same code to the expenses module for the src_nid/src_vid for invoice items but the code isn't used until that issue is actually fixed. It just saves going back to it later or maybe forgetting to change it.

juliangb’s picture

juliangb commented

Seems ok by me.

tchurch’s picture

tchurch commented

I will commit this if no-one has any objections.

juliangb’s picture

juliangb commented

Feel free. I should also have cvs access for a bit soon.

tchurch’s picture

tchurch commented
Status: Needs review » Fixed

committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.