Bug 12648 refactores some code for the patron search (link patrons to a basket and an order, in the acquisition module). This code can be reuse for the guarantor search.
This will remove 1 call to C4::Members::Search (see bug 12633).
Created attachment 31970 [details] [review] Bug 13021: guarantor search - prepare existing code to be reused The add_user_search tt file will be reuse in next commits, this commit just moves it in a "common" directory.
Created attachment 31971 [details] [review] Bug 13021: guarantor search - main patch This patch is the main patch. The "common" template is improved to allow different type of picking: "add" or "select". The first one appends a patron to a list, the second one selects the patron and close the result search window. The members/guarantor_search.pl has completly changed but is quite the same file as acqui/add_user_search.pl. Both should exist: they don't belong to the same module (acqui vs members), the picking type is different (add vs select) and the columns are not the same. The changes in the common template are very powerful, it's now possible to list the column we want! This will be very useful for further reusability. Before this patch, all patrons mathing the pattern were return. Now only the first 20 are (depends on the DataTables selected value). For QA: This patch introduces a new template plugin "To", for now it permits to convert a perl structure to json. In the idea, it could permit to convert foo to bar too. Test plan: 1/ Verify there is no regression in the guarantor search. When the selection has been done, all data from the guarantor should fill the form in the "main address" section. Note that the request is done when the search input in not empty and the user stop to write for 1 sec. 2/ Verify there is no regression on the 2 other pages where this patron search is used: link a patron to an order and to a basket (in the acquisition module).
Created attachment 31972 [details] [review] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely.
Created attachment 32231 [details] [review] Bug 13021: guarantor search - prepare existing code to be reused The add_user_search tt file will be reuse in next commits, this commit just moves it in a "common" directory.
Created attachment 32232 [details] [review] Bug 13021: guarantor search - main patch This patch is the main patch. The "common" template is improved to allow different type of picking: "add" or "select". The first one appends a patron to a list, the second one selects the patron and close the result search window. The members/guarantor_search.pl has completly changed but is quite the same file as acqui/add_user_search.pl. Both should exist: they don't belong to the same module (acqui vs members), the picking type is different (add vs select) and the columns are not the same. The changes in the common template are very powerful, it's now possible to list the column we want! This will be very useful for further reusability. Before this patch, all patrons mathing the pattern were return. Now only the first 20 are (depends on the DataTables selected value). For QA: This patch introduces a new template plugin "To", for now it permits to convert a perl structure to json. In the idea, it could permit to convert foo to bar too. Test plan: 1/ Verify there is no regression in the guarantor search. When the selection has been done, all data from the guarantor should fill the form in the "main address" section. Note that the request is done when the search input in not empty and the user stop to write for 1 sec. 2/ Verify there is no regression on the 2 other pages where this patron search is used: link a patron to an order and to a basket (in the acquisition module).
Created attachment 32233 [details] [review] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely.
I can't apply your patches, and get this Git error: fatal: sha1 information is lacking or useless (acqui/add_user_search.pl).
(In reply to Frédéric Demians from comment #8) > I can't apply your patches, and get this Git error: > > fatal: sha1 information is lacking or useless (acqui/add_user_search.pl). This patch depends on bug 12648, try to apply it before this one.
Created attachment 35296 [details] [review] Bug 13021: guarantor search - main patch This patch is the main patch. The "common" template is improved to allow different type of picking: "add" or "select". The first one appends a patron to a list, the second one selects the patron and close the result search window. The members/guarantor_search.pl has completly changed but is quite the same file as acqui/add_user_search.pl. Both should exist: they don't belong to the same module (acqui vs members), the picking type is different (add vs select) and the columns are not the same. The changes in the common template are very powerful, it's now possible to list the column we want! This will be very useful for further reusability. Before this patch, all patrons mathing the pattern were return. Now only the first 20 are (depends on the DataTables selected value). For QA: This patch introduces a new template plugin "To", for now it permits to convert a perl structure to json. In the idea, it could permit to convert foo to bar too. Test plan: 1/ Verify there is no regression in the guarantor search. When the selection has been done, all data from the guarantor should fill the form in the "main address" section. Note that the request is done when the search input in not empty and the user stop to write for 1 sec. 2/ Verify there is no regression on the 2 other pages where this patron search is used: link a patron to an order and to a basket (in the acquisition module).
Created attachment 35297 [details] [review] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely.
Created attachment 35321 [details] [review] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely. Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com>
I think everything looks okay. Signing off.
Created attachment 35322 [details] [review] Bug 13021: guarantor search - prepare existing code to be reused The add_user_search tt file will be reuse in next commits, this commit just moves it in a "common" directory. Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com>
Created attachment 35323 [details] [review] Bug 13021: guarantor search - main patch This patch is the main patch. The "common" template is improved to allow different type of picking: "add" or "select". The first one appends a patron to a list, the second one selects the patron and close the result search window. The members/guarantor_search.pl has completly changed but is quite the same file as acqui/add_user_search.pl. Both should exist: they don't belong to the same module (acqui vs members), the picking type is different (add vs select) and the columns are not the same. The changes in the common template are very powerful, it's now possible to list the column we want! This will be very useful for further reusability. Before this patch, all patrons mathing the pattern were return. Now only the first 20 are (depends on the DataTables selected value). For QA: This patch introduces a new template plugin "To", for now it permits to convert a perl structure to json. In the idea, it could permit to convert foo to bar too. Test plan: 1/ Verify there is no regression in the guarantor search. When the selection has been done, all data from the guarantor should fill the form in the "main address" section. Note that the request is done when the search input in not empty and the user stop to write for 1 sec. 2/ Verify there is no regression on the 2 other pages where this patron search is used: link a patron to an order and to a basket (in the acquisition module). Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com>
Created attachment 35324 [details] [review] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely. Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com>
(In reply to the.invinnysible.one from comment #13) > I think everything looks okay. Signing off. Thanks Morag! I added you signed off to the 3 patches.
The patron name column appears to be missing from the search results table!
Created attachment 35371 [details] [review] Bug 13021: Reintroduce the name column for guarantor search Previous patch removed the name column, it's a regression. This patch reintroduces it.
(In reply to Kyle M Hall from comment #18) > The patron name column appears to be missing from the search results table! Thanks for catching that! The last patch reintroduced the name column.
I'm afraid I must report that with the last followup, the search never returns any results! If I remove the last patch I once again get results, but of course the name column is missing without it.
Created attachment 35627 [details] [review] Bug 13021: Fix conflict with bug 8096 Bug 8096 added new breaklines, the corrupt the json string. This patch removes them.
Created attachment 35628 [details] [review] [PASSED QA] Bug 13021: guarantor search - prepare existing code to be reused The add_user_search tt file will be reuse in next commits, this commit just moves it in a "common" directory. Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35629 [details] [review] [PASSED QA] Bug 13021: guarantor search - main patch This patch is the main patch. The "common" template is improved to allow different type of picking: "add" or "select". The first one appends a patron to a list, the second one selects the patron and close the result search window. The members/guarantor_search.pl has completly changed but is quite the same file as acqui/add_user_search.pl. Both should exist: they don't belong to the same module (acqui vs members), the picking type is different (add vs select) and the columns are not the same. The changes in the common template are very powerful, it's now possible to list the column we want! This will be very useful for further reusability. Before this patch, all patrons mathing the pattern were return. Now only the first 20 are (depends on the DataTables selected value). For QA: This patch introduces a new template plugin "To", for now it permits to convert a perl structure to json. In the idea, it could permit to convert foo to bar too. Test plan: 1/ Verify there is no regression in the guarantor search. When the selection has been done, all data from the guarantor should fill the form in the "main address" section. Note that the request is done when the search input in not empty and the user stop to write for 1 sec. 2/ Verify there is no regression on the 2 other pages where this patron search is used: link a patron to an order and to a basket (in the acquisition module). Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35630 [details] [review] [PASSED QA] Bug 13021: guarantor search - guarantor_search.tt is useless The guarantor_search.tt is now useless, no script uses it. It can be removed safely. Signed-off-by: Morag Hills <the.invinnysible.one@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35631 [details] [review] [PASSED QA] Bug 13021: Reintroduce the name column for guarantor search Previous patch removed the name column, it's a regression. This patch reintroduces it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35632 [details] [review] [PASSED QA] Bug 13021: Fix conflict with bug 8096 Bug 8096 added new breaklines, the corrupt the json string. This patch removes them. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Jonathan, is it possible that the latest patch doesn't belong here?
(In reply to Tomás Cohen Arazi from comment #28) > Jonathan, is it possible that the latest patch doesn't belong here? Yep, same as bug 12648 comment 76: Created attachment 36810 [details] [review] [review] Bug 12648: Fix conflict with bug 8096
Patches pushed to master. Good job Jonathan!
Regression found, will be fixed soon on bug 13879.