GNOME Bugzilla – Bug 638543
[PATCH] Custom counter formatting and value editing
Last modified: 2018-06-29 22:50:44 UTC
This is a patch series that achieves two things: * Allow using custom format strings for counters (to allow a different number of digits or a pre- or postfix to invoice numbers, for example). * Allow editing the format strings and the current counter values through the File->Properties GUI. A couple of remarks regarding the implementation. The first patch removes the "counter" method from the backends. This method was meant for an (atomic?) increment-and-get operation in backends, but since backends could not define "get" and "set" operations in a similar methods, this "counter" method would conflict with this patch series. Since neither the xml and dbi backend implement this method, I've removed it altogether. An alternative would be to implement an addition "get" and "set" method for counters. One of the patches introduces a function for validating format strings, to prevent (security) problems from invalid printf strings. This validator is a very simple parser that allows a string with a single gint64 format specifier surrounded by a literal pre- and postfix. I've considered using regular expressions for this, but pulling in a regex engine seemed too much to bother and making a special-cased validator manually seemed the easiest way to make this work and be secure. The last patch is a bit tricky: It defines the GUI components to edit the counter values and formats. This is done by adding a section (tab) to the File -> Properties dialog. However, all of the other options offered in that dialog are saved in the "options"/<section> kvp slot, whereas the counters (and the new counter formats) as well are saved in different kvp slots next to the "options" slot. Since the options machinery is quite generic and also used for report options, there isn't a very clean and obvious way to handle this situation. I've opted for a slightly ugly solution: Override the scm->kvp and kvp->scm methods on the options to simply ignore the "options"/<section> slot they get passed and just use "counters" or "counters_format" instead. I haven't been able to think of a really better way to handle this that didn't involve significant changes to the options/report framework, and I guess this way should be acceptable... One alternative to this would be to actually move the counter values into the "options"/"Counters" slot (and then probably use special option naming for the format strings, like "gncInvoice_format") so the counter options can become normal options. I don't suppose this is worth the trouble of the backwards incompatible change (also this would mean this feature would be delayed to at least 2.6...) I'll attach the patches in a minute, they're in git-format-patch format.
Created attachment 177377 [details] [review] Patch against r20023: 0001 - Remove the counter() method from the backends.
Created attachment 177378 [details] [review] Patch against r20023: 0002 - Split the qof_book_get_counter function.
Created attachment 177380 [details] [review] Patch against r20023: 0003 - Centralize the counter formatting in qofbook.
Created attachment 177381 [details] [review] Patch against r20023: 0004 - Make the various counter formats configurable.
Created attachment 177382 [details] [review] Patch against r20023: 0005 - Validate counter format strings before using them.
Created attachment 177383 [details] [review] Patch against r20023: 0006 - Add a GUI for the counter format and current value.
That's all six patches in the series. Sorry for the spam, folks... :-)
Created attachment 177384 [details] [review] Patch against r20023: 0006 - Add a GUI for the counter format and current value. W00ps, made a last-minute thinko while cleaning up the patches. I've replaced the 0006 patch with one that does actually work. I also just realized there is a seventh patch missing: To actually use the validator to check the format strings after they're entered in the GUI (that's why I made the validator actually return useful error messages. While writing this down, I also realized that those error messages should really be made translatable, which they aren't in the current patch.
Comment on attachment 177380 [details] [review] Patch against r20023: 0003 - Centralize the counter formatting in qofbook. This patch#3 might have implications on the python wrappers, but that's acceptable.
Comment on attachment 177384 [details] [review] Patch against r20023: 0006 - Add a GUI for the counter format and current value. This finally makes the (default) invoice ID counter editable from the GUI. Great!
Review of attachment 177377 [details] [review]: Removing the counter() method from the backend is a bad idea. It was put in there in order to allow the backends to provide an atomic counter. The fact that the SQL backend didn't implement it is arguably a bug; because we're single-user it doesn't matter now. HOWEVER if we do move to a multi-user system we will DEFINITELY need to have the database create/modify/increment the counter.
(In reply to comment #11) > Review of attachment 177377 [details] [review]: > > Removing the counter() method from the backend is a bad idea. It was put in > there in order to allow the backends to provide an atomic counter. The fact > that the SQL backend didn't implement it is arguably a bug; because we're > single-user it doesn't matter now. HOWEVER if we do move to a multi-user > system we will DEFINITELY need to have the database create/modify/increment the > counter. Err... I do not agree. To me, it is enough of an argument that this method hasn't been implemented so far anyway. This is purely a declaration without any implementation, which is useless for us as application project, and can be removed. If we do move to a multi-user system, we will have to modify so many things anyway (and more of the implemented ones instead of the non-implemented ones) that this single part here doesn't have to pay respect to those potential future plans.
The unimplemented "counter" of the QofBook can very well be implemented again at a later point in time. Until then, it doesn't matter at all whether the field exists but is unused, or it is just directly removed altogether as proposed by Matthijs here. Hence, I'll commit the patch series.
r20052 - r20057.
After thinking about Derek's comments for a while last week, I realized he did make a valid point. I agree with Christian that re-adding it would be trivial. However, right now the final patch in this series, that adds the GUI, quite heavily relies on saving the counter values into a kvp. There's already a bit of stretching the (report) options logic to save into a non-standard kvp, but I don't think the current code can handle saving to completely different place in any sane way. So as Christian says, implementing a proper multi-user system later on will be a lot of work anyway. However, now (re-)adding proper atomically incrementing counters to the dbi backend also needs some (unrelated) substantial refactoring of the options framework, not just making the qof code respect the counter method again (though I'm not sure that that is a problem). Anyway, good to see this patch series is in. Like I said, I still need to create the final patch to make formats entered in the GUI be validated, I'll just reopen this ticket when I get around to making that patch.
@Matthijs: Is it correct that the new options for the format don't have a default value? Does that mean by default the counter values cannot be used (because they need a %d as format)? Can you come up with a patch that adds a default value for the format which gives some plain numbers?
Christian, does this need attention or can it be closed again?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for. Thanks!
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=638543. Please update any external references or bookmarks.