|
Description
Kyle M Hall (khall)
2014-11-03 15:20:41 UTC
Created attachment 33194 [details] [review] Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes The browse by last name letters on the patron search for the patron card creator doesn't work quite right. If extended patron attributes are disabled, it works fine, but if they are enabled, they are searched even when using the browse last name. Thus, if a searchable attribute has a "D" in it, and one clicks the "D" link for the last name browser, that patron will show even if he or she has no "D" in his or her hame! Test Plan: 1) Enable extended patron attributes 2) Add a new searchable patron attribute 3) Create a new patron with the last name "Ace" 4) Add the value "D" to the attribute for this patron 5) Browse to the patron card maker, start a new patron batch 6) Click "Add item(s)" to bring up the patron search 7) Click the letter "D" in the patron search box 8) Note that "Ace" shows in the results list 9) Apply this patch 10) Repeat step 7 11) Note that "Ace" no longer shows in the results list 12) Perform a regular search by putting the letter "D" in the "Name:" field, and hit the "Search" button 13) Note this time the results *do* have Ace in them Created attachment 33433 [details] [review] Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes The browse by last name letters on the patron search for the patron card creator doesn't work quite right. If extended patron attributes are disabled, it works fine, but if they are enabled, they are searched even when using the browse last name. Thus, if a searchable attribute has a "D" in it, and one clicks the "D" link for the last name browser, that patron will show even if he or she has no "D" in his or her hame! Test Plan: 1) Enable extended patron attributes 2) Add a new searchable patron attribute 3) Create a new patron with the last name "Ace" 4) Add the value "D" to the attribute for this patron 5) Browse to the patron card maker, start a new patron batch 6) Click "Add item(s)" to bring up the patron search 7) Click the letter "D" in the patron search box 8) Note that "Ace" shows in the results list 9) Apply this patch 10) Repeat step 7 11) Note that "Ace" no longer shows in the results list 12) Perform a regular search by putting the letter "D" in the "Name:" field, and hit the "Search" button 13) Note this time the results *do* have Ace in them Signed-off-by: Frederic Demians <f.demians@tamil.fr> Well described for a tricky bug. Reproducible. Fixed with this patch. Created attachment 33451 [details] [review] [PASSED QA] Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes The browse by last name letters on the patron search for the patron card creator doesn't work quite right. If extended patron attributes are disabled, it works fine, but if they are enabled, they are searched even when using the browse last name. Thus, if a searchable attribute has a "D" in it, and one clicks the "D" link for the last name browser, that patron will show even if he or she has no "D" in his or her hame! Test Plan: 1) Enable extended patron attributes 2) Add a new searchable patron attribute 3) Create a new patron with the last name "Ace" 4) Add the value "D" to the attribute for this patron 5) Browse to the patron card maker, start a new patron batch 6) Click "Add item(s)" to bring up the patron search 7) Click the letter "D" in the patron search box 8) Note that "Ace" shows in the results list 9) Apply this patch 10) Repeat step 7 11) Note that "Ace" no longer shows in the results list 12) Perform a regular search by putting the letter "D" in the "Name:" field, and hit the "Search" button 13) Note this time the results *do* have Ace in them Signed-off-by: Frederic Demians <f.demians@tamil.fr> Well described for a tricky bug. Reproducible. Fixed with this patch. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This works as described, no problems or regressions found. As this changes the search routinges in Member.pm, I'd feel better with a regression test here. Regression test++ Created attachment 34171 [details] [review] Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes The browse by last name letters on the patron search for the patron card creator doesn't work quite right. If extended patron attributes are disabled, it works fine, but if they are enabled, they are searched even when using the browse last name. Thus, if a searchable attribute has a "D" in it, and one clicks the "D" link for the last name browser, that patron will show even if he or she has no "D" in his or her hame! Test Plan: 1) Enable extended patron attributes 2) Add a new searchable patron attribute 3) Create a new patron with the last name "Ace" 4) Add the value "D" to the attribute for this patron 5) Browse to the patron card maker, start a new patron batch 6) Click "Add item(s)" to bring up the patron search 7) Click the letter "D" in the patron search box 8) Note that "Ace" shows in the results list 9) Apply this patch 10) Repeat step 7 11) Note that "Ace" no longer shows in the results list 12) Perform a regular search by putting the letter "D" in the "Name:" field, and hit the "Search" button 13) Note this time the results *do* have Ace in them Signed-off-by: Frederic Demians <f.demians@tamil.fr> Well described for a tricky bug. Reproducible. Fixed with this patch. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This works as described, no problems or regressions found. Created attachment 34172 [details] [review] Bug 13189 [QA Followup] - Unit Tests Can u please rebase? Thanks! holding until unit tests are pushed Created attachment 37741 [details] [review] [Signed-off] Bug 13189 [QA Followup] - Unit Tests Resolved easy conflict (test count) and tested. Signed-off-by: Marc Véron <veron@veron.ch> I hope it is OK to set it back to "Passed QA" as it was before status "Patch doesn't apply" Created attachment 37747 [details] [review] [PASSED QA] Bug 13189: (QA Followup) Unit Tests Resolved easy conflict (test count) and tested. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Remaining patch (regression tests) pushed. Thanks Kyle, and Marc (rebasing)! Comment on attachment 37747 [details] [review] [PASSED QA] Bug 13189: (QA Followup) Unit Tests Review of attachment 37747 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Members.t @@ +189,5 @@ > +my $attr_type = C4::Members::AttributeTypes->new("TEST", "Test Description"); > +$attr_type->staff_searchable(1); > +$attr_type->store(); > +SetBorrowerAttributes($addmem, [ { code => 'TEST', value => 'XYZZYX' } ] ); > +$results = Search( "XYZZYX", undef, undef, undef, undef, undef, 1 ); It was really not a good idea to push this patch. It introduces a call to C4::Members::Search (I fight with it for months to remove SQLHelper...) Tomas, that means no sense at all,
commit f6c750165de6a96b058dccd77b0000848cb3ef96
Date: Fri Mar 20 17:48:43 2015 +0100
Bug 13891: DataTables server-side processing - patron cards
has been pushed and remove the Search calls in the pl files.
I think the second patch should be reverted to master.
|