Description
delaye
2012-03-08 14:31:59 UTC
Created attachment 11479 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names i test in a sandbox I can not get results and i don't no why.... Created attachment 13099 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Tested in sandbox (maroille) It works (I compared the results with report sql) just one thing : It seems that the limits of end intervals are included for Period and exclued for itemcallnumber ? (see Stéphane Delaye comment) Created attachment 16242 [details] [review] -Bug-7679-circulation-statistics-wizard-improvements.patch Created attachment 16243 [details] [review] Bug-7679-Followup-circulation-statistics-wizard-impr.patch Didn't install with git bz - but easily installed with git am. This works as advertised. I will be attaching the sign off the old fashion way. Marking as signed off. -Brendan Comment on attachment 16243 [details] [review] Bug-7679-Followup-circulation-statistics-wizard-impr.patch Review of attachment 16243 [details] [review]: ----------------------------------------------------------------- ::: reports/issues_stats.pl @@ +258,4 @@ > ( $line =~ /datetime/ ) ? @$filters[0] > : ( $line =~ /category/ ) ? @$filters[2] > : ( $line =~ /itemtype/ ) ? @$filters[3] > + : ( $line =~ /^branch/ ) ? @$filters[4] Hi Brendan, was this line changed by intention? Comment on attachment 16243 [details] [review] Bug-7679-Followup-circulation-statistics-wizard-impr.patch Review of attachment 16243 [details] [review]: ----------------------------------------------------------------- ::: reports/issues_stats.pl @@ +258,4 @@ > ( $line =~ /datetime/ ) ? @$filters[0] > : ( $line =~ /category/ ) ? @$filters[2] > : ( $line =~ /itemtype/ ) ? @$filters[3] > + : ( $line =~ /^branch/ ) ? @$filters[4] Hi Brendan, was this line changed by intention? 0) See comment about Brendan's patch. 1) I am a bit confused about some things in this patch, for example on master we get for the filters: 'Event issue' and with this patch it's 'AA issue'. 2) Also I note that the filters part before = is not translatable, but that's not a regression. 3) I would like to see Mathilde's comment addressed. 4) I think I need a test plan for creating a report that groups by hour (I have not many issues in my database, so it should be as general as possible). <del>2) Also I note that the filters part before = is not translatable, but that's not a regression</del> Scratch 2) - of course using the cases in the template will make those translatabale :) Hi Katrin, 0) Yes, /branch/ is changed to /^branch/ to distinguish from /homebranch/ and /holdingbranch/ 1) This is a bug + debug residues (I'll write a patch) 3) It seems that Mathilde is right: date filters are inclusive, and end itemcallnumber filter is exclusive (for this one it is explicitly mentioned) Is this a problem? 4) All you need is some issues made at different hours (check statistics.datetime). - chose "Period" for row and select "group by hour" - chose another for column (for example categorycode) - submit - values in row N represent how much issues have been made between N:00 (inclusive) and (N+1):00 (exclusive) Created attachment 17798 [details] [review] Bug 7679: Fix criteria display Hi Nice enh Tested on sandbox 1 * Test of Sort by hours: seems OK Period From 03/09/2013 Period To 04/09/2013 Library = MAURES Home branch = MAURES Holding branch = MAURES Event = issue Display by = 4 datetime / itemtype Compact Disque Livre audio Livre Revue TOTAL 9 0 0 0 1 0 0 1 10 0 0 0 2 0 0 2 11 0 0 0 3 0 0 3 In database : 2013-09-03 09:45:23 MAURES 0.0000 issue 5464 LIVR 6068 2013-09-03 10:57:34 MAURES 0.0000 issue 4862 LIVR 7570 2013-09-03 10:57:44 MAURES 0.0000 issue 1338 LIVR 7570 2013-09-03 11:19:39 MAURES 0.0000 issue 5663 LIVR 3488 2013-09-03 11:29:47 MAURES 0.0000 issue 2171 LIVR 3431 2013-09-03 11:39:31 MAURES 0.0000 issue 4350 LIVR 5212 * test of flter by itemtype : seems ok (same kind of test) * test of patron attribute filter : - display all checkouts between 01/08/2013 and 04/09/2013 ; dates in columns ; professions in rows => I did not check database, but presentation of results seemed ok : agent administratif 0 0 0 0 0 0 0 0 0 0 0 agent d'accueil 0 0 0 0 0 0 0 0 0 0 0 enfant 0 0 1 0 0 0 0 0 0 0 1 Enseignant 0 0 1 0 0 0 0 0 0 0 1 etc. - use a value of attribute Profession as a filter : value "Enseignant" display only one line => ok PROFESSION / datetime 9 10 11 12 13 14 15 16 17 18 TOTAL Enseignant 0 0 1 0 0 0 0 0 0 0 1 TOTAL 0 0 1 0 0 0 0 0 0 0 3 ============ But, at that point, I remarked a bug : TOTAL is wrong : the last column is displaying "3", while the last column for the only row ('Enseignant') is 1. So, for Koha, the "sum" of 1 is 3. Wrong somewhere Other remarks (cosmetic) I think "Item type" should be better understood if named "Document type". That make obvious that is using biblioitems.itemtype (I suppose so?) and not items.itype. In the results, the " Display by = [1...4] " line is not easy to understand. It seems that each number match a kind of date grouping (hours, months...). Could you display it in plain english? Could you check the calculation of "TOTAL" line? M. Saby (in fact, tested on sandbox 3, with radio button "Count total items ") M. Saby The problem is that PROFESSION attribute has equivalent values with different cases. mysql> SELECT attribute FROM borrower_attributes WHERE code = 'PROFESSION' AND borrower_attributes.attribute is not null AND borrower_attributes.attribute LIKE 'Enseignant'; +------------+ | attribute | +------------+ | Enseignant | | enseignant | +------------+ The mysql GROUP BY clause is case insensitive, so in the process we lose one of those values mysql> SELECT attribute FROM borrower_attributes WHERE code = 'PROFESSION' AND borrower_attributes.attribute is not null AND borrower_attributes.attribute LIKE 'Enseignant' group by attribute; +------------+ | attribute | +------------+ | Enseignant | +------------+ The TOTAL takes into account 'Enseignant' and 'enseignant' values but does not display the 'enseignant' value because of that. I don't know what is the best way to solve this issue. Should we continue to display only one line but display all values in the same line? Or should we display multiple lines for every "case-variant"? Changing status to "In discussion". isn't that a bug in the dataset ? Seems silly to me to have "enseignant" and "Enseignant" being 2 different values... Created attachment 22745 [details] [review] Bug 7679: Group different case values in the same row/column This is a proposal patch for the first mentioned option : display only one line with all "case-variant" values in it. The 2nd option seems to require more work. (In reply to Paul Poulain from comment #17) > isn't that a bug in the dataset ? Seems silly to me to have "enseignant" and > "Enseignant" being 2 different values... Yes, that would be silly! Mathieu I tested on a sandbox (langres) by comparing the results with sql reports. It seems that the main part of the patch is ok but I still notice two problems : - test of sort by hours : if we choose the same date in "period", for example "from 30/01/2014 To 30/01/2014", group by hour doesn't work. (nevertheless, limits of end intervals are included for period ) - test of patron attributes : if we don't use filter and display all the values, total is still wrong. I notice that the lign with the value "NULL" is empty. Maybe only patron attributes with an authorised values list should be added in this patch ? PROFESSION / datetime 9 10 11 13 14 15 16 17 TOTAL agent administratif 0 0 0 0 1 1 0 0 2 écolier 0 0 0 0 1 0 0 0 1 NULL TOTAL 0 0 0 0 2 1 0 0 58 For that reason, changing status to "In discussion". (In reply to Julian Maurice from comment #19) > This is a proposal patch for the first mentioned option : display only one > line with all "case-variant" values in it. > > The 2nd option seems to require more work. It's ok for this patch. "agent administratif" and "Agent administratif" are included in the same row. Created attachment 25053 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) This patch should fix problems of comment 21 (In reply to Julian Maurice from comment #23) > Created attachment 25053 [details] [review] [review] > Bug 7679: Fix several bugs in circulation statistics wizard > - Display values in NULL columns/rows > - Display columns/rows that contain values only in NULL rows/columns --> It still does not work with patron attributes. It displays only the value NULL in rows or column and total is false because other value should be displayed. Filtered on: Period From 07/02/2014 Period To 07/02/2014 Event = issue PROFESSION / branch BDP MAURES TOTAL NULL 0 55 55 TOTAL 0 55 56 > - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) --> It's ok. > - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) --> It's ok Because of patron attributes, I change status in "Failed QA" Created attachment 25443 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings This time the problem should be fixed ;) So sorry, problem is not fixed ! Test options : Test on sandbox (Langres) Filtered on: Period From 01/01/2012 To 31/12/2012 Event = issue Column = Library (all values) Cell value : Count total items --> in all results (except the following one), with differents options in rows , total = 6251 - test on row = Patron attribute (all values) Output : to screen into the browser --> The last row "TOTAL" does not make sense (0 + 6237 = 6251 !!!) PROFESSION / branch BDP MAURES TOTAL agent administratif 0 0 0 (...) Urbaniste 0 7 7 NULL 0 6063 6063 TOTAL 0 6237 6251 Output : To a file --> The last row "TOTAL" does not appear but when I work out the total, I have total sum for each column = 6237. - test on row = patron sort 1 or row=patron sort 2 borrowers.sort1 / branch BDP MAURES TOTAL (...) NULL 0 2317 2317 TOTAL 0 8568 6251 --> Total of the 2nd column is higher than total sum - test on row = itemtype or row = shelving location itemtype / branch BDP MAURES TOTAL UNKNOWN VALUE 0 0 0 Compact Disque 0 463 463 Livre audio 0 24 24 Livre 0 5041 5041 Revue 0 688 688 UNKNOWN VALUE 0 35 35 TOTAL 0 6251 6251 --> there are many rows "Unknown value" with differents results Change status in "Failed QA" (hope so to change status in "signed off" soon ! ;-) ) Created attachment 26531 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Hopefully, I easily reproduced your problems. This patch fixed them for me, so I hope it will work for you too :) (In reply to Julian Maurice from comment #29) > Created attachment 26531 [details] [review] [review] > Bug 7679: Various fixes for circulation statistics wizard > > - use Text::Unaccent to remove accents from columns or rows values when > accessing %table. This is required as MySQL consider as equals two > strings that differ only by their accents when using GROUP BY clause. In some languages some of these "accented" letters are separate letters of the alphabet, and should not be "unaccented". For example, in Finnish, Ä (latin capital a with diaeresis) should not be turned into A. Created attachment 31311 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Created attachment 39250 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 39251 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 39252 [details] [review] Bug 7679: Fix criteria display Created attachment 39253 [details] [review] Bug 7679: Group different case values in the same row/column Created attachment 39254 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Created attachment 39255 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Created attachment 39256 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Created attachment 39257 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Comment on attachment 39251 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Review of attachment 39251 [details] [review]: ----------------------------------------------------------------- ::: reports/issues_stats.pl @@ +251,4 @@ > ( $line =~ /datetime/ ) ? @$filters[0] > : ( $line =~ /category/ ) ? @$filters[2] > : ( $line =~ /itemtype/ ) ? @$filters[3] > + : ( $line =~ /^branch/ ) ? @$filters[4] issuingbranch branchcode Unchanged, both match. This change only matches the last one. Katrin raises a valid point: Is this really a desired change? This should now cleanly apply. It is identical change-wise as far as I can tell. Setting this to 'Needs Signoff' since Julian posted patchs as a result of the 'Failed QA' starting in comment #28. Falling back to patching base and 3-way merge... Auto-merging reports/issues_stats.pl CONFLICT (content): Merge conflict in reports/issues_stats.pl Failed to merge in the changes. Not a simple conflict (In reply to Chris Cormack from comment #43) > Falling back to patching base and 3-way merge... > Auto-merging reports/issues_stats.pl > CONFLICT (content): Merge conflict in reports/issues_stats.pl > Failed to merge in the changes. > > > Not a simple conflict Most of it seemed to be whitespace indentation issues. I'll upload a fix, and then do a comparison to make sure nothing seems lost. Created attachment 46184 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names NOTE: Removed signed off, since I believe I have dealt with the conflicts. Retesting required. Created attachment 46185 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 46186 [details] [review] Bug 7679: Fix criteria display Created attachment 46187 [details] [review] Bug 7679: Group different case values in the same row/column Created attachment 46188 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Created attachment 46189 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Created attachment 46190 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Created attachment 46191 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Created attachment 46192 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names NOTE: Removed signed off, since I believe I have dealt with the conflicts. Retesting required. Found some comments C4::Date that I missed. line prepatch 194/ postpatch 233 seems to be where the major conflict is. Created attachment 46193 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 46194 [details] [review] Bug 7679: Fix criteria display Created attachment 46195 [details] [review] Bug 7679: Group different case values in the same row/column Created attachment 46196 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Created attachment 46197 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Created attachment 46198 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Created attachment 46199 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Does not apply: Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt Failed to merge in the changes. Created attachment 54598 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names NOTE: Removed signed off, since I believe I have dealt with the conflicts. Retesting required. Found some comments C4::Date that I missed. line prepatch 194/ postpatch 233 seems to be where the major conflict is. Created attachment 54599 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 54600 [details] [review] Bug 7679: Fix criteria display Created attachment 54601 [details] [review] Bug 7679: Group different case values in the same row/column Created attachment 54602 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Created attachment 54603 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Created attachment 54604 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Created attachment 54605 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Patches rebased on master Applying: Bug 7679: circulation statistics wizard improvements Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt M reports/issues_stats.pl Falling back to patching base and 3-way merge... Auto-merging reports/issues_stats.pl CONFLICT (content): Merge conflict in reports/issues_stats.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt Failed to merge in the changes. Patch failed at 0001 Bug 7679: circulation statistics wizard improvements Very sorry. :( Created attachment 55481 [details] [review] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names NOTE: Removed signed off, since I believe I have dealt with the conflicts. Retesting required. Found some comments C4::Date that I missed. line prepatch 194/ postpatch 233 seems to be where the major conflict is. Created attachment 55482 [details] [review] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Created attachment 55483 [details] [review] Bug 7679: Fix criteria display Created attachment 55484 [details] [review] Bug 7679: Group different case values in the same row/column Created attachment 55485 [details] [review] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Created attachment 55486 [details] [review] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Created attachment 55487 [details] [review] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Created attachment 55488 [details] [review] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Patches rebased on master Patch tested with a sandbox, by delaye <stephane.delaye@biblibre.com> It looks like the sign-off from sandboxes went wrong here - all the patches got obsoleted :( I will try to undo. Created attachment 56877 [details] [review] Bug 7679 - Follow-up for terminology Patch changes "Library" to "Issuing library" and "Patron branch" to "Patron library". It also changes the text of the filters to match the terms and spelling on the form itself. Created attachment 56878 [details] [review] Bug 7679: Follow-up to fix group by hour Grouping by hour does not work with the collates. This patch removes them after talking to Julian on IRC. This partially reverts one of the earlier patches: Bug 7679: Display all table headers (case and accent sensitive) One could argue that this is more of a data problem, than something we should fix/'hide' with code. Created attachment 56879 [details] [review] [PASSED QA] Bug 7679: circulation statistics wizard improvements - Add patron branch to the list of possible rows, columns, and filters - Add extended patron attributes to the list of possible rows, columns, and filters - Allow to group period by hour - Allow to translate filter names NOTE: Removed signed off, since I believe I have dealt with the conflicts. Retesting required. Found some comments C4::Date that I missed. line prepatch 194/ postpatch 233 seems to be where the major conflict is. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56880 [details] [review] [PASSED QA] Bug 7679: Followup: circulation statistics wizard improvements Signed-off-by: Brendan <brendan@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56881 [details] [review] [PASSED QA] Bug 7679: Fix criteria display Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56882 [details] [review] [PASSED QA] Bug 7679: Group different case values in the same row/column Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56883 [details] [review] [PASSED QA] Bug 7679: Fix several bugs in circulation statistics wizard - Display values in NULL columns/rows - Display columns/rows that contain values only in NULL rows/columns - "To" filter in period row should be inclusive (YYYY-MM-DD should be YYYY-MM-DD 23:59:59) - Make it possible to use only the "To" filter in period row (actually it results in DBI error (2 bind variables instead of 1)) Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56884 [details] [review] [PASSED QA] Bug 7679: Fix two SQL queries when using patron attributes and remove some Perl warnings Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56885 [details] [review] [PASSED QA] Bug 7679: Various fixes for circulation statistics wizard - use SQL TRIM functions to avoid having '' and ' ' considered as different values - use Text::Unaccent to remove accents from columns or rows values when accessing %table. This is required as MySQL consider as equals two strings that differ only by their accents when using GROUP BY clause. - Exclude '' values from the list of columns or rows. Otherwise we could have a row 'UNKNOWN VALUE' and a row 'NULL' which both have the same values in their cells. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56886 [details] [review] [PASSED QA] Bug 7679: Display all table headers (case and accent sensitive) It appears that we can't assume that a lowercase word is equivalent to the same word in uppercase (or a mix of lowercase and uppercase letters). And this is also true about accented words. So display all values even if they differ only by case or by their accents. This means that: - "foobar" and "Foobar" will be in two separate rows/columns - "Etudiant" and "Étudiant" (french) will be in two separate rows/columns too Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 56887 [details] [review] Bug 7679 - Follow-up for terminology Patch changes "Library" to "Issuing library" and "Patron branch" to "Patron library". It also changes the text of the filters to match the terms and spelling on the form itself. Created attachment 56888 [details] [review] Bug 7679: Follow-up to fix group by hour Grouping by hour does not work with the collates. This patch removes them after talking to Julian on IRC. This partially reverts one of the earlier patches: Bug 7679: Display all table headers (case and accent sensitive) One could argue that this is more of a data problem, than something we should fix/'hide' with code. Pushed to master for 16.11, thanks Julian, Jonthan, Katrin! The patron library dropdown list does not the list of library (it has been pushed after bug 15758). |