Created attachment 42844 [details] [review] Bug 14888: Add Koha::Cit[y|ies] classes
Created attachment 42845 [details] [review] Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes Test plan: Add/edit/remove cities from the administration module (admin/cities.pl). You should get message feedback after each action.
Created attachment 42853 [details] [review] [Signed-off] Bug 14888: Add Koha::Cit[y|ies] classes Tested both patches together, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 42854 [details] [review] [Signed-off] Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes Test plan: Add/edit/remove cities from the administration module (admin/cities.pl). You should get message feedback after each action. Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 43015 [details] [review] Bug 14888: Add Koha::Cit[y|ies] classes Tested both patches together, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Last patch just removes +use C4::Members::Messaging;
Some first notes on this: I tested adding modifying and deleting cities and the display of the the cities list. All works well. but 3 requests, most important first: 1) Unit tests please! 2) Should the copyright line be updated/amended in cities.pl? # Copyright 2006 SAN OUEST-PROVENCE et Paul POULAIN 3) A suggestion for the error messages: use 'adding' instead of 'inserting' - as that fits with the terminology in Koha a bit better.
Created attachment 43048 [details] [review] Bug 14888: Update copyright and wording
Created attachment 43049 [details] [review] Bug 14888: Add tests for Koha::Cit[y|ies]
(In reply to Katrin Fischer from comment #7) > Some first notes on this: > > I tested adding modifying and deleting cities and the display of the the > cities list. All works well. but 3 requests, most important first: > > 1) Unit tests please! I am not sure they are useful in this case. The tests for Koha::Object[s] already cover this area. But they cannot hurt :) > 2) Should the copyright line be updated/amended in cities.pl? > # Copyright 2006 SAN OUEST-PROVENCE et Paul POULAIN > > 3) A suggestion for the error messages: use 'adding' instead of 'inserting' > - as that fits with the terminology in Koha a bit better. Done.
Created attachment 43086 [details] [review] [PASSED QA] Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes Test plan: Add/edit/remove cities from the administration module (admin/cities.pl). You should get message feedback after each action. Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43087 [details] [review] [PASSED QA] Bug 14888: Add Koha::Cit[y|ies] classes Tested both patches together, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43088 [details] [review] [PASSED QA] Bug 14888: Update copyright and wording Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43089 [details] [review] [PASSED QA] Bug 14888: Add tests for Koha::Cit[y|ies] Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43146 [details] [review] Bug 14888: (QA followup) remove unused lib/var Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Patches pushed to master. Thanks Jonathhhhhhhhan!