Bug 7679 - Add new filters to circulation statistics wizard
Summary: Add new filters to circulation statistics wizard
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact:
URL:
Keywords:
Depends on: 17495
Blocks: 18742
  Show dependency treegraph
 
Reported: 2012-03-08 14:31 UTC by delaye
Modified: 2018-12-03 20:04 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7679: circulation statistics wizard improvements (41.47 KB, patch)
2012-08-09 11:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.44 KB, patch)
2012-10-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
-Bug-7679-circulation-statistics-wizard-improvements.patch (41.48 KB, patch)
2013-03-18 14:05 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug-7679-Followup-circulation-statistics-wizard-impr.patch (1.50 KB, patch)
2013-03-18 14:06 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1007 bytes, patch)
2013-04-29 15:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2013-11-06 07:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.78 KB, patch)
2014-02-05 14:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.14 KB, patch)
2014-02-19 11:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.63 KB, patch)
2014-03-24 11:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.93 KB, patch)
2014-09-01 15:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: circulation statistics wizard improvements (41.47 KB, patch)
2015-05-17 14:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.49 KB, patch)
2015-05-17 14:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1012 bytes, patch)
2015-05-17 14:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2015-05-17 14:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.77 KB, patch)
2015-05-17 14:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.13 KB, patch)
2015-05-17 14:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.64 KB, patch)
2015-05-17 14:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.95 KB, patch)
2015-05-17 14:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: circulation statistics wizard improvements (41.76 KB, patch)
2016-01-04 05:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.49 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1012 bytes, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.77 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.13 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.64 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.95 KB, patch)
2016-01-04 05:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: circulation statistics wizard improvements (41.89 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.49 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1012 bytes, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.77 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.13 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.64 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.95 KB, patch)
2016-01-04 05:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 7679: circulation statistics wizard improvements (41.73 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.49 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1012 bytes, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.77 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.13 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.64 KB, patch)
2016-08-19 09:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.95 KB, patch)
2016-08-19 09:35 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: circulation statistics wizard improvements (41.29 KB, patch)
2016-09-12 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Followup: circulation statistics wizard improvements (1.49 KB, patch)
2016-09-12 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix criteria display (1012 bytes, patch)
2016-09-12 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Group different case values in the same row/column (3.71 KB, patch)
2016-09-12 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix several bugs in circulation statistics wizard (6.77 KB, patch)
2016-09-12 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Fix two SQL queries when using patron attributes (2.13 KB, patch)
2016-09-12 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Various fixes for circulation statistics wizard (3.64 KB, patch)
2016-09-12 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679: Display all table headers (case and accent sensitive) (2.95 KB, patch)
2016-09-12 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7679 - Follow-up for terminology (2.72 KB, patch)
2016-10-26 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7679: Follow-up to fix group by hour (1.72 KB, patch)
2016-10-26 16:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: circulation statistics wizard improvements (41.35 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Followup: circulation statistics wizard improvements (1.56 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Fix criteria display (1.06 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Group different case values in the same row/column (3.78 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Fix several bugs in circulation statistics wizard (6.84 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Fix two SQL queries when using patron attributes (2.20 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Various fixes for circulation statistics wizard (3.71 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7679: Display all table headers (case and accent sensitive) (3.02 KB, patch)
2016-10-26 17:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7679 - Follow-up for terminology (2.72 KB, patch)
2016-10-26 17:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7679: Follow-up to fix group by hour (1.72 KB, patch)
2016-10-26 17:20 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description delaye 2012-03-08 14:31:59 UTC
In the circulation wizard, add the following lines that can be used as row/column/filter: 
* extended attributes (all of them) 
* patron branch 
* issuing branch 
* item branch 
(today, there is only item branch, described as "library". With this improvement we will be able to see how many issues have been made from library X by patrons from library Y) 

* today, we can group period by dow, month, day, this request is also to add a "group by hour" option

notes : check that the limits of start and end intervals are included (Period, item call number).
Comment 1 Julian Maurice 2012-08-09 11:37:01 UTC Comment hidden (obsolete)
Comment 2 delaye 2012-08-28 13:38:40 UTC
i test in a sandbox

I can not get results and i don't no why....
Comment 3 Jonathan Druart 2012-10-29 15:21:51 UTC Comment hidden (obsolete)
Comment 4 MathildeF 2013-03-13 16:01:19 UTC
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)
Comment 5 Brendan Gallagher 2013-03-18 14:05:16 UTC Comment hidden (obsolete)
Comment 6 Brendan Gallagher 2013-03-18 14:06:03 UTC Comment hidden (obsolete)
Comment 7 Brendan Gallagher 2013-03-18 14:06:54 UTC
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 8 Katrin Fischer 2013-03-22 06:43:25 UTC
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 9 Katrin Fischer 2013-03-22 06:47:05 UTC
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 10 Katrin Fischer 2013-03-22 07:02:58 UTC
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).
Comment 11 Katrin Fischer 2013-03-22 10:08:26 UTC
<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 :)
Comment 12 Julian Maurice 2013-04-29 15:07:34 UTC
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)
Comment 13 Julian Maurice 2013-04-29 15:08:26 UTC Comment hidden (obsolete)
Comment 14 Mathieu Saby 2013-10-12 13:28:16 UTC
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
Comment 15 Mathieu Saby 2013-10-12 13:28:57 UTC
(in fact, tested on sandbox 3, with radio button "Count total items ")

M. Saby
Comment 16 Julian Maurice 2013-11-05 15:57:21 UTC
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".
Comment 17 Paul Poulain 2013-11-05 17:39:32 UTC
isn't that a bug in the dataset ? Seems silly to me to have "enseignant" and "Enseignant" being 2 different values...
Comment 18 Julian Maurice 2013-11-06 07:51:14 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2013-11-06 07:53:29 UTC
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.
Comment 20 Mathieu Saby 2013-11-29 21:49:44 UTC
(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
Comment 21 Koha Team AMU 2014-01-30 16:12:27 UTC
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".
Comment 22 Koha Team AMU 2014-01-31 13:02:45 UTC
(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.
Comment 23 Julian Maurice 2014-02-05 14:49:19 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2014-02-05 14:51:08 UTC
This patch should fix problems of comment 21
Comment 25 Koha Team AMU 2014-02-07 15:13:03 UTC
(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"
Comment 26 Julian Maurice 2014-02-19 11:45:47 UTC Comment hidden (obsolete)
Comment 27 Julian Maurice 2014-02-19 11:46:07 UTC
This time the problem should be fixed ;)
Comment 28 Koha Team AMU 2014-02-27 10:28:53 UTC
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 ! ;-) )
Comment 29 Julian Maurice 2014-03-24 11:16:53 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2014-03-24 11:18:20 UTC
Hopefully, I easily reproduced your problems. This patch fixed them for me, so I hope it will work for you too :)
Comment 31 paxed 2014-03-24 12:19:57 UTC
(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.
Comment 32 Julian Maurice 2014-09-01 15:34:12 UTC Comment hidden (obsolete)
Comment 33 Mark Tompsett 2015-05-17 14:08:29 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2015-05-17 14:08:44 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2015-05-17 14:08:50 UTC Comment hidden (obsolete)
Comment 36 Mark Tompsett 2015-05-17 14:08:55 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2015-05-17 14:09:02 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2015-05-17 14:09:08 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2015-05-17 14:09:14 UTC Comment hidden (obsolete)
Comment 40 Mark Tompsett 2015-05-17 14:09:21 UTC Comment hidden (obsolete)
Comment 41 Mark Tompsett 2015-05-17 14:16:00 UTC
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?
Comment 42 Mark Tompsett 2015-05-17 14:27:09 UTC
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.
Comment 43 Chris Cormack 2016-01-04 02:48:31 UTC
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
Comment 44 Mark Tompsett 2016-01-04 05:38:39 UTC
(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.
Comment 45 Mark Tompsett 2016-01-04 05:39:55 UTC Comment hidden (obsolete)
Comment 46 Mark Tompsett 2016-01-04 05:40:01 UTC Comment hidden (obsolete)
Comment 47 Mark Tompsett 2016-01-04 05:40:08 UTC Comment hidden (obsolete)
Comment 48 Mark Tompsett 2016-01-04 05:40:15 UTC Comment hidden (obsolete)
Comment 49 Mark Tompsett 2016-01-04 05:40:21 UTC Comment hidden (obsolete)
Comment 50 Mark Tompsett 2016-01-04 05:40:28 UTC Comment hidden (obsolete)
Comment 51 Mark Tompsett 2016-01-04 05:40:35 UTC Comment hidden (obsolete)
Comment 52 Mark Tompsett 2016-01-04 05:40:41 UTC Comment hidden (obsolete)
Comment 53 Mark Tompsett 2016-01-04 05:57:14 UTC Comment hidden (obsolete)
Comment 54 Mark Tompsett 2016-01-04 05:57:21 UTC Comment hidden (obsolete)
Comment 55 Mark Tompsett 2016-01-04 05:57:27 UTC Comment hidden (obsolete)
Comment 56 Mark Tompsett 2016-01-04 05:57:34 UTC Comment hidden (obsolete)
Comment 57 Mark Tompsett 2016-01-04 05:57:40 UTC Comment hidden (obsolete)
Comment 58 Mark Tompsett 2016-01-04 05:57:45 UTC Comment hidden (obsolete)
Comment 59 Mark Tompsett 2016-01-04 05:57:51 UTC Comment hidden (obsolete)
Comment 60 Mark Tompsett 2016-01-04 05:57:57 UTC Comment hidden (obsolete)
Comment 61 Marc Véron 2016-04-17 13:18:38 UTC
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.
Comment 62 Julian Maurice 2016-08-19 09:34:28 UTC Comment hidden (obsolete)
Comment 63 Julian Maurice 2016-08-19 09:34:34 UTC Comment hidden (obsolete)
Comment 64 Julian Maurice 2016-08-19 09:34:38 UTC Comment hidden (obsolete)
Comment 65 Julian Maurice 2016-08-19 09:34:43 UTC Comment hidden (obsolete)
Comment 66 Julian Maurice 2016-08-19 09:34:48 UTC Comment hidden (obsolete)
Comment 67 Julian Maurice 2016-08-19 09:34:52 UTC Comment hidden (obsolete)
Comment 68 Julian Maurice 2016-08-19 09:34:56 UTC Comment hidden (obsolete)
Comment 69 Julian Maurice 2016-08-19 09:35:01 UTC Comment hidden (obsolete)
Comment 70 Julian Maurice 2016-08-19 09:35:18 UTC
Patches rebased on master
Comment 71 Liz Rea 2016-09-12 02:34:28 UTC
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. :(
Comment 72 Julian Maurice 2016-09-12 07:14:36 UTC
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.
Comment 73 Julian Maurice 2016-09-12 07:14:41 UTC
Created attachment 55482 [details] [review]
Bug 7679: Followup: circulation statistics wizard improvements

Signed-off-by: Brendan <brendan@bywatersolutions.com>
Comment 74 Julian Maurice 2016-09-12 07:14:46 UTC
Created attachment 55483 [details] [review]
Bug 7679: Fix criteria display
Comment 75 Julian Maurice 2016-09-12 07:14:51 UTC
Created attachment 55484 [details] [review]
Bug 7679: Group different case values in the same row/column
Comment 76 Julian Maurice 2016-09-12 07:14:55 UTC
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))
Comment 77 Julian Maurice 2016-09-12 07:15:00 UTC
Created attachment 55486 [details] [review]
Bug 7679: Fix two SQL queries when using patron attributes

and remove some Perl warnings
Comment 78 Julian Maurice 2016-09-12 07:15:06 UTC
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.
Comment 79 Julian Maurice 2016-09-12 07:15:11 UTC
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
Comment 80 Julian Maurice 2016-09-12 07:16:29 UTC
Patches rebased on master
Comment 81 Biblibre Sandboxes 2016-10-10 14:39:11 UTC
Patch tested with a sandbox, by delaye <stephane.delaye@biblibre.com>
Comment 82 Katrin Fischer 2016-10-26 14:16:44 UTC
It looks like the sign-off from sandboxes went wrong here - all the patches got obsoleted :( I will try to undo.
Comment 83 Katrin Fischer 2016-10-26 15:44:09 UTC
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.
Comment 84 Katrin Fischer 2016-10-26 16:42:07 UTC
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.
Comment 85 Katrin Fischer 2016-10-26 17:19:25 UTC
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>
Comment 86 Katrin Fischer 2016-10-26 17:19:29 UTC
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>
Comment 87 Katrin Fischer 2016-10-26 17:19:34 UTC
Created attachment 56881 [details] [review]
[PASSED QA] Bug 7679: Fix criteria display

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 88 Katrin Fischer 2016-10-26 17:19:38 UTC
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>
Comment 89 Katrin Fischer 2016-10-26 17:19:43 UTC
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>
Comment 90 Katrin Fischer 2016-10-26 17:19:48 UTC
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>
Comment 91 Katrin Fischer 2016-10-26 17:19:53 UTC
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>
Comment 92 Katrin Fischer 2016-10-26 17:19:57 UTC
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>
Comment 93 Katrin Fischer 2016-10-26 17:20:04 UTC
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.
Comment 94 Katrin Fischer 2016-10-26 17:20:11 UTC
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.
Comment 95 Kyle M Hall 2016-10-28 11:57:27 UTC
Pushed to master for 16.11, thanks Julian, Jonthan, Katrin!
Comment 96 Jonathan Druart 2017-09-19 12:19:48 UTC
The patron library dropdown list does not the list of library (it has been pushed after bug 15758).