Currently there is no sort here so patrons are returned in db order (unordered) We should return them alphabetically for display and other purposes
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
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
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>
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>
Nick, tests for Koha::Patrons are in t/db_dependent/Koha/Patrons.t
Created attachment 63617 [details] [review] Bug 18635 - (QA followup) move tests
Sorry but it is t/db_dependent/Koha/Patrons.t, not t/db_dependent/Patrons.t
Should not we override Koha::Patrons->search instead?
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>
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>
(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
Nick, What are you doing here?! Deleting tests without further notice? Failed QA
(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?
Hi Nick, the first patch is removing t/db_dependent/Patrons.t. I think this is what Jonathan meant?
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>
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>
(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
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 {
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>
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>
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>
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>
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>
Fixed the commit messages.
Awesome work all! Pushed to master for 18.11
Trivial enhancement which could be seen as a bugfix by many. Pushed to 18.05.x fro 18.05.03
UT patch does not apply easily, I prefer not push to 17.11.x
(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 (?)
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