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

(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.

> 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

There is a problem with the patch, some whitespace are rare, they look like a A :
    { 'mDataProp': 'dt_name'Â }
See patch in browser.

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]

ok for me the patch applies correctly

Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>

We need unit tests for this in a follow up - can then be switched back to signed off of course.

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()";

(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.

The tests are all passing now - I think my qa-tools were not up to date at that point :)

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.

(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.

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.

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.

(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) ?

Sounds like a good idea :)

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.

(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

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.

(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.

(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.)

Oh and Galen just reminded me Datatables supports lazy loading, so it shouldnt be too hard to add that.

(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.

(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

(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.

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.

With 35k patrons in the DB, the table is displayed in 2sec (on my dev instance).

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.

(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.

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.

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.

(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).

Main patches already passed qa.
only latest patches need a signed off here, especially not to get data when loading the page.

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".

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.

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.

Patch applied cleanly, go forth and signoff

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 .

Rebased and squashed patch.
A huge conflict (caused by bug 10565) appeared on rebasing so I preferred squashing the patches.

(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?

Patch applied cleanly, go forth and signoff

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

4 remarks in fact...

(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'.

Rebased patch.
Fix conflicts with bug 10834 and add support for search on sort1 and sort2 fields.

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

(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.

(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.

(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.

(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.

> > - 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!

Tested on sandbox 15 in French : problem of diacritics. Example : adhérent becomes adhérent.

Hi collected my bug findings here.
1. Monster circulation note breaks search functionality.
2. Members.t throws undefined errors.
http://pastebin.com/Es9MD48q

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?

(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.

(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.

(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.

kick ass!

The last patch set only fixes the patches order.

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

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? :)

Found another tiny issue: missing space between category and type: Staff(S)

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.

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.

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!

Hi Jonathan, thx for the follow-ups! I will try to test your script and dump my database before doing so :)

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.

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.

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 29028 [details] [review]
[PASSED QA] Bug 9811: Cardnumber could be alphanumeric

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

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.

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.

Patches pushed to master.

Thanks Jonathan and everyone involved!

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.

(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?

(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.

(In reply to Jonathan Druart from comment #138)
> If you open a bug report, I can submit a patch.

I opened bug 12595.