Bug 19130

Summary: K::A::Booksellers->search broken for attribute 'name'
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13726    
Bug Blocks: 18120    
Attachments: Bug 19130: Remove ->search() overloading and adjust tests
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t
Bug 19130: (followup) Add POD
Bug 19130: (followup) Controller scripts should preserve behaviour
Bug 19130: Remove ->search() overloading and adjust tests
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t
Bug 19130: (followup) Add POD
Bug 19130: (followup) Controller scripts should preserve behaviour
Bug 19130: Remove ->search() overloading and adjust tests
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t
Bug 19130: (followup) Add POD
Bug 19130: (followup) Controller scripts should preserve behaviour

Description Tomás Cohen Arazi 2017-08-16 21:46:29 UTC
The current overloaded implementation doesn't allow to search for stuff that is suffix of a bigger word. For example, if you have

- name: Name2
- name: Name21

the query expression { name => 'Name%2' } should only return 'Name2'. The current implementation returns both.

This was implemented this way to ease the transition into the new code without changing too many things on the controller scripts. It is time to fix it.
Comment 1 Tomás Cohen Arazi 2017-08-22 15:17:36 UTC
Created attachment 66348 [details] [review]
Bug 19130: Remove ->search() overloading and adjust tests

This patch removes the custom ->search() function. Tests are adjusted
so the results from ->search() calls are not expected to return in the
previously hardcoded order.

To test:
- Apply this patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Bookseller.t
=> SUCCESS: Tests pass
- Sign off :-D
Comment 2 Tomás Cohen Arazi 2017-08-22 15:17:42 UTC
Created attachment 66349 [details] [review]
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t

This followup patch adds a proper file in which add tests for
Koha::Acquisition::Bookseller(s) methods.

All current methods are covered.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Acquisition/Booksellers.t
 => SUCCESS: Test pass!
 - Sign off :-D
Comment 3 Tomás Cohen Arazi 2017-08-22 15:17:47 UTC
Created attachment 66350 [details] [review]
Bug 19130: (followup) Add POD

This patch adds POD to the following files:

- Koha/Acquisition/Bookseller.pm
- Koha/Acquisition/Booksellers.pm
Comment 4 Tomás Cohen Arazi 2017-08-22 16:15:44 UTC
Created attachment 66352 [details] [review]
Bug 19130: (followup) Controller scripts should preserve behaviour

This patch is a followup to making
Koha::Acquisition::Booksellers->search work as any other Koha::Objects
(DBIC) query instead of having a different behaviour hardcoded.

To achieve it, this patch makes the controller scripts add
wildcard/truncation chars as prefix and sufix for searches, and make the
default sorting for results be by 'name', ascending.

To test:
- Just verify the behaviour remains unchanged by this patchset on the
controller scripts (re. searching).
Comment 5 Kyle M Hall 2017-08-22 18:19:46 UTC
Created attachment 66365 [details] [review]
Bug 19130: Remove ->search() overloading and adjust tests

This patch removes the custom ->search() function. Tests are adjusted
so the results from ->search() calls are not expected to return in the
previously hardcoded order.

To test:
- Apply this patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Bookseller.t
=> SUCCESS: Tests pass
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2017-08-22 18:19:53 UTC
Created attachment 66366 [details] [review]
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t

This followup patch adds a proper file in which add tests for
Koha::Acquisition::Bookseller(s) methods.

All current methods are covered.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Acquisition/Booksellers.t
 => SUCCESS: Test pass!
 - Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2017-08-22 18:19:56 UTC
Created attachment 66367 [details] [review]
Bug 19130: (followup) Add POD

This patch adds POD to the following files:

- Koha/Acquisition/Bookseller.pm
- Koha/Acquisition/Booksellers.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2017-08-22 18:19:58 UTC
Created attachment 66368 [details] [review]
Bug 19130: (followup) Controller scripts should preserve behaviour

This patch is a followup to making
Koha::Acquisition::Booksellers->search work as any other Koha::Objects
(DBIC) query instead of having a different behaviour hardcoded.

To achieve it, this patch makes the controller scripts add
wildcard/truncation chars as prefix and sufix for searches, and make the
default sorting for results be by 'name', ascending.

To test:
- Just verify the behaviour remains unchanged by this patchset on the
controller scripts (re. searching).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Jonathan Druart 2017-08-25 14:57:59 UTC
Created attachment 66480 [details] [review]
Bug 19130: Remove ->search() overloading and adjust tests

This patch removes the custom ->search() function. Tests are adjusted
so the results from ->search() calls are not expected to return in the
previously hardcoded order.

To test:
- Apply this patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Bookseller.t
=> SUCCESS: Tests pass
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2017-08-25 14:58:04 UTC
Created attachment 66481 [details] [review]
Bug 19130: (followup) Add t/db_dependent/Koha/Acquisition/Booksellers.t

This followup patch adds a proper file in which add tests for
Koha::Acquisition::Bookseller(s) methods.

All current methods are covered.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Acquisition/Booksellers.t
 => SUCCESS: Test pass!
 - Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2017-08-25 14:58:08 UTC
Created attachment 66482 [details] [review]
Bug 19130: (followup) Add POD

This patch adds POD to the following files:

- Koha/Acquisition/Bookseller.pm
- Koha/Acquisition/Booksellers.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2017-08-25 14:58:12 UTC
Created attachment 66483 [details] [review]
Bug 19130: (followup) Controller scripts should preserve behaviour

This patch is a followup to making
Koha::Acquisition::Booksellers->search work as any other Koha::Objects
(DBIC) query instead of having a different behaviour hardcoded.

To achieve it, this patch makes the controller scripts add
wildcard/truncation chars as prefix and sufix for searches, and make the
default sorting for results be by 'name', ascending.

To test:
- Just verify the behaviour remains unchanged by this patchset on the
controller scripts (re. searching).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2017-08-25 14:59:05 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Fridolin Somers 2017-09-29 13:25:06 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 15 Katrin Fischer 2017-10-01 22:19:14 UTC
Dependent bug 13726 is not in 16.11.x.