Bug 13891

Summary: Make all patron searches use DataTables server-side processing
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: barton, gmcharlt, gwilliams, indradg, kyle.m.hall, kyle, robin, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13021
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14739
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9811, 13879    
Bug Blocks: 12633, 14123    
Attachments: Bug 13891: DataTables server-side processing - budget users
Bug 13891: DataTables server-side processing - Serial recipients
Bug 13891: DataTables server-side processing - patron cards
Bug 13891: DataTables server-side processing - Fix regressions
No links on patron's names (behaviour without patches)
[Signed-off] Bug 13891: DataTables server-side processing - budget users
[Signed-off] Bug 13891: DataTables server-side processing - Serial recipients
[Signed-off] Bug 13891: DataTables server-side processing - patron cards
[Signed-off] Bug 13891: DataTables server-side processing - Fix regressions
[PASSED QA] Bug 13891: DataTables server-side processing - budget users
[PASSED QA] Bug 13891: DataTables server-side processing - Serial recipients
[PASSED QA] Bug 13891: DataTables server-side processing - patron cards
[PASSED QA] Bug 13891: DataTables server-side processing - Fix regressions

Description Jonathan Druart 2015-03-23 10:00:33 UTC
Bug 9811 introduces a new search way for patrons. Some code have been refactored and the server-side option of DataTables is now used.

Bug 12648 (basket users) and bug 13021 (guarantors) uses the same way to search patrons.

But in 3 other places, the former C4::Members::Search subroutine is still used (and in some cases, a manual pagination...):
- budget users and owner
- serial recipients
- patron cards


The 2 expected goals are 1/ Use the same centralized way to search for patrons and 2/ to get rid of the last calls to the C4::Members::Search (and so SQLHelper).
Comment 1 Jonathan Druart 2015-03-23 11:06:47 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-03-23 11:06:52 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-03-23 11:06:56 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-03-23 11:07:00 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-03-23 11:10:05 UTC
6 files deleted, 400 lines of code removed but new features added :)
Please test!
Comment 6 Marc Véron 2015-03-24 10:42:05 UTC
Patch does not apply:

Applying: Bug 13891: DataTables server-side processing - budget users
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudget_user_search.tt
Removing admin/aqbudget_user_search.pl
Applying: Bug 13891: DataTables server-side processing - Serial recipients
Applying: Bug 13891: DataTables server-side processing - patron cards
Applying: Bug 13891: DataTables server-side processing - Fix regressions
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 13891: DataTables server-side processing - Fix regressions
Comment 7 Jonathan Druart 2015-03-24 11:04:20 UTC
(In reply to Marc Véron from comment #6)

Sorry Marc, I forgot to add the dependency on bug 13879.
Comment 8 Marc Véron 2015-03-24 13:41:26 UTC
I have the following problem (tested with adding owner and user to fund and with routing list, but I thing it will appear at all places):

1) Do a search in the pop-up window
2) The names in the result table are linked to patron's detail page. If you click on a name, patron's details appear inside this pop-up window, no way back.

I would expect the names to have no detail link at all OR the link on the names to behave like the 'Select' link.
Comment 9 Jonathan Druart 2015-03-24 14:10:07 UTC
(In reply to Marc Véron from comment #8)
> I have the following problem (tested with adding owner and user to fund and
> with routing list, but I thing it will appear at all places):
> 
> 1) Do a search in the pop-up window
> 2) The names in the result table are linked to patron's detail page. If you
> click on a name, patron's details appear inside this pop-up window, no way
> back.
> 
> I would expect the names to have no detail link at all OR the link on the
> names to behave like the 'Select' link.

I think the links already existed before this patch set. I am not sure removing them is a good idea.
What did you mean with "behave like the 'Select' link"? Open in a new window?
Comment 10 Marc Véron 2015-03-24 14:33:02 UTC
Created attachment 37180 [details]
No links on patron's names (behaviour without patches)

Hi Jonathan,

Without patches applied the names are not linked.

With "Behave like the select link" I mean that if a link is on the name, it should select this patron for the fund (or whatever the pop-up was opened for).

But I think it is better to have no links at all on the names, as it was without patch.

See screenshot in attachment.
Comment 11 Jonathan Druart 2015-03-24 14:44:41 UTC
(In reply to Marc Véron from comment #10)
> Created attachment 37180 [details]
> No links on patron's names (behaviour without patches)
> 
> Hi Jonathan,
> 
> Without patches applied the names are not linked.

On the users and owner of a budget, but it is on the order users search :)
So we need to have the same behavior everywhere.

> With "Behave like the select link" I mean that if a link is on the name, it
> should select this patron for the fund (or whatever the pop-up was opened
> for).
> 
> But I think it is better to have no links at all on the names, as it was
> without patch.

I don't have any objection to remove the link but I would prefer to wait for the QA step before removing it.

If it's your only concern, you can add it as a tester note and let the QAer and RM take the decision.
I will submit a followup if they agree with you.
Note that it is possible to have a link in a page and not in another one, but it would be a weird behavior.
Comment 12 Marc Véron 2015-03-24 15:46:33 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-03-24 15:48:20 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2015-03-24 15:57:12 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2015-03-24 15:59:02 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-03-27 16:58:37 UTC
QA Note, it appears that serials/add_user_search takes into account the search text field when browsing by last name. Original version doesn't have name browse. Could be considered a bug or a feature depending on your point of view.

This behavior does no occur on the "patron cards" patron search.
Comment 17 Kyle M Hall 2015-03-27 16:59:44 UTC
> This behavior does no occur on the "patron cards" patron search.
correct, it *does* occur.
Comment 18 Kyle M Hall 2015-03-27 17:01:11 UTC
Created attachment 37334 [details] [review]
[PASSED QA] Bug 13891: DataTables server-side processing - budget users

This first patch remove the previous way to search for users to link to
budgets.

Test plan:
1/ Edit or create a fund
2/ Edit the owner of this fund a select a patron
3/ Add 1+ users to the user lists of this fund

Tested together with other patches.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2015-03-27 17:01:21 UTC
Created attachment 37335 [details] [review]
[PASSED QA] Bug 13891: DataTables server-side processing - Serial recipients

This second patch removes the previous way to search for serial recipients.

Test plan:
1/ Edit the routing list for a subscription (serials/routing.pl?subscriptionid=X)
2/ Add 1+ recipients
3/ Confirm there is no regression on the add/delete/search actions

QA note: This search does not use a clean way to interact with the
window opener. Indeed the opener is reloaded to display the new item.
This patch does not change this behavior, but note the trick in the
template (common/patron_search.tt) to wait for the opener in order not
to get a JS error.
This is also used by the next patch (patron card).

Tested together with other patches.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2015-03-27 17:01:23 UTC
Created attachment 37336 [details] [review]
[PASSED QA] Bug 13891: DataTables server-side processing - patron cards

This third patch removes the previous way to search for patron cards users.

Test plan:
1/ Add items to a patron card batch (patroncards/edit-batch.pl?op=edit&element_id=X)
2/ Add 1+ patrons to the batch
3/ Confirm there is no regression

Tested together with other patches.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2015-03-27 17:01:26 UTC
Created attachment 37337 [details] [review]
[PASSED QA] Bug 13891: DataTables server-side processing - Fix regressions

This patch fixes the regressions introduced by the previous patches.
If you have tested all in once, you didn't see them.

It introduces library, category and "first letter" filters.

Test plan:
1/ On all pages impacted by previous patches + new order empty (link patron to
an order) + guarantor search
2/ Add / Select patron to the list
3/ Use the filters
4/ Confirm there is no regression

Tested together with other patches.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Tomás Cohen Arazi 2015-04-13 14:21:35 UTC
Patches pushed to master.

Good job Jonathan!
Comment 23 Barton Chittenden 2015-11-29 21:50:59 UTC
Is it possible to port this back to 3.18?
Comment 24 Jonathan Druart 2015-12-01 09:47:42 UTC
(In reply to Barton Chittenden from comment #23)
> Is it possible to port this back to 3.18?

I don't think it's a good idea.
The patches don't apply and the dependency tree is quite important (most of them are not in 3.18 yet).
Comment 25 Jonathan Druart 2016-02-15 17:30:47 UTC
*** Bug 6513 has been marked as a duplicate of this bug. ***