Bug 18635 - Koha::Patron->guarantees() should return results alphabetically
Summary: Koha::Patron->guarantees() should return results alphabetically
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-19 01:06 UTC by Nick Clemens
Modified: 2018-09-21 12:40 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18635 - Unit tests (3.75 KB, patch)
2017-05-19 01:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically (1.01 KB, patch)
2017-05-19 01:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Unit tests (3.88 KB, patch)
2017-05-19 05:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically (1.14 KB, patch)
2017-05-19 05:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18635 - (QA followup) move tests (6.40 KB, patch)
2017-05-22 14:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically (1.14 KB, patch)
2017-08-28 16:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Unit tests (7.00 KB, patch)
2017-08-28 16:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically (1.14 KB, patch)
2018-07-16 14:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Unit tests (3.22 KB, patch)
2018-07-16 14:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Koha::Patron->guarantees() should return results alphabetically (1.14 KB, patch)
2018-07-16 16:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Unit tests (3.61 KB, patch)
2018-07-16 16:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635 - Unit tests (5.22 KB, patch)
2018-07-16 16:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18635: Koha::Patron->guarantees() should return results alphabetically (1.19 KB, patch)
2018-08-02 12:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18635: Unit tests (5.27 KB, patch)
2018-08-02 12:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (?)