Bug 9811

Summary: Patrons search improvements
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: boutrosboutrosboutros, cedric.vita, chris, christophe.croullebois, dpavlin, francois.pichenot, fridolin.somers, gitbot, gmcharlt, jean-manuel.broust, katrin.fischer, koha.aixmarseille, kyle.m.hall, kyle, liz, mathsabypro, oleonard, olli-antti.kivilahti, paul.poulain, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10155
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10565    
Bug Blocks: 12595, 12833, 13458, 13891, 17375, 24076    
Attachments: Bug 9811: Patron search improvement
Bug 9811: Take into account of Independent branches
Bug 9811: Take into account of Independant branches
Bug 9811: replaces " " with " "
Bug 9811: Patron search improvement
Bug 9811: Patron search improvement
Bug 9811: Add unit tests for C4::Utils::DT::Members
Bug 9811: Patron search improvement
Bug 9811: Add unit tests for C4::Utils::DT::Members
Bug 9811: QA followup
Patron module start page
Bug 9811: QA followup
[PASSED QA] Bug 9811: Patron search improvement
[PASSED QA] Bug 9811: Add unit tests for C4::Utils::DT::Members
[PASSED QA] Bug 9811: QA followup
Bug 9811 - multilines notes breaks JSON
Bug 9811: use count(primary_key) instead of count(*)
Bug 9811: A limit clause should be always added.
Bug 9811: Load the page without any data.
Bug 9811: Patron search improvement
Bug 9811: Add unit tests for C4::Utils::DT::Members
Bug 9811: QA followup
Bug 9811 - multilines notes brakes JSON
Bug 9811: use count(primary_key) instead of count(*)
Bug 9811: A limit clause should be always added.
Bug 9811: Load the page without any data.
Bug 9811: Patron search improvement
Bug 9811: Patron search improvement
Bug 9811: Patron search improvement
Bug 9811: FIX branches loop was not correctly set
Bug 9811: Patron search improvement
Bug 9811: FIX branches loop was not correctly set
Bug 9811: Fixing JSON validation error for non-javascript string literal characters
Bug 9811: FIX encoding issue on patron categories
Bug 9811: FIX encoding issue on patron categories
Bug 9811: Patron search improvement
Bug 9811 - datetime does not respect DateFormat-system prefrence
Bug 9811: Patron search improvement
Bug 9811: FIX branches loop was not correctly set
Bug 9811: Fixing JSON validation error for non-javascript string literal characters
Bug 9811: FIX encoding issue on patron categories
Bug 9811 - datetime does not respect DateFormat-system prefrence
Bug 9811: QA follow-up: Fix license version
Bug 9811: Patron search improvement
Bug 9811: FIX branches loop was not correctly set
Bug 9811: Fixing JSON validation error for non-javascript string literal characters
Bug 9811: FIX encoding issue on patron categories
Bug 9811 - datetime does not respect DateFormat-system prefrence
Bug 9811: QA follow-up: Fix license version
Bug 9811: Display email address in patron result list
Bug 9811: Remove useless orderby management
Bug 9811: Various fixes
Build patrons
Bug 9811: Cardnumber could be alphanumeric
Bug 9811: Fix wording branch vs library
[PASSED QA] Bug 9811: Patron search improvement
[PASSED QA] Bug 9811: FIX branches loop was not correctly set
[PASSED QA] Bug 9811: Fixing JSON validation error for non-javascript string literal characters
[PASSED QA] Bug 9811: FIX encoding issue on patron categories
[PASSED QA] Bug 9811 - datetime does not respect DateFormat-system prefrence
[PASSED QA] Bug 9811: QA follow-up: Fix license version
[PASSED QA] Bug 9811: Display email address in patron result list
[PASSED QA] Bug 9811: Remove useless orderby management
[PASSED QA] Bug 9811: Various fixes
[PASSED QA] Bug 9811: Cardnumber could be alphanumeric
[PASSED QA] Bug 9811: Fix wording branch vs library
Bug 9811 [QA Followup] - Enable bProcessing

Description Jonathan Druart 2013-03-13 16:11:04 UTC

    
Comment 1 Jonathan Druart 2013-03-13 16:16:01 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-03-14 09:31:39 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-03-14 09:33:00 UTC Comment hidden (obsolete)
Comment 4 Mathieu Saby 2013-03-17 16:32:20 UTC
If I click on "edit" and show the record of a patron, or if there is only one result to my search and Koha directs me to the right record, there is a problem :
If I use the "Back" button of my browser to go back to members-home page, after 1 or 2 seconds, Koha leaves members-home and redirect me automaticaly to the record I was consulting.

An other detail : I note this patch does not change the current behavior of " Browse by last name: A B C D E F G H I J K L M N O P Q R S T U V W X Y Z ", which is often disturbing.
Librarians often think clicking on "M" after a first search will act as a "facet", showing only names begining with "M" among the results of the first search.

But this is not the aim of your patch, so maybe someone will make a patch for improving this behavior later ;-)


M. Saby
Comment 5 Jonathan Druart 2013-03-18 13:06:39 UTC
(In reply to comment #4)
> If I click on "edit" and show the record of a patron, or if there is only
> one result to my search and Koha directs me to the right record, there is a
> problem :
> If I use the "Back" button of my browser to go back to members-home page,
> after 1 or 2 seconds, Koha leaves members-home and redirect me automaticaly
> to the record I was consulting.

I don't know how I can fix this problem.
I can remove the redirection but it is the current behavior on master.
If you click on back, your browser automatically refill the filter and Datatable still returns 1 result.

> An other detail : I note this patch does not change the current behavior of
> " Browse by last name: A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
> ", which is often disturbing.
> Librarians often think clicking on "M" after a first search will act as a
> "facet", showing only names begining with "M" among the results of the first
> search.
> 
> But this is not the aim of your patch, so maybe someone will make a patch
> for improving this behavior later ;-)

Yes, it the not the goal of this patch. It could be improve later.
Comment 6 Mathieu Saby 2013-03-18 13:16:29 UTC
> I don't know how I can fix this problem.
> I can remove the redirection but it is the current behavior on master.
> If you click on back, your browser automatically refill the filter and Datatable still returns 1 result.

I don't know javascript, but is not there a way to prevent that behavior?
And it won't be a problem if the browser only refill the filter but stay on members-home.pl instead of going again to the record of the patron.

M. Saby
Comment 7 Fridolin Somers 2013-03-18 16:35:00 UTC
There is a problem with the patch, some whitespace are rare, they look like a A :
    { 'mDataProp': 'dt_name'Â }
See patch in browser.
Comment 8 Jonathan Druart 2013-03-19 08:21:19 UTC Comment hidden (obsolete)
Comment 9 Fridolin Somers 2013-03-19 08:51:45 UTC
the follow-up [Bug 9811: replaces " " with " " does not work].
The git am returns : 

Traceback (most recent call last):
  File "/home/koha/bin/git-bz", line 2141, in <module>
    do_apply(*args)
  File "/home/koha/bin/git-bz", line 1485, in do_apply
    print patch.description

Maybe I should be better to update the patch : [Bug 9811: Take into account of Independant branches]
Comment 10 Jonathan Druart 2013-03-19 10:24:40 UTC Comment hidden (obsolete)
Comment 11 Christophe Croullebois 2013-03-19 14:00:25 UTC
ok for me the patch applies correctly
Comment 12 Paul Poulain 2013-03-19 14:56:20 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 13 Paul Poulain 2013-03-19 14:56:38 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-03-27 12:17:36 UTC
We need unit tests for this in a follow up - can then be switched back to signed off of course.
Comment 15 Jonathan Druart 2013-03-28 10:20:12 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-06-17 08:48:24 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-06-17 08:48:57 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2013-06-30 20:07:42 UTC
I really like the idea to use datatables for the patron search page, but I found a few problems with the patches.

1) The patches don't pass the QA script:

* koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc               FAIL
forbidden patterns          FAIL
The patch introduces a forbidden pattern: tabulation character (95)
The patch introduces a forbidden pattern: tabulation character (94)

* C4/Utils/DataTables/Members.pm                                           FAIL
pod                         OK
forbidden patterns          OK
valid                       FAIL
Use of qw(...) as parentheses is deprecated 

2) The new C4/Utils/DataTables/Members.pm adds mysqlisms:

my $select = "SELECT SQL_CALC_FOUND_ROWS
"SELECT FOUND_ROWS()";
Comment 19 Jonathan Druart 2013-07-01 08:44:18 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2013-07-01 08:45:27 UTC
(In reply to Katrin Fischer from comment #18)

> * koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc              
> FAIL
> forbidden patterns          FAIL

Fixed.

> * C4/Utils/DataTables/Members.pm                                          
> FAIL
> pod                         OK
> forbidden patterns          OK
> valid                       FAIL
> Use of qw(...) as parentheses is deprecated 

Strange, I don't have this failure. And perl -wc returns OK:
  perl -wc C4/Utils/DataTables/Members.pm
    C4/Utils/DataTables/Members.pm syntax OK

> 2) The new C4/Utils/DataTables/Members.pm adds mysqlisms:

Fixed.
Comment 21 Katrin Fischer 2013-07-14 15:21:37 UTC
The tests are all passing now - I think my qa-tools were not up to date at that point :)
Comment 22 Katrin Fischer 2013-07-14 15:42:07 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2013-07-14 15:43:47 UTC
On the screenshot are some more odd things, but they went away when I reloaded the page and JS. But you can see the little color difference in the datatables controls.
Comment 24 Jonathan Druart 2013-07-15 09:32:03 UTC
(In reply to Katrin Fischer from comment #22)

Thanks for the review Katrin!

> Created attachment 19633 [details]
> Patron module start page
> 
> Adding some notes about my findings here:
> 
> 1) (Normal) Sorting: I think it would be nice if fines, circ notes and
> checkouts would be sortable too. I know checkouts is a bit difficult, but if
> it was sortable you could easily find those with checkouts at least.

I will add the sort on the notes, but others are a little bit complicated.

> 2) (Trivial) There is some wrong capitalization, that was already there
> before and now appears in the filters too: Street Address > Street address

Will be done. I did not find others wrong capitalization.

> 3) (Major) Filters: I tried to limit the search to a specific patron
> category and while it said "Processing" nothing happened and the page seemed
> broken. I had to reload the page. Same happens when I search for a name
> using the normal search. Not sure what's going wrong here. I checked the
> logs, pasting some of the errors here:

There is 1 missing space in the built query, will be fixed.

> 4) (Trivial) There is something a little odd about the datatables CSS - the
> first part of the controls is a darker grey than the second part starting
> with "First".

Maybe it is wanted by Owen? It is not added by these patchs.
Bug 10558 already deals with datatable CSS.

> 5) Wishlist: Could we rename the column Cat = Category? I remember all too
> well when someone (probably me) translated it to "Katze" :) But not sure if
> that takes up too much room or not.

Cat => Category, done.
Comment 25 Jonathan Druart 2013-07-15 09:33:52 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2013-08-04 09:52:26 UTC
There is a lot I like about this :) I just would like some other people to take a look, as this is a really important feature/page for libraries/librarians.

Some notes from testing:

- The styling of the datatables toolbar is still a bit off/different to how it
  looks in other places like subscriptions. Maybe Owen could take a look?

- If no results are found it would be nicer to not display all the controls but
  just a short message.

- There is a big change in behaviour as the page loads all borrowers on loading 
  the page. I am worried about the performance of this for bigger databases.

- I wonder if we could also offer a general search field like we have for 
  subscriptions?

- The search options are doubled up now on the left and on the top. The search
  on the left is working a bit nicer for me, because it's easier to see and
  handle. It would be interesting what other people think about this.

Adding Owen CC in hope he might have some time to take a look. Also Liz as she has worked on the patron search in the past.
Comment 27 Katrin Fischer 2013-08-04 09:57:10 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2013-08-04 09:57:26 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2013-08-04 09:57:46 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2013-08-04 09:58:52 UTC
Funcionaltiy wise this works well for me, so found nothing to not pass QA :) - I'd just like to get some more comments and eyes on this.
Comment 31 Paul Poulain 2013-08-05 09:50:58 UTC
(In reply to Katrin Fischer from comment #30)
> Funcionaltiy wise this works well for me, so found nothing to not pass QA :)
> - I'd just like to get some more comments and eyes on this.

Hi Katrin, you're asking for more eyes, but I don't see any mail to warn/ask on mailing lists, so I fear that your request will stay as is. Maybe sending a mail to koha@ mailing list (with some screenshots to present the feature, and/or link to sandboxes for ppl to test it) ?
Comment 32 Katrin Fischer 2013-08-05 09:52:25 UTC
Sounds like a good idea :)
Comment 33 Owen Leonard 2013-08-05 12:47:51 UTC
I just tried this out with a copy of our production database of patrons (62,000+ records) and I don't get any results.

When I first load the page it looks like the table is trying to process *all* patron records? The POST response has this:

    "iTotalRecords": 62459,
    "iTotalDisplayRecords": 62459,

...but they are never displayed. I don't get any results from clicking on the "browse by last name" alphabet links, and I don't get any results after performing a simple search by last name. In all cases I see the "processing" message on the screen but nothing is ever displayed.
Comment 34 Chris Cormack 2013-08-06 01:29:11 UTC
(In reply to Owen Leonard from comment #33)
> I just tried this out with a copy of our production database of patrons
> (62,000+ records) and I don't get any results.
> 
> When I first load the page it looks like the table is trying to process
> *all* patron records? The POST response has this:
> 
>     "iTotalRecords": 62459,
>     "iTotalDisplayRecords": 62459,
> 
> ...but they are never displayed. I don't get any results from clicking on
> the "browse by last name" alphabet links, and I don't get any results after
> performing a simple search by last name. In all cases I see the "processing"
> message on the screen but nothing is ever displayed.

Ouch, let me test that on a big database also. Testing now
Comment 35 Chris Cormack 2013-08-06 01:33:51 UTC
Before the patches, 2.1s to load the page

After the patches, up around 3 mins before I hit stop, and yes it was loading every single borrower.

This is simply unusable with a library with a decent number of borrowers.
Comment 36 Fridolin Somers 2013-08-06 15:30:04 UTC
(In reply to Chris Cormack from comment #35)
> After the patches, up around 3 mins before I hit stop
I think searchable borrowers column need a SQL index : surname, ...
I'll try to propose a new bug.
Comment 37 Chris Cormack 2013-08-06 20:02:48 UTC
(In reply to Fridolyn SOMERS from comment #36)
> (In reply to Chris Cormack from comment #35)
> > After the patches, up around 3 mins before I hit stop
> I think searchable borrowers column need a SQL index : surname, ...
> I'll try to propose a new bug.

That might speed it up, but no that is not the problem. The problem is that page returns every single borrower. Doing it a little faster will still be wrong. You will just crash the browser faster that way.

There is no way we can have the whole borrower table loaded in and sorted with javascript. A better solution (lazy loading, server side chunking, or something similar will have to be done.)
Comment 38 Chris Cormack 2013-08-06 20:16:53 UTC
Oh and Galen just reminded me Datatables supports lazy loading, so it shouldnt be too hard to add that.
Comment 39 Fridolin Somers 2013-08-07 10:21:51 UTC
(In reply to Chris Cormack from comment #38)
> Oh and Galen just reminded me Datatables supports lazy loading, so it
> shouldnt be too hard to add that.

Yes, I also was thinking about Datatables, they already exist in many places in Koha.
Comment 40 Chris Cormack 2013-08-07 10:32:57 UTC
(In reply to Fridolyn SOMERS from comment #39)
> (In reply to Chris Cormack from comment #38)
> > Oh and Galen just reminded me Datatables supports lazy loading, so it
> > shouldnt be too hard to add that.
> 
> Yes, I also was thinking about Datatables, they already exist in many places
> in Koha.

Yes, and this set of patches adds them, but in doing so, it renders the members-home.pl page utterly unusable on any system with lots of borrowers.

I think you may be misunderstanding, these patches add datatables, but they also change the behaviour so when you visit the members page it fetches every single borrower from the database. This is not acceptable, hence the failed qa, nor will an index fix it. It either needs to be rewritten to lazy load, or to not fetch all borrowers, or some other fix
Comment 41 Fridolin Somers 2013-08-08 13:34:08 UTC Comment hidden (obsolete)
Comment 42 Fridolin Somers 2013-08-08 14:07:51 UTC
(In reply to Chris Cormack from comment #40)
> these patches add datatables, but they also change the behaviour
> so when you visit the members page it fetches every single borrower from the database.
In C4/Utils/DataTables/Members.pm :
The SQL query does not fetch all borrowers, it uses iDisplayStart and iDisplayLength to limit to the first page (usually 20 borrowers).
And when there is not filter, there is only  a select count(*) from borrowers, this should not take a long time.
Comment 43 Jonathan Druart 2013-08-12 10:55:38 UTC
I am not able to reproduce Owen and Chris issues.

(In reply to Owen Leonard from comment #33)
> I just tried this out with a copy of our production database of patrons
> (62,000+ records) and I don't get any results.
> 
> When I first load the page it looks like the table is trying to process
> *all* patron records? The POST response has this:
> 
>     "iTotalRecords": 62459,
>     "iTotalDisplayRecords": 62459,
> 
> ...but they are never displayed. I don't get any results from clicking on
> the "browse by last name" alphabet links, and I don't get any results after
> performing a simple search by last name. In all cases I see the "processing"
> message on the screen but nothing is ever displayed.

Without any errors in your koha logs (or js error)?

(In reply to Chris Cormack from comment #40)
> I think you may be misunderstanding, these patches add datatables, but they
> also change the behaviour so when you visit the members page it fetches
> every single borrower from the database. This is not acceptable, hence the
> failed qa, nor will an index fix it. It either needs to be rewritten to lazy
> load, or to not fetch all borrowers, or some other fix

This patch normally does that! :)

Of course, the goal of this patch is to retrieve only the xx first results, not all.

After applying these patches, I go on members/members-home.pl and the generated sql query is:

  SELECT borrowers.borrowernumber, borrowers.surname, borrowers.firstname, borrowers.address, borrowers.address2, borrowers.city, borrowers.zipcode, borrowers.country, CAST(borrowers.cardnumber AS UNSIGNED) AS cardnumber, borrowers.dateexpiry, borrowers.borrowernotes, borrowers.branchcode, categories.description AS category_description, categories.category_type, branches.branchname
  FROM borrowers
  LEFT JOIN branches ON borrowers.branchcode = branches.branchcode
  LEFT JOIN categories ON borrowers.categorycode = categories.categorycode
  ORDER BY borrowers.surname asc,borrowers.firstname asc
  LIMIT 0,20

The limit clause exists!

You can display this query in the log, warning the $query var in C4::Utils::DataTables::Members l.102.

Maybe the executing time issue is caused by the 2 COUNT(*) queries. But since we don't want the "SELECT SQL_CALC_FOUND_ROWS" mysqlism, I don't know how we can do that on a different way.

I will submit a patch replacing the count(*) with count(borrowers.borrowernumber), but I am not sure it could be useful.
Comment 44 Jonathan Druart 2013-08-12 10:56:56 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-08-12 11:44:04 UTC
With 35k patrons in the DB, the table is displayed in 2sec (on my dev instance).
Comment 46 Galen Charlton 2013-08-12 18:48:26 UTC
I've spent a bit of time with the current patch series, and I've *not* been able to reproduce the problem where going to members/member.pl induced DataTables to try loading the entire patron database.

Chris, Owen, could you try again?  I'd be particularly interested to know exact browser versions if you're still encountering the problem.

Looking at the code, I do see a place in C4/Utils/DataTables/Members.pm where some defensive coding is in order:

    my $limit;
    if(defined $dt_params->{iDisplayStart} and defined $dt_params->{iDisplayLength}) {
        # In order to avoir sql injection
        $dt_params->{iDisplayStart} =~ s/\D//g;
        $dt_params->{iDisplayLength} =~ s/\D//g;
        $limit = "LIMIT $dt_params->{iDisplayStart},$dt_params->{iDisplayLength}";
    }

The problem is that if iDisplayStart is never passed as a CGI parameter (because of a DataTables bug or some unrelated JavaScript bug interfering with the proper initialization of the DataTables object), the patron query will be unlimited.

To avoid that, a LIMIT clause should *always* be added to the patron query, with sensible default values if iDisplayStart and/or iDisplayLength are not supplied.  0 and 20, respectively, I'd say.

We may also want to consider hardcoding an absolute cap (say 100?) on iDisplayLength.
Comment 47 Galen Charlton 2013-08-12 18:53:36 UTC
(In reply to Jonathan Druart from comment #43)
> Maybe the executing time issue is caused by the 2 COUNT(*) queries. But
> since we don't want the "SELECT SQL_CALC_FOUND_ROWS" mysqlism, I don't know
> how we can do that on a different way.

And FWIW, even if we allowed the MySQLism, it might not actually be any faster than running two queries:

http://www.mysqlperformanceblog.com/2007/08/28/to-sql_calc_found_rows-or-not-to-sql_calc_found_rows/

That blog post is pretty old, of course, so things may have changed, but benchmarking would be in order either way.
Comment 48 Galen Charlton 2013-08-12 18:58:55 UTC
As a final comment, for the moment, I think it is best if merely going to the patron home page does not do a search -- retrieving 20 records isn't a huge performance hit, of course, but the first 20 records are very unlikely to be useful.  It would be better to omit the search so that the search form can be rendered more quickly.

This should be distinguished (by checking for POST vs. GET, presumably) from the case where the staff user intentionally does a blank patron search as a way of bringing up a list of all patrons.
Comment 49 Galen Charlton 2013-08-12 19:10:41 UTC
I lied, one more comment: there is a minor but annoying change of behavior.

Prior to the patch, if you start from the patron home page and do a search that retrieves a single hit, it automatically redirects to the details page for that patron.  You can then use the back button to get back to the patrons home page.

After the patch, the back button is effectively broken -- you can go back, but it will then immediately re-run the search and redirect you back to the patron record.

It would be *really* nice if it didn't do that.
Comment 50 Jonathan Druart 2013-08-13 09:56:00 UTC
(In reply to Galen Charlton from comment #46)
> I've spent a bit of time with the current patch series, and I've *not* been
> able to reproduce the problem where going to members/member.pl induced
> DataTables to try loading the entire patron database.

Thanks for testing Galen!

> To avoid that, a LIMIT clause should *always* be added to the patron query,
> with sensible default values if iDisplayStart and/or iDisplayLength are not
> supplied.  0 and 20, respectively, I'd say.
> 
> We may also want to consider hardcoding an absolute cap (say 100?) on
> iDisplayLength.

Will be fixed, using 20 by default (like the DT default value).

(In reply to Galen Charlton from comment #48)
> As a final comment, for the moment, I think it is best if merely going to
> the patron home page does not do a search -- retrieving 20 records isn't a
> huge performance hit, of course, but the first 20 records are very unlikely
> to be useful.  It would be better to omit the search so that the search form
> can be rendered more quickly.
> 
> This should be distinguished (by checking for POST vs. GET, presumably) from
> the case where the staff user intentionally does a blank patron search as a
> way of bringing up a list of all patrons.

Will be fixed too, the table will be hidden by default.

(In reply to Galen Charlton from comment #49)
> I lied, one more comment: there is a minor but annoying change of behavior.
> 
> Prior to the patch, if you start from the patron home page and do a search
> that retrieves a single hit, it automatically redirects to the details page
> for that patron.  You can then use the back button to get back to the
> patrons home page.
> 
> After the patch, the back button is effectively broken -- you can go back,
> but it will then immediately re-run the search and redirect you back to the
> patron record.
> 
> It would be *really* nice if it didn't do that.

It was a feature :) The goal was to display the same results using the back button. The former request already lead to the disappearance of this feature (with the side-effect too).
Comment 51 Jonathan Druart 2013-08-13 09:56:32 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2013-08-13 09:56:42 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2013-09-09 15:56:32 UTC
Main patches already passed qa.
only latest patches need a signed off here, especially not to get data when loading the page.
Comment 54 I'm just a bot 2013-09-18 10:41:30 UTC
Applying: Bug 9811: Patron search improvement
Using index info to reconstruct a base tree...
M	C4/Members.pm
M	koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
M	members/member.pl
M	members/moremember.pl
Falling back to patching base and 3-way merge...
Auto-merging members/moremember.pl
Auto-merging members/member.pl
CONFLICT (content): Merge conflict in members/member.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging C4/Members.pm
Patch failed at 0001 Bug 9811: Patron search improvement
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 55 Jonathan Druart 2013-09-18 11:47:03 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2013-09-18 11:47:12 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2013-09-18 11:47:18 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2013-09-18 11:47:23 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2013-09-18 11:47:29 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2013-09-18 11:47:34 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2013-09-18 11:47:39 UTC Comment hidden (obsolete)
Comment 62 cedric.vita 2013-09-26 09:28:42 UTC
The patch works with the note circulation in the borrower record, the note appears. But in the note off the borrower record "Add a new message"(circulation.pl?borrowernumber=XXX), the note is not displayed.
Comment 63 Katrin Fischer 2013-09-26 09:31:09 UTC
I think that is ok - the messages notes system was added later and I think it did never show up in the table. This could go on a separate bug.
Comment 64 I'm just a bot 2013-09-29 06:17:29 UTC
Patch applied cleanly, go forth and signoff
Comment 65 cedric.vita 2013-10-17 09:20:55 UTC
Mail when the application sandbox

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 9811:
<h1>Something went wrong !</h1>Applying: Bug 9811: Patron search improvement
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Members.pm
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt
CONFLICT (content): Merge conflict in
koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt
Auto-merging members/member.pl
CONFLICT (content): Merge conflict in members/member.pl
Auto-merging members/moremember.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 9811: Patron search improvement
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 9811 - Patrons search improvements

21169 - Bug 9811: Patron search improvement
21170 - Bug 9811: Add unit tests for C4::Utils::DT::Members
21171 - Bug 9811: QA followup
21172 - Bug 9811 - multilines notes brakes JSON
21173 - Bug 9811: use count(primary_key) instead of count(*)
21174 - Bug 9811: A limit clause should be always added.
21175 - Bug 9811: Load the page without any data.

Apply? [(y)es, (n)o, (i)nteractive] Patch left in
/tmp/Bug-9811-Patron-search-improvement-zsuviY.patch .
Comment 66 Jonathan Druart 2013-10-22 10:14:23 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2013-10-22 10:17:32 UTC
Rebased and squashed patch.
A huge conflict (caused by bug 10565) appeared on rebasing so I preferred squashing the patches.
Comment 68 Jonathan Druart 2013-10-22 10:19:24 UTC
(In reply to Jonathan Druart from comment #66)
> Amended patch: Since bug 10565 has been pushed, these patches don't
> apply cleanly. I had to rewrite a part of the patron list feature.
> I removed the choice to add all resultant patrons from a search. I think
> this choice is useless with this patch: we are able to display the
> number of patrons we want and to select all of them.

Kyle, could you agree or disagree with that please?
Comment 69 I'm just a bot 2013-10-30 07:49:23 UTC
Patch applied cleanly, go forth and signoff
Comment 70 Mathieu Saby 2013-11-29 17:57:17 UTC
Hi Jonathan

3 remarks:

- there is a little conflict (I solved it on my VM)

- when I click on A to see all the patrons with name begining with A, I am automatically redirected to the card of the only patron whose name begins with A.
Is it a wanted behavior?

- when I try to sort the table according to names or cardnumber, the sorting is quite slow. It's surprizing, because I only have 10 patrons in my test base. So I fear the results will be worse with a real base. Maybe it's an issue of my computer, but do you think there is some part of code to optimize? 

- Is there a way to cancel a filter when it is too restrictive? or to cancel all filters?

Mathieu
Comment 71 Mathieu Saby 2013-11-29 17:57:31 UTC
4 remarks in fact...
Comment 72 Jonathan Druart 2013-12-03 12:09:04 UTC
(In reply to mathieu saby from comment #70)
> Hi Jonathan
> 
> 3 remarks:
> 
> - there is a little conflict (I solved it on my VM)

I will submit a rebased patch.

> - when I click on A to see all the patrons with name begining with A, I am
> automatically redirected to the card of the only patron whose name begins
> with A.
> Is it a wanted behavior?

When the table has only 1 result, the patron detail page is displayed.

> - when I try to sort the table according to names or cardnumber, the sorting
> is quite slow. It's surprizing, because I only have 10 patrons in my test
> base. So I fear the results will be worse with a real base. Maybe it's an
> issue of my computer, but do you think there is some part of code to
> optimize? 

With 3.7k patrons, I get a response in 600ms.

> - Is there a way to cancel a filter when it is too restrictive? or to cancel
> all filters?

I don't understand. The filters on the left can be canceled on choosing the first entry 'all/any'.
Comment 73 Jonathan Druart 2013-12-03 12:09:39 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2014-01-09 15:11:51 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2014-01-09 15:13:34 UTC
Rebased patch.
Fix conflicts with bug 10834 and add support for search on sort1 and sort2 fields.
Comment 76 Jonathan Druart 2014-01-10 10:26:07 UTC Comment hidden (obsolete)
Comment 77 jmbroust 2014-01-10 10:49:18 UTC
Tested on sandbox 16 biblibre.

We tried to test this patch. We have detected several problems :
- A clic on Cancel button makes appear a branch (médiathèque de prêt) whereas we didn't select this branch
- Synchronization and consistency problems with the two text search boxes (searchmember and searchmember_filter)
- Problems with Back button : old searches are saved and performed
- This patch looks like the advanced research : is it made to filter results after a search or to perform the same search as an advanced research ?
- We have detected encoding problems, example : Catégorie (fr-FR)

Thanks
Comment 78 Jonathan Druart 2014-01-10 11:01:29 UTC
(In reply to jmbroust from comment #77)
> Tested on sandbox 16 biblibre.
> 
> We tried to test this patch. We have detected several problems :
> - A clic on Cancel button makes appear a branch (médiathèque de prêt)
> whereas we didn't select this branch

Fixed by the last patch.

> - Synchronization and consistency problems with the two text search boxes
> (searchmember and searchmember_filter)

Could you detail please?

> - Problems with Back button : old searches are saved and performed

I don't know how to fix that :-/ I will see while the QA step.

> - This patch looks like the advanced research : is it made to filter results
> after a search or to perform the same search as an advanced research ?

Filters allow you to refine your search.

> - We have detected encoding problems, example : Catégorie (fr-FR)

Not introduced by this patch.
Comment 79 jmbroust 2014-01-10 11:16:43 UTC
(In reply to Jonathan Druart from comment #78)

> > - Synchronization and consistency problems with the two text search boxes
> > (searchmember and searchmember_filter)
> 
> Could you detail please?
Example 1 : we make a research "Maxime" with the top search box and then a research "Jean" with the filter search. When we clic on the back button, "Jean" didn't appear in the filter search field (it's "Maxime" instead).
Example 2 : When we make an advanced research, fields are automatically filled in the filter research. So the synchronisation doesn't work everytime.

Thanks.
Comment 80 Jonathan Druart 2014-01-10 11:36:10 UTC
(In reply to jmbroust from comment #79)
> (In reply to Jonathan Druart from comment #78)
> 
> > > - Synchronization and consistency problems with the two text search boxes
> > > (searchmember and searchmember_filter)
> > 
> > Could you detail please?
> Example 1 : we make a research "Maxime" with the top search box and then a
> research "Jean" with the filter search. When we clic on the back button,
> "Jean" didn't appear in the filter search field (it's "Maxime" instead).
> Example 2 : When we make an advanced research, fields are automatically
> filled in the filter research. So the synchronisation doesn't work everytime.

It is the behavior of the browser. In fact, when you use the filters, the page is not reloaded entirely, only the results table.
What you describe is a correct behavior.
Comment 81 Jonathan Druart 2014-01-10 11:38:08 UTC
(In reply to Jonathan Druart from comment #78)
> > - Problems with Back button : old searches are saved and performed
> 
> I don't know how to fix that :-/ I will see while the QA step.

I don't have a solution for that. A workaround would be to open the patron detail page into a new tab/window.

I switch back the status to needs signoff.
Comment 82 Petter Goksøyr Åsen 2014-02-06 09:45:37 UTC
> > - Problems with Back button : old searches are saved and performed
> 
> I don't know how to fix that :-/ I will see while the QA step.

> I don't have a solution for that. A workaround would be to open the patron  detail page into a new tab/window.

Note: I havent tried this patch, but I recently resolved a similar issue in another project.

Most browsers now use something called BFCache (back-forward cache) witch means that when you click back button, the page is not reloaded, and scripts not rerun, The page is still in browser memory, so it just revert to previous state.

To force any action when you are going back, you have to listen for the "pageshow" event. For example with jqUery:
$(window).bind("pageshow", function() {
  // clear input fields here!
});

You can read about BFcache logic here:
https://developer.mozilla.org/en-US/docs/Working_with_BFCache

Hope this helps!
Comment 83 Jonathan Druart 2014-03-11 10:34:28 UTC Comment hidden (obsolete)
Comment 84 Jonathan Druart 2014-03-11 10:34:39 UTC Comment hidden (obsolete)
Comment 85 jmbroust 2014-03-12 10:32:24 UTC
Tested on sandbox 15 in French : problem of diacritics. Example : adhérent becomes adhérent.
Comment 86 Olli-Antti Kivilahti 2014-03-12 11:37:00 UTC
Hi collected my bug findings here.
1. Monster circulation note breaks search functionality.
2. Members.t throws undefined errors.
http://pastebin.com/Es9MD48q
Comment 87 Jonathan Druart 2014-03-12 14:27:27 UTC
This is break(In reply to jmbroust from comment #85)
> Tested on sandbox 15 in French : problem of diacritics. Example : adhérent
> becomes adhérent.

Are you sure it is from this patch? Is it append on all pages?
Comment 88 Jonathan Druart 2014-03-12 14:29:35 UTC
(In reply to Olli-Antti Kivilahti from comment #86)
> Hi collected my bug findings here.
> 1. Monster circulation note breaks search functionality.
> 2. Members.t throws undefined errors.
> http://pastebin.com/Es9MD48q

This appends if the notes contain \s or \d, the generated JSON is not valid.
Even if I use the json filter (JSON.Espace) in the template, the problem exists.
I think it is not critical.
Comment 89 jmbroust 2014-03-12 14:57:35 UTC
(In reply to Jonathan Druart from comment #87)
> This is break(In reply to jmbroust from comment #85)
> > Tested on sandbox 15 in French : problem of diacritics. Example : adhérent
> > becomes adhérent.
> 
> Are you sure it is from this patch? Is it append on all pages?

It seems to be only in the member.pl pages. I am sure it is introduced by the patch because I had a look before applying this patch.
Comment 90 Jonathan Druart 2014-03-13 10:56:55 UTC
(In reply to jmbroust from comment #89)
> (In reply to Jonathan Druart from comment #87)
> > This is break(In reply to jmbroust from comment #85)
> > > Tested on sandbox 15 in French : problem of diacritics. Example : adhérent
> > > becomes adhérent.
> > 
> > Are you sure it is from this patch? Is it append on all pages?
> 
> It seems to be only in the member.pl pages. I am sure it is introduced by
> the patch because I had a look before applying this patch.

I don't reproduce.
Comment 91 Olli-Antti Kivilahti 2014-03-13 11:15:27 UTC Comment hidden (obsolete)
Comment 92 Jonathan Druart 2014-03-14 10:04:42 UTC Comment hidden (obsolete)
Comment 93 Olli-Antti Kivilahti 2014-03-14 10:08:48 UTC Comment hidden (obsolete)
Comment 94 Olli-Antti Kivilahti 2014-03-14 11:36:48 UTC Comment hidden (obsolete)
Comment 95 Olli-Antti Kivilahti 2014-03-14 11:41:54 UTC Comment hidden (obsolete)
Comment 96 Olli-Antti Kivilahti 2014-03-14 11:42:36 UTC
kick ass!
Comment 97 Jonathan Druart 2014-04-25 15:17:11 UTC Comment hidden (obsolete)
Comment 98 Jonathan Druart 2014-04-25 15:17:38 UTC Comment hidden (obsolete)
Comment 99 Jonathan Druart 2014-04-25 15:17:44 UTC Comment hidden (obsolete)
Comment 100 Jonathan Druart 2014-04-25 15:17:50 UTC Comment hidden (obsolete)
Comment 101 Jonathan Druart 2014-04-25 15:17:57 UTC Comment hidden (obsolete)
Comment 102 Jonathan Druart 2014-04-25 15:18:50 UTC
The last patch set only fixes the patches order.
Comment 103 Katrin Fischer 2014-04-27 21:31:49 UTC
Patches still apply, QA script noted something small:

 FAIL	C4/Utils/DataTables/Members.pm
   OK	  pod
   FAIL	  forbidden patterns
		forbidden pattern: Koha is now under the GPLv3 license (line 204)
   OK	  valid
   OK	  critic
Comment 104 Jonathan Druart 2014-04-28 09:06:54 UTC Comment hidden (obsolete)
Comment 105 Katrin Fischer 2014-06-09 11:11:47 UTC
Giving this some quick tests for today, hopefully more soon:

Tested with problems:

1) Sending an empty search from the patrons module top search results in a perl error: Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2059.

2) There is a sort option on the first column with the checkboxes for the patron list feature, but if you sort with checkboxes checked, all checkboxes are unchecked.

3) I had it happen that my existing patron lists didn't show up in the pull down - after switching to master and back to the branch with the patches applied, I can't reproduce that. So for now this is mostly a note to keep an eye out for that.

Suggestions:

3) It would be nice to be able to sort on the 'fines' column as well.

Tested OK:

1) Searching for % still returns all borrowers.

2) Alphabetic browse still works correctly.

Does someone know a good trick to add a few thousand borrowers easily? :)
Comment 106 Katrin Fischer 2014-06-09 11:21:30 UTC
Found another tiny issue: missing space between category and type: Staff(S)
Comment 107 Katrin Fischer 2014-06-09 11:24:03 UTC
Another small one: Some time ago the email address was added to the information displayed, with the patch we have a small regression there as it no longer displays.
Comment 108 Jonathan Druart 2014-06-09 15:40:58 UTC Comment hidden (obsolete)
Comment 109 Jonathan Druart 2014-06-09 15:41:06 UTC Comment hidden (obsolete)
Comment 110 Jonathan Druart 2014-06-09 15:41:16 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2014-06-09 15:41:23 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2014-06-09 15:41:32 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2014-06-09 15:41:38 UTC Comment hidden (obsolete)
Comment 114 Jonathan Druart 2014-06-09 15:41:45 UTC Comment hidden (obsolete)
Comment 115 Jonathan Druart 2014-06-09 15:41:51 UTC Comment hidden (obsolete)
Comment 116 Jonathan Druart 2014-06-09 15:41:57 UTC Comment hidden (obsolete)
Comment 117 Jonathan Druart 2014-06-09 15:45:58 UTC
10318:(In reply to Katrin Fischer from comment #105)
> Giving this some quick tests for today, hopefully more soon:

Thanks!

> 1) Sending an empty search from the patrons module top search results in a
> perl error: Can't use an undefined value as an ARRAY reference at
> /usr/lib/perl5/DBI.pm line 2059.

Fixed.

> 2) There is a sort option on the first column with the checkboxes for the
> patron list feature, but if you sort with checkboxes checked, all checkboxes
> are unchecked.

Fixed.

> 3) I had it happen that my existing patron lists didn't show up in the pull
> down - after switching to master and back to the branch with the patches
> applied, I can't reproduce that. So for now this is mostly a note to keep an
> eye out for that.

Hum, weird... :)

> 3) It would be nice to be able to sort on the 'fines' column as well.

No more improvement is envisage in this report. But could be done later.

(In reply to Katrin Fischer from comment #106)
> Found another tiny issue: missing space between category and type: Staff(S)

Fixed.

(In reply to Katrin Fischer from comment #107)
> Another small one: Some time ago the email address was added to the
> information displayed, with the patch we have a small regression there as it
> no longer displays.

Yep, good catch, introduced by bug 10318. Reintroduced in one of the last patches.

Back to Signed Off.
Comment 118 Jonathan Druart 2014-06-09 16:10:33 UTC
Created attachment 28747 [details]
Build patrons

(In reply to Katrin Fischer from comment #105)
> Does someone know a good trick to add a few thousand borrowers easily? :)

Try this script (you have to edit the $number_of_patrons_to_create variable value).

/!\ Don't use this script if you are in love with you DB!
Comment 119 Katrin Fischer 2014-06-10 09:29:26 UTC
Hi Jonathan, thx for the follow-ups! I will try to test your script and dump my database before doing so :)
Comment 120 Jonathan Druart 2014-06-11 13:06:21 UTC Comment hidden (obsolete)
Comment 121 Jonathan Druart 2014-06-11 13:21:04 UTC Comment hidden (obsolete)
Comment 122 Katrin Fischer 2014-06-11 13:21:53 UTC
1) Top search
- (will test later)

2) New filters

- OK Terminology
- (will test more later)

3) Result list

- OK Cardnumber
- OK Name, address and email
- OK Fines, checkouts and overdues
- OK Adding patrons to patron lists
- OK Select all/unselect all
- OK Sorting on the various columns
- OK Paging

4) Performance
My database currently has about 10.000 patrons, response times so far
seem ok.
Comment 123 Katrin Fischer 2014-06-19 13:24:49 UTC
Created attachment 29019 [details] [review]
[PASSED QA] Bug 9811: Patron search improvement

This patch add DataTables using server-side processing for the patrons
search.

It adds:
- 1 module C4/Utils/DataTables/Members.pm
- 2 services svc/members/search and svc/members/add_to_list
- 1 template members/tables/members_results.tt
- 1 new practice which is to add template for DataTables in a
  subdirectory named 'tables'.

Impacted scripts: members/members-home.pl and members/members.pl

To go further: We can imagine that all patrons searches use the same
service with no big changes: 1 little template creates a JSON file and
to implement DataTables on the template page, that's all.

Amended patch: Since bug 10565 has been pushed, these patches don't
apply cleanly. I had to rewrite a part of the patron list feature.
I removed the choice to add all resultant patrons from a search. I think
this choice is useless with this patch: we are able to display the
number of patrons we want and to select all of them.

Test plan:
- Check that there is no regression on searching patrons.
- Try filters on the left of the screen.
- Try to sort each column.
- Try the "Browse by last name" links.
- Check that the "Clear" button clears yours filters.
- Try with IndependantBranches ON and OFF.
- Verify this feature does not break the patron list feature (cf bug
  10565).

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script, couldn't find any regressions
or problems. Some notes left on the bug.

Bug 9811: Add unit tests for C4::Utils::DT::Members

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Bug 9811: QA followup

- removes 2 tabs
- removes mysqlisms
- add sort on borrowernotes
- fix wrong capitalization
- cat => Category

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Thx for fixing these!

Bug 9811 - multilines notes brakes JSON

In new patron search feature, the search results are fetched using Ajax and returned in JSON format.
The JSON is created by TT using koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/members_results.tt.
One of the fields is the borrower notes. When this notes contains several lines, the JSON is broken.

This patch uses TT fileters to consert in notes linefeeds into HTML line break (html_line_break) and then remove linefeeds (collapse).

Test plan :
- perform a member search that does not return a borrower with a circ note
- edit one of the borrowers returned by this search
- enter serveral lines of text in "Circulation note" and save
- reperform the member search
=> circ note is well displayed on several lines

Bug 9811: use count(primary_key) instead of count(*)

Bug 9811: A limit clause should be always added.

By default, we want to retrieve 20 first results.

Bug 9811: Load the page without any data.

Displaying the first 20 patrons is not useful. With this patch, the
table is hidden and no record is retrieved by default.
On the same way, the existing side effect on redirect disappears.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
-------------
-TEST REPORT-
-------------
For the filter: Tested all the search fields, branches, search type.
Found a bug with "date of birth", followup provided.
Tested display limits and verified that AJAX-queries are
  efficient (using LIMIT clause) to not stress DB needlessly.
Tested adding Patrons to a list.
A good feature, which seems to work quite well.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Adding my test plan to the last patch of this bug.
Comment 124 Katrin Fischer 2014-06-19 13:24:54 UTC
Created attachment 29020 [details] [review]
[PASSED QA] Bug 9811: FIX branches loop was not correctly set

The branches loop was not set with the values.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 125 Katrin Fischer 2014-06-19 13:25:01 UTC
Created attachment 29021 [details] [review]
[PASSED QA] Bug 9811: Fixing JSON validation error for non-javascript string literal characters

Issue:
When a patron has backslashes in his circulation notes, the JSON transportation
layer cannot send the message because it will fail JSON validation when trying to
validate backslashed non-literal characters (\s \d ...).
This causes the whole search to fail when even one Borrower has a non-literal
backslashed character in his notes.

Solution:
The Borrowers' circulation notes are filtered through a regexp which doubles all
backslashes to prepare them for the JSON validation routine.

Result:
Backslashes pass through unchanged.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 126 Katrin Fischer 2014-06-19 13:25:07 UTC
Created attachment 29022 [details] [review]
[PASSED QA] Bug 9811: FIX encoding issue on patron categories

The patron categories should use the html_entity filter (on the current
master).

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
This fix solves the issue of double encoding some utf8-characters by html_encoding the
patron categories, which are not marked as utf8.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 127 Katrin Fischer 2014-06-19 13:25:13 UTC
Created attachment 29023 [details] [review]
[PASSED QA] Bug 9811 - datetime does not respect DateFormat-system prefrence

To test:
Try searching by the date of birth using the left filter column. Good luck.

After applying this patch:
You can search using your preferred DateFormat.

Merci bocu

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 128 Katrin Fischer 2014-06-19 13:25:20 UTC
Created attachment 29024 [details] [review]
[PASSED QA] Bug 9811: QA follow-up: Fix license version

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 129 Katrin Fischer 2014-06-19 13:25:26 UTC
Created attachment 29025 [details] [review]
[PASSED QA] Bug 9811: Display email address in patron result list

This patch restores the enhancement introduced by Bug 10318.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 130 Katrin Fischer 2014-06-19 13:25:33 UTC
Created attachment 29026 [details] [review]
[PASSED QA] Bug 9811: Remove useless orderby management

I don't know what is this stuff. I didn't find a call to member.pl with
an orderby parameter.
I am not sure this is in used.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 131 Katrin Fischer 2014-06-19 13:25:39 UTC
Created attachment 29027 [details] [review]
[PASSED QA] Bug 9811: Various fixes

This patch fixes the issues raised in bug 9811 comment 105, 106 and 107:
1/ Add missing space between category and type.
2/ Fix error on searching an empty string (caused by a previous rebase).
3/ Remove sort option on checkbox/borrowernumber column.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 132 Katrin Fischer 2014-06-19 13:25:44 UTC
Created attachment 29028 [details] [review]
[PASSED QA] Bug 9811: Cardnumber could be alphanumeric

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 133 Katrin Fischer 2014-06-19 13:25:49 UTC
Created attachment 29029 [details] [review]
[PASSED QA] Bug 9811: Fix wording branch vs library

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

1) Top search
- OK no regressions found

2) New filters

- OK Terminology
- OK Search fields, search types, category, library
- OK Clear button

3) Result list

- OK Cardnumber
- OK Name, address and email
- OK Fines, checkouts and overdues
- OK Adding patrons to patron lists
- OK Select all/unselect all
- OK Sorting on the various columns
- OK Paging
- OK PatronsPerPage system preference

4) Performance
Tested with a database containing 40.000 patrons.
Search results were displayed with a little delay,
overall performance seemed pretty good.
Comment 134 Kyle M Hall 2014-07-02 14:18:43 UTC
Created attachment 29442 [details] [review]
Bug 9811 [QA Followup] - Enable bProcessing

It's a bit disconcerting to not have any feedback while paging
through the patrons. Enabling bProcessing helps resolve that.
Comment 135 Tomás Cohen Arazi 2014-07-02 18:14:32 UTC
Patches pushed to master.

Thanks Jonathan and everyone involved!
Comment 136 Dobrica Pavlinusic 2014-07-17 10:58:18 UTC
It seems that this patch changes behavior when we search for first name last name combination.

In old code, we where able to enter full patron name and get just that patron, while with this change we get all patrons which have same first OR last name.

This is quite annoying because for common names we get 300+ results instead of just one.

It seems that we should use AND operator for all "standard" fields in C4::Utils::DataTables::Members, but I might be wrong. Comments how to address this are very welcomed.
Comment 137 Jonathan Druart 2014-07-17 11:09:26 UTC
(In reply to Dobrica Pavlinusic from comment #136)
> It seems that we should use AND operator for all "standard" fields in
> C4::Utils::DataTables::Members, but I might be wrong.

Could you detail, I don't understand.

If you search "john doe", which query are you expected?
Comment 138 Jonathan Druart 2014-07-17 11:19:56 UTC
(In reply to Jonathan Druart from comment #137)
> (In reply to Dobrica Pavlinusic from comment #136)
> > It seems that we should use AND operator for all "standard" fields in
> > C4::Utils::DataTables::Members, but I might be wrong.
> 
> Could you detail, I don't understand.
> 
> If you search "john doe", which query are you expected?

Ok, understood looking at the previous code. It should work as it. It's a bug.

If you open a bug report, I can submit a patch.
Comment 139 Jonathan Druart 2014-07-17 13:21:48 UTC
(In reply to Jonathan Druart from comment #138)
> If you open a bug report, I can submit a patch.

I opened bug 12595.