Bug 18635

Summary: Koha::Patron->guarantees() should return results alphabetically
Product: Koha Reporter: Nick Clemens <nick>
Component: PatronsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, m.de.rooy, martin.renvoize, veron
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:
Attachments: Bug 18635 - Unit tests
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Bug 18635 - Unit tests
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Bug 18635 - (QA followup) move tests
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Bug 18635 - Unit tests
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Bug 18635 - Unit tests
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Bug 18635 - Unit tests
Bug 18635 - Unit tests
Bug 18635: Koha::Patron->guarantees() should return results alphabetically
Bug 18635: Unit tests

Description Nick Clemens 2017-05-19 01:06:51 UTC
Currently there is no sort here so patrons are returned in db order (unordered)

We should return them alphabetically for display and other purposes
Comment 1 Nick Clemens 2017-05-19 01:10:30 UTC
Created attachment 63552 [details] [review]
Bug 18635 - Unit tests

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Patron.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass
Comment 2 Nick Clemens 2017-05-19 01:10:33 UTC
Created attachment 63553 [details] [review]
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical
Comment 3 Marc Véron 2017-05-19 05:30:43 UTC
Created attachment 63558 [details] [review]
Bug 18635 - Unit tests

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Patron.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 4 Marc Véron 2017-05-19 05:34:37 UTC
Created attachment 63559 [details] [review]
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 5 Jonathan Druart 2017-05-22 13:53:07 UTC
Nick, tests for Koha::Patrons are in t/db_dependent/Koha/Patrons.t
Comment 6 Nick Clemens 2017-05-22 14:50:38 UTC
Created attachment 63617 [details] [review]
Bug 18635 - (QA followup) move tests
Comment 7 Jonathan Druart 2017-05-22 19:19:01 UTC
Sorry but it is t/db_dependent/Koha/Patrons.t, not t/db_dependent/Patrons.t
Comment 8 Jonathan Druart 2017-05-22 19:25:34 UTC
Should not we override Koha::Patrons->search instead?
Comment 9 Nick Clemens 2017-08-28 16:24:48 UTC
Created attachment 66557 [details] [review]
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Nick Clemens 2017-08-28 16:24:52 UTC
Created attachment 66558 [details] [review]
Bug 18635 - Unit tests

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Patron.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 11 Nick Clemens 2017-08-28 16:27:15 UTC
(In reply to Jonathan Druart from comment #8)
> Should not we override Koha::Patrons->search instead?

I am not sure, happy to have another opinion on this or make the change if you think it is necessary
Comment 12 Marcel de Rooy 2017-09-01 07:26:24 UTC
Nick,
What are you doing here?!
Deleting tests without further notice?

Failed QA
Comment 13 Jonathan Druart 2018-06-29 13:44:10 UTC
(In reply to Marcel de Rooy from comment #12)
> Nick,
> What are you doing here?!
> Deleting tests without further notice?
> 
> Failed QA

Which tests are missing? Maybe as_list, is it what you meant?
Comment 14 Katrin Fischer 2018-07-01 17:43:39 UTC
Hi Nick, the first patch is removing t/db_dependent/Patrons.t. I think this is what Jonathan meant?
Comment 15 Nick Clemens 2018-07-16 14:58:53 UTC
Created attachment 77005 [details] [review]
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 16 Nick Clemens 2018-07-16 14:58:57 UTC
Created attachment 77006 [details] [review]
Bug 18635 - Unit tests

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Patron.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 17 Nick Clemens 2018-07-16 14:59:53 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Marcel de Rooy from comment #12)
> > Nick,
> > What are you doing here?!
> > Deleting tests without further notice?
> > 
> > Failed QA
> 
> Which tests are missing? Maybe as_list, is it what you meant?

When I moved the tests I also deleted tests, that was an error, undone
Comment 18 Jonathan Druart 2018-07-16 15:10:50 UTC
I do not think it was, tests in t/db_dependent/Patrons.t are pointless.

Anyway, did you see that we already have a subtest for this method, in the same file?

t/db_dependent/Koha/Patrons.t:subtest 'guarantees' => sub {
Comment 19 Nick Clemens 2018-07-16 16:17:17 UTC
Created attachment 77011 [details] [review]
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 20 Nick Clemens 2018-07-16 16:17:21 UTC
Created attachment 77012 [details] [review]
Bug 18635 - Unit tests

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Koha/Patrons.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 21 Nick Clemens 2018-07-16 16:30:23 UTC
Created attachment 77013 [details] [review]
Bug 18635 - Unit tests

I also adjust a few tests that assume value of $nb_of_patrons to not
rely on existing datas

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Koha/Patrons.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 22 Katrin Fischer 2018-08-02 12:36:40 UTC
Created attachment 77435 [details] [review]
Bug 18635: Koha::Patron->guarantees() should return results alphabetically

To test:
1 - Add the same guarantor to a number of patrons, ensuring random order
alphabetically
2 - View the guarantors record, note unordered list of guarantees
3 - Apply patch
4 - Guarantees are alphabetical

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2018-08-02 12:36:44 UTC
Created attachment 77436 [details] [review]
Bug 18635: Unit tests

I also adjust a few tests that assume value of $nb_of_patrons to not
rely on existing datas

To test:
1 - Apply this patch only
2 - Prove t/db_dependent/Koha/Patrons.t
3 - Tests fail
4 - Apply other patch
5 - Tests pass

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2018-08-02 12:36:56 UTC
Fixed the commit messages.
Comment 25 Nick Clemens 2018-08-09 12:22:50 UTC
Awesome work all!

Pushed to master for 18.11
Comment 26 Martin Renvoize 2018-08-10 08:15:53 UTC
Trivial enhancement which could be seen as a bugfix by many.

Pushed to 18.05.x fro 18.05.03
Comment 27 Fridolin Somers 2018-09-21 12:33:37 UTC
UT patch does not apply easily, I prefer not push to 17.11.x
Comment 28 Jonathan Druart 2018-09-21 12:40:51 UTC
(In reply to Fridolin SOMERS from comment #27)
> UT patch does not apply easily, I prefer not push to 17.11.x

It only adds a subtest (?)
Comment 29 Fridolin Somers 2018-09-24 15:03:01 UTC
Ok I've managed to adapt UT.

BTW the 1st patch was pushed with 17.11.10.

UT patch will be in 17.11.11