Bug 14888 - Move the cities related code to Koha::Cities
Summary: Move the cities related code to Koha::Cities
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14974 15449 15631 16001
  Show dependency treegraph
 
Reported: 2015-09-24 14:00 UTC by Jonathan Druart
Modified: 2016-12-05 21:27 UTC (History)
4 users (show)

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


Attachments
Bug 14888: Add Koha::Cit[y|ies] classes (2.80 KB, patch)
2015-09-24 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes (17.72 KB, patch)
2015-09-24 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 14888: Add Koha::Cit[y|ies] classes (3.00 KB, patch)
2015-09-24 19:30 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes (17.92 KB, patch)
2015-09-24 19:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14888: Add Koha::Cit[y|ies] classes (2.95 KB, patch)
2015-10-01 14:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14888: Update copyright and wording (1.46 KB, patch)
2015-10-02 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14888: Add tests for Koha::Cit[y|ies] (2.42 KB, patch)
2015-10-02 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 14888: use Koha::Cit[y|ies] in admin/itemtypes (17.97 KB, patch)
2015-10-03 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14888: Add Koha::Cit[y|ies] classes (3.02 KB, patch)
2015-10-03 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14888: Update copyright and wording (1.80 KB, patch)
2015-10-03 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14888: Add tests for Koha::Cit[y|ies] (2.49 KB, patch)
2015-10-03 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14888: (QA followup) remove unused lib/var (829 bytes, patch)
2015-10-05 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-09-24 14:00:09 UTC

    
Comment 1 Jonathan Druart 2015-09-24 14:03:03 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-09-24 14:03:13 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-09-24 19:30:41 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2015-09-24 19:34:41 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-10-01 14:57:48 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-10-01 14:59:39 UTC
Last patch just removes +use C4::Members::Messaging;
Comment 7 Katrin Fischer 2015-10-01 22:11:09 UTC
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.
Comment 8 Jonathan Druart 2015-10-02 07:48:22 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-10-02 07:48:26 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-10-02 07:49:56 UTC
(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.
Comment 11 Katrin Fischer 2015-10-03 21:27:49 UTC
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>
Comment 12 Katrin Fischer 2015-10-03 21:27:52 UTC
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>
Comment 13 Katrin Fischer 2015-10-03 21:27:55 UTC
Created attachment 43088 [details] [review]
[PASSED QA] Bug 14888: Update copyright and wording

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2015-10-03 21:27:58 UTC
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>
Comment 15 Tomás Cohen Arazi 2015-10-05 15:25:58 UTC
Created attachment 43146 [details] [review]
Bug 14888: (QA followup) remove unused lib/var

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2015-10-05 15:38:54 UTC
Patches pushed to master.

Thanks Jonathhhhhhhhan!