Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes
Summary: Patron card creator patron search browse by last name broken by extended attr...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-03 15:20 UTC by Kyle M Hall
Modified: 2015-12-03 22:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes (4.32 KB, patch)
2014-11-04 11:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes (4.49 KB, patch)
2014-11-10 15:54 UTC, Frédéric Demians
Details | Diff | Splinter Review
[PASSED QA] Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes (4.62 KB, patch)
2014-11-11 07:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13189 - Patron card creator patron search browse by last name broken by extended attributes (4.56 KB, patch)
2014-12-08 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13189 [QA Followup] - Unit Tests (1.70 KB, patch)
2014-12-08 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed-off] Bug 13189 [QA Followup] - Unit Tests (1.89 KB, patch)
2015-04-13 13:40 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 13189: (QA Followup) Unit Tests (1.94 KB, patch)
2015-04-13 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-11-03 15:20:41 UTC
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!
Comment 1 Kyle M Hall 2014-11-04 11:28:45 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2014-11-10 15:54:59 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-11-11 07:12:21 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-11-11 07:14:39 UTC
As this changes the search routinges in Member.pm, I'd feel better with a regression test here.
Comment 5 Martin Renvoize 2014-11-12 08:10:11 UTC
Regression test++
Comment 6 Kyle M Hall 2014-12-08 13:33:12 UTC
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.
Comment 7 Kyle M Hall 2014-12-08 13:33:19 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2015-02-12 18:35:26 UTC
Can u please rebase? Thanks!
Comment 9 Chris Cormack 2015-02-14 08:19:27 UTC
holding until unit tests are pushed
Comment 10 Marc Véron 2015-04-13 13:40:25 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2015-04-13 13:43:03 UTC
I hope it is OK to set it back to "Passed QA" as it was before status "Patch doesn't apply"
Comment 12 Tomás Cohen Arazi 2015-04-13 14:17:56 UTC
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>
Comment 13 Tomás Cohen Arazi 2015-04-13 14:23:44 UTC
Remaining patch (regression tests) pushed.

Thanks Kyle, and Marc (rebasing)!
Comment 14 Jonathan Druart 2015-04-15 11:40:28 UTC
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...)
Comment 15 Jonathan Druart 2015-04-15 11:53:09 UTC
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.