Description
Chris Cormack
2010-05-21 01:23:21 UTC
Created attachment 10164 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - Updated C4 Modules to add Collection Codes to stats table Created attachment 10165 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - DB Update Created attachment 10213 [details] [review] [SIGNED] Bugfix 4118 - Add Collection Code to Statistics Table - Updated C4 Modules to add Collection Codes to stats table Created attachment 10214 [details] [review] [SIGNED] Bugfix 4118 - Add Collection Code to Statistics Table - DB Update Add data as expected... Signing off. First patch is okay, just adds ccode to the stats table so you don't have to do a multi-table join to get it from items and deleteditems. The database update, though, looks a little janky: ALTER TABLE `statistics` ADD `ccode` VARCHAR( 80 ) NULL ; should be more like ALTER TABLE `statistics` ADD COLUMN `ccode` VARCHAR ( 10 ) AFTER `associatedborrower` The update message is also inaccurate ("Category code" instead of "collection code") Created attachment 10602 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - Fix DB Update Created attachment 10603 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - Updated C4 Modules to add Collection Codes to stats table Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 10604 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - DB Update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 10605 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - Fix DB Update Created attachment 10887 [details] [review] Bugfix 4118 - Add Collection Code to Statistics Table - Fix DB Update Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> QA comment: Why do you have + +=item DropAllForeignKeys($table) + in 1st patch ? Other than this, it's OK, and it passes ../qa/koha-qa.pl -c 3 answer my previous question (and remove it if it was not intended) to earn "passed QA" (In reply to comment #12) > QA comment: > > Why do you have > + > +=item DropAllForeignKeys($table) > + > in 1st patch ? That was an accident. I'll add another followup to remove it. Created attachment 11322 [details] [review] Bug 4118 - Add Collection Code to Statistics Table - QA Followup QA comment: I was about to push your patch (after squashing the 2 last follow-up and adding a removal of useless `` in updatedatabase), and, at the end, I won't, because I've found a tiny mistake: The update says: UPDATE statistics SET statistics.ccode = ( SELECT items.ccode FROM items WHERE statistics.itemnumber = items.itemnumber ) which seems OK OTOH you record a ccode only in Circulation.pm, which seems OK (you don't need to record the item ccode when a fine is paid, so nothing changed in C4::Accounts.pm) BUT: when you pay a fine because of a lost item, the itemnumber field *can* be set. In this case, you will have an itemnumber, but no ccode. Your updatedatabase will populate ccode anyway. So, 2 options: * populate ccode in Accounts.pm, when there is an item * UPDATE by discarding statistics.type='payment' or 'writeoff' (maybe there are other values, not 100% sure) Please choose and resubmit. I suggest you squash the 4 current patches, and add a follow-up for this QA question Created attachment 12172 [details] [review] Bug 4118 - Add Collection Code to Statistics Table Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 12173 [details] [review] Bug 4118 - Add Collection Code to Statistics Table - Followup > So, 2 options:
> * populate ccode in Accounts.pm, when there is an item
> * UPDATE by discarding statistics.type='payment' or 'writeoff' (maybe
> there are other values, not 100% sure)
>
> Please choose and resubmit. I suggest you squash the 4 current patches, and
> add a follow-up for this QA question
Since a payment may be for multiple items, I don't think we should add a ccode for payments, so I have chosen the latter option.
I have also squashed the earlier patches per your suggestion.
Patch pushed to master My database does not contain ccode, so I could not check that the updatedatabase really worked properly (at least the column is created) A nice follow-up could be to have the line "collection" not visible on reports/issues_stats.pl is there is nothing in the list. But it's not worth failing QA for that ;-) Created attachment 12792 [details] [review] Bug 4118: Follow up: Add ccode to kohastructure for stats CCode was added to the statistics table in this bug, but was not added to kohastructure.sql. This patch adds it with comment to the kohastructure. follow up needs sign off. Created attachment 12793 [details] [review] [SIGNED-OFF] Bug 4118: Follow up: Add ccode to kohastructure for stats CCode was added to the statistics table in this bug, but was not added to kohastructure.sql. This patch adds it with comment to the kohastructure. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Change back to enhancement after pushed, but marked critical right now. Created attachment 12795 [details] [review] Follow up to Bug 8585: add missing word to preference The preference added in this bug is missing the word 'day(s)'. This patch adds it in. Comment on attachment 12795 [details] [review] Follow up to Bug 8585: add missing word to preference Attached to the wrong bug. Add the new field statistics.ccode to kohastructure. Marked as Passed QA. follow-up pushed, (back to ENH) |