GNOME Bugzilla – Bug 728136
Rate value in job
Last modified: 2018-06-29 23:29:33 UTC
Created attachment 274214 [details] [review] patch generated with git format-patch origin/master..master A "Rate" value has been added to jobs, to record (e.g.) hourly rate or project price. New invoices that are generated for this job, will take that value as the default price. The definition of the Job structure has changed, so new files / databases created will not be read by older versions.
(In reply to comment #0) > The definition of the Job structure has changed, so new files / databases > created will not be read by older versions. This will need to be fixed. GnuCash makes a point of being able to read/write in forward/backward compatible ways, at least between adjoining versions. So either this feature will need to be added to 2.6 AND trunk, or (better!) you should change the patch to use the KVP as your storage so it wont affect older versions.
Created attachment 274318 [details] [review] patch to add "rate" in jobs
I adjusted the patch to use KVP (and I'm afraid I shouldn't have marked it as "patch" in the upload window - sorry if that's a mistake).
Marking it as a patch is absolutely correct. It turns on the patch tracking features in Bugzilla. You get extra points as well for keeping the kvp usage encapsulated in the GncJob class instead of leaking the slots pointer to the UI, and for providing a test, even if it's minimal. In general, this looks good to me. I'd like Geert to look it over before we commit it because he's the one most familiar with the business code at the moment.
Testing on Fedora18 and with reference to Bug 728103 I note that the job rate is reset to zero over a save, close, open cycle.
Hi Mike I would like to try find the reason and fix it, but I'm not sure how to duplicate it. Can you please clarify 'save, close, open cycle'? Do you perform that cycle on the job, or the invoice? And where does the job rate reset to zero? Thanks.
(In reply to comment #6) > Hi Mike > I would like to try find the reason and fix it, but I'm not sure how to > duplicate it. > Can you please clarify 'save, close, open cycle'? Sequence: Modify job to include a rate. Open q new invoice using that job. Add a line to invoice. save and close GnuCash. Reopen Gnucash. Result: Job field is empty. Opening job for editing reveals that rate is = 0.0.
Mike, using the SQL backend? If so, the issue is the lack of begin_edit/commit_edit around the KVP modification. (I haven't actually looked at the patch).
(In reply to comment #8) > Mike, using the SQL backend? Not me, XML file only. > > If so, the issue is the lack of begin_edit/commit_edit around the KVP > modification. (I haven't actually looked at the patch).
Derek, begin_edit and commit_edit are there. Mike, I cannot replicate it. I did the following: Modify job to include a rate. Open a new invoice using that job. Add a line to invoice. save and close GnuCash. Reopen Gnucash. Result: Job field is empty. THIS IS NOT WORKING FOR ME: Opening job for editing reveals that rate is = 0.0. As long as the job field is empty, pushing the edit button does nothing. If I add another line (and the job field gets populated), the rate is still the original value. tested on Ubuntu 13.10, xml and sqlite backends Now, when you open the job for editing, are the rest of the values correct (name, billing id?) or are they also zeroed out?
Everything else is fine. Rate is set to zero. I edit the job via, Business->Find Job, then open it and edit. My XML file has a section like: <gnc:GncJob version="2.0.0"> <job:guid type="guid">d00d093d4dbf800505f9177e484f8988</job:guid> <job:id>000002</job:id> <job:name>Website</job:name> <job:owner version="2.0.0"> <owner:type>gncCustomer</owner:type> <owner:id type="guid">02e98110c5e572f3fef55b8321d754b6</owner:id> </job:owner> <job:active>1</job:active> </gnc:GncJob> Adding a rate to job# 000002 then saving does not add a rate entry to this section. I can only assume that the rate isn't being saved to the XML. It works for you however with XML, yes?
Hi Mike Thanks for your help. I can replicate it now - I have no idea how I thought it was working, I'm pretty sure I did most of the tests but it looks like the rate value is lost when gnucash closes in both xml and sql. I'll try to figure it out and resend the patch.
Check the save routines for GncJob and make sure that they're saving KVP.
GRRR.. Looking at src/backend/xml/gnc-job-xml-v2.c in job_dom_tree_create() it indeed does NOT save the KVP. :-(
(It does *LOAD* it, but doesn't save it)
Adding the functionality to save the kvp means breaking compatibility with previous versions of gnucash. This means the new functionality will have to be inserted with great care. We can fix the kvp saving in both the maint and master branch. Actually storing the rate value in a job should be reserved for the master branch only and a new feature flag should be set for it. But in this case I wonder if it really matters to work via kvp ? There is no way to implement this in a backwards compatible way (due to the missing kvp save in job_dom_tree_create). So in this case we could consider to insert a new field in the xml/sql structure directly instead.
I disagree, Geert. Saving the KVP wont break compatibility per-se with older versions, because the older versions *DO* have the ability to read in the KVP.. They just wont save them back out again, which means loading into an older version will cause data loss. I guess the question is which is more important: being able to load data into an older version even if you don't set a Job Rate, or making sure you don't lose the data if you decide to load it into an older version? Personally I'd rather be able to load it in even if the rate isn't used (I guess this could be accomplished by not emitting the entry if it's 0). Regardless, we should add the KVP-save now.
Hmm, just to be sure I understand you properly: - being able to load the file in an older version can only be accomplished if the job-rate is stored via kvp. Without further safeguards this can lead to data loss if the older version is used the modify the data file. - using a real data field in xml will cause older versions of gnucash to fail to load the file altogether. In this case there is no risk of data loss because older files are not able to load the file, let alone change it. In short using kvp we could consider the job-rate field an optional thing. Using an xml tag will make it always present. That may not be necessary. So yes, kvp is probably better. And I agree the kvp-save should be added as soon as possible. Now to prevent a saved rate to be lost when the file is modified with an older version of gnucash, we should still add a new feature flag as well that gets set whenever there is at least one rate in use in the data file. The question is which version of gnucash should be the first one marked to be compatible with the job-rate feature ? I'd suggest the version in which the kvp-save for jobs is introduced. It will not actively use the rate, but it will preserve the data. Earlier versions (2.6.3 and earlier) may potentially loose it.
Instead of calling it the "Job Rate Feature" we should call it the "Job Extra Data Feature", which should be set any time the Job's KVP is used to anything. And yes, this feature should be tied to the Job KVP Save patch (whenever that is).
If I had seen comment 14 earlier, it would have saved me one hour banging my head to find out what is wrong :) I was not familiar with KVP so it was not easy for me. Nevertheless, I found it and I'm creating the patch as we speak to add kvp_saving, but I will not submit it unless you reach a consensus on how it should be handled (and my poor programming skills will hopefully be enough to implement it). Thank you all for the input and your time
And I don't think that xml code loads the KVP, since job_dom_tree_create does not have any reference to kvp_frames.
(In reply to comment #21) > And I don't think that xml code loads the KVP, since job_dom_tree_create does > not have any reference to kvp_frames. Indeed, the job_slots_handler doesn't actually handle the slots, it just ignores it. :(
The SQL backend already loads and saves GncJob slots. The fact that the XML backend doesn't is a bug: Slots are a property of QofInstance, inherited by all subclasses including GncJob. It just happens that slots on GncJob hadn't been used before, so the bug went unnoticed. Just fix it and move on.
Created attachment 274560 [details] [review] New patch including the fix for slot saving in job xml backend
Created attachment 274561 [details] [review] Removal of accidentally left-over unreachable code Just for clean code.
Created attachment 274562 [details] Minimal test case for the suggested functionality
I added a new (cleaner) patch containing the code for rate in job. Previous patch was not correct - it lacked the ability to update the price in vendor bills. The patch also contains the fix to job xml backend to save slots. There is also another patch to remove an (unreachable) line that I accidentally left behind. Finally, I attached a minimal test case for the functionality of rate in job. I executed the test case in my system and it looks ok. Any testers please follow at least this test case (additional ones are more than welcome).
Comment on attachment 274560 [details] [review] New patch including the fix for slot saving in job xml backend Thank you for your patch. I have read through it and this is my feedback: 1. The code to load/save the job kvp looks ok. Still I would like to ask you to make this part of a separate patch because it is subject to the feature discussion that was going on earlier in the bug comments. And depending on the outcome it may need some additional tweaks. 2. The gncJobSetRate and gncJobGetRate functions have unresolved merge conflicts (marked with <<<<<<<, ======, and >>>>> markers). That should not happen in a patch as this won't compile. Can you correct this ?
Comment on attachment 274561 [details] [review] Removal of accidentally left-over unreachable code Oh and while fixing the patch you can include this minor change in the main patch as well.
Comment on attachment 274560 [details] [review] New patch including the fix for slot saving in job xml backend One additional suggestion for better backwards compatibility: if no rate has been set you assume 0 as default. That's good. However the way the job dialog works it will always set a rate (0 or otherwise). Hence the kvp will always be created. On the other hand to prevent data loss (loss of the kvp) older versions of GnuCash will not be allowed to open a data file containing a job kvp. So if you create a job in a gnucash version that will have the rate functionality yet don't use the rate functionality (leave the rate to 0) then you create a data file that can no longer be opened in older gnucash versions. To solve this I propose to modify the gncJobSetRate function as follows: If the rate to set is 0, then don't create the kvp rate value and instead remove any pre-existing one.
In general: the patches above have two parts: 1. fix the gncJob kvp bug in the xml backend. 2. Introduce a new kvp that can be used to prefill an invoice's price field. While we wait for Michalis to fix the few minor issues I pointed out I'd like to discuss to which branch to apply the fixes and how to guard for potential data loss. The patch for the xml backend bug should in any case be applied to 2.6.x. The faster this bug gets fixed the better. By our maintenance rules the second part should only be applied to master as this is a new feature. At some point in time we will be in a situation that users create jobs with the new gnucash version that has the rate feature (2.8.x+) and the same file may be opened on a gnucash version that still has to kvp bug (<2.6.4). That situation could lead to data loss if the user makes changes to the data file in the older version. So we still need to protect against this. I have been pondering Derek's suggestion in comment 19. I'm currently not sure if this is practically feasible. The best place to catch "Anytime a job kvp is used" would in job_dom_tree_create. I doubt however that we can call the feature code at that point. The feature code would add a new kvp to the book object. job_dom_tree_create is called when the data file is saved. I don't think that we can alter the data model while creating the dom tree which is what the feature code would do. Is there some place in the gncJob code that gets called whenever the job kvp slot is altered ?
Hi, I'll separate and fix the patches. In the meantime, I noticed that the following xml handling entities do NOT contain slot handling functionality (i.e. slot_handler function just returns true): address (gnc-address-xml-v2.c) entry (gnc-entry-xml-v2.c) order (gnc-order-xml-v2.c) although I don't know if orders are still in use somewhere taxtable (gnc-tax-table-xml-v2.c) Based on comment 23, should they be fixed too? If yes, I can attempt to do so, in one single patch.
(In reply to comment #32) > Hi, > I'll separate and fix the patches. > Thanks > In the meantime, I noticed that the following xml handling entities do NOT > contain slot handling functionality (i.e. slot_handler function just returns > true): > > address (gnc-address-xml-v2.c) > entry (gnc-entry-xml-v2.c) > order (gnc-order-xml-v2.c) although I don't know if orders are still in use > somewhere > taxtable (gnc-tax-table-xml-v2.c) > > Based on comment 23, should they be fixed too? > If yes, I can attempt to do so, in one single patch. That would be great. The sooner these get fixed the better. order was never completely finished and is hence not in active use. But since the code is still there and may some day be finished, just add the handler for it as well. It shouldn't be much work.
> The best place to catch "Anytime a job kvp is used" would in job_dom_tree_create. No, it's not, because then it would work only for the XML backend. And while we're interested in "anytime a job kvp is used" for *this* iteration, next time we'll be interested in *which* kvp key is used, and the backend kvp functions don't care about what keys are being processed, they just get the key and the value and make a DOM tree from what they find. Besides, it's a bit late to set the flag when saving the file, especially if you're going to notify the user that it will break backwards compatibility. Use gncJobSetRate() to set the feature flag.
I probably should have written "Initial knee-jerk reflex would reveal job_dom_tree_create as best place to catch <anytime a job kvp is used>". I agree it's not the best place. I had a conversation with Derek on irc yesterday about this as well. He suggested to use gncJobCommitEdit instead of gncJobSetRate. In this particular case I tend to agree: the job rate feature doesn't break backwards compatibility in any way. Not having it only means that new invoices based on the job will not have a default price value. But that's how it always has been so there's no regression. The fact that the price stored in the kvp is lost when saving the data file again in an older version of gnucash *is* a problem. And that's the more generic "feature" we should protect in older gnucash versions. As said on irc this can be done by setting the feature flag in gncjobcommitedit if it detects the job's kvp is not empty. If in the future we end up adding a feature that sets a kvp value which would result in incorrect data processing if not used by the engine, then indeed we'd need to define a feature flag specifically for that feature. I don't believe that's the case for Job Rate though.
I have created a new bug, 728841 for the XML backend fixes. The patch against maint is there.
(In reply to comment #36) > I have created a new bug, 728841 for the XML backend fixes. The patch against > maint is there. I have noticed. I should have continued the discussion over there.
Created attachment 275074 [details] [review] Implementation of rate in job I believe it is ok. I tested and the rate is written, read and updated correctly. Additionaly the slots are deleted if the rate is zero. Nevertheless, please test extensively. What troubles me is that the line //if (gnc_numeric_eq (job->rate, rate)) return; in gncJobSetRate is commented because for rate == 0 it was always giving me true, even though the previous value was not 0. So I'm probably missing something.
Comment on attachment 275074 [details] [review] Implementation of rate in job Thank you for your update. Overall it looks quite good. A few more comments: - the line you commented out in gncJobSetRate is not working because you are comparing the value of job->rate with the new rate. However you never actually set job->rate ! You only set the kvp value. To fix you could compare the value stored in the kvp with the new rate or effectively set job->rate as well in gncJobSetRate. The same applies to the test in gncJobEqual. - git complains your patch introduces some whitespace errors (usually this means there is some trailing whitespace somewhere or indentation is a mix of spaces and tabs). This is not a big issue. I can easily have git fix this while applying the patch. I'm just mentioning it as perhaps you can configure your editor to avoid these errors.
Oh and just one more minor nit: Your patch subject is [Patch x/y] [Bug abc] - title When I run git am on such a patch the subject gets stripped to - title I don't suppose that's what you'd want. To get the bug number in the subject instead you should skip the square brackets around the bug number when you commit your work - at least if I remove them from your patch I get this subject: Bug abc - title
Thanks Geert, I understand now the point with job->rate not being set. It was a leftover from my initial attempt to have job->rate in xml file, not in kvp slot but never understood it. But that makes me think that the patch is no good. Is the line job->rate = gnc_numeric_zero (); needed in the initialization (gncJobCreate)? and is the line: gnc_numeric rate; needed in the struct _gncJob? should I remove these lines and resubmit?
(In reply to comment #41) > Thanks Geert, > I understand now the point with job->rate not being set. It was a leftover from > my initial attempt to have job->rate in xml file, not in kvp slot but never > understood it. > > But that makes me think that the patch is no good. There's a bug in you patch ;) > Is the line > job->rate = gnc_numeric_zero (); > > needed in the initialization (gncJobCreate)? > No. > and is the line: > > gnc_numeric rate; > needed in the struct _gncJob? > > should I remove these lines and resubmit? Yes. But on top of this also uncomment your line in in gncJobSetRate and compare the rate value currently in kvp with the new rate value. You can simply call gncJobGetRate to get the current kvp value.
Created attachment 275231 [details] [review] Rate in job implementation Yes another attempt
Review of attachment 275231 [details] [review]: Michalis, this patch looks good to me. I haven't applied it yet because I'm not sure if it needs some work after John Ralls' recent private-kvp branch merge. I will wait in this case for John to give his feedback as well.
Comment on attachment 275231 [details] [review] Rate in job implementation It's OK as is, though it could be streamlined a bit using qof_instance_[sg]et_kvp() instead of dealing with the frame directly.
Just for my understanding: it's ok because the kvp handling is internal. Or put differently the use of kvp doesn't leak via the gncjob 'public api' is that right ? I just wanted to be sure because it seemed to me your private-kvp branch also gobjectified the kvp data in many cases. But perhaps that's not needed here.
From what I understand, this patch was developed before qof_instance_[sg]et_kvp(). I'll be happy to update it, but as I am feeling lost with the myriads of changes, I'll reclone the git tree and start from scratch. In this case, I think it would be easier and faster (for me :)) if the current patch was applied, so that the new patch was only minimal and more focused.
That's fine with me. I'll apply it later today.
Comment on #46: Yes, that's right. The public API should reveal as little as possible -- ideally nothing at all -- about the implementation. That lets us change the implementation whenever and however we like without affecting any other code. Yeah, it would be better to use GObject properties instead of explicit getter/setter functions to avoid adding to the API, but I don't see that as sufficient reason to make Michalis rewrite the patch.
Comment on attachment 275231 [details] [review] Rate in job implementation There you go - patch applied. Thank you very much for your contribution!
Created attachment 276557 [details] [review] Replaced direct kvp_frame access with qof_instance_[sg]et_kvp I didn't find a way to replace kvp_frame_delete though. I also removed some temp lines that were left overs in other files. Finally, please review thoroughly as I'm not very confident for the proper use of qof_instance_... and g_value_... functions.
Comment on attachment 276557 [details] [review] Replaced direct kvp_frame access with qof_instance_[sg]et_kvp Hmm. I guess I wasn't thinking about that the right way. That's more code and more overhead; I guess I was thinking of it from the property code, where the GValue already exists. You did the GValue setting fine, except that it's probably better to put it on the stack. It lives and dies in scope so there's no benefit to putting it on the heap, which adds some overhead.
(In reply to comment #52) > (From update of attachment 276557 [details] [review]) > Hmm. I guess I wasn't thinking about that the right way. That's more code and > more overhead; I guess I was thinking of it from the property code, where the > GValue already exists. > So what's the path forward ? Leave it at what is committed so far and forget about using gvalue in this context ? Or is it still better to use a GValue for consistency with the other engine objects or because it may make future transition away from gobject easier ?
I think better to leave it as-is. There's no consistency issue because most of the rest of kvp access inside the engine is done just the way you originally wrote it. There's also no real benefit for moving away from GObject: GValue is very much a GObject concept. It's how they get around not having overloading and generics. C++ offers both and I expect we'll end up with a different architecture when we get to rewriting that part of the engine.
Comment on attachment 276557 [details] [review] Replaced direct kvp_frame access with qof_instance_[sg]et_kvp Marking this patch as rejected because it adds too much overhead. Sorry for the effort you put into it.
Hi I just upgraded to 2.6.5 and noticed that option is not there. Should I try to get through the code and find out why, or is there a decision somewhere that it will not be included? Or, finally, is there an option somewhere that I should enable? Thanks M.
As I wrote in my comment 32, the rate option will only be released in gnucash 2.8.0. That's our policy: new features go in to the next minor release (after 2.6 the next minor release will be 2.8) and not in a bugfix release (2.6.3 to 2.6.5 for example). Only bugfixes can go into a bugfix release. The part about missing kvp load-save was clearly a bug, and so it was added to 2.6.x. The rate feature for jobs on the other hand is not, so it went into the current development branch only which will eventually become 2.8. Unfortunately there is not release date for gnucash 2.8 yet. Expect 2017-2018. At the same time bugs/enhancement requests in bugzilla get closed as soon as they have been merged into our git repository. That's why this bug report was closed already (and why I'll close it again).
Thanks Geert, I had, indeed, not understood this information.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=728136. Please update any external references or bookmarks.