Bug 15548 - New patron related code should have been put to Patron instead of Borrower
Summary: New patron related code should have been put to Patron instead of Borrower
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15449 15635 15653 15656 20951
  Show dependency treegraph
 
Reported: 2016-01-11 16:00 UTC by Jonathan Druart
Modified: 2018-06-15 11:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 15548: Move new patron related code to Patron* (113.43 KB, patch)
2016-01-11 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15548: Move new patron related code to Patron* (113.63 KB, patch)
2016-01-14 04:13 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15548: Move new patron related code to Patron* (113.68 KB, patch)
2016-01-15 18:44 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - Catch a couple new Koha::Borrower uses (1.02 KB, patch)
2016-01-15 18:44 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 15548: Move new patron related code to Patron* (113.71 KB, patch)
2016-01-21 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - Catch a couple new Koha::Borrower uses (1.02 KB, patch)
2016-01-21 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548: Move new patron related code to Patron* (113.65 KB, patch)
2016-02-16 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - Catch a couple new Koha::Borrower uses (1.02 KB, patch)
2016-02-16 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - More new uses of Koha::Borrower (3.69 KB, patch)
2016-02-16 15:36 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 15548: Move new patron related code to Patron* (113.75 KB, patch)
2016-03-03 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - Catch a couple new Koha::Borrower uses (1.02 KB, patch)
2016-03-03 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15548 [QA Followup] - More new uses of Koha::Borrower (3.69 KB, patch)
2016-03-03 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-01-11 16:00:30 UTC
As previously agreed, we need to standardize our wording, Borrower should be Patron.
Comment 1 Jonathan Druart 2016-01-11 16:11:31 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-01-11 18:49:44 UTC
Testing with patch:
t/db_dependent/Circulation.t OK
t/db_dependent/Members.t OK
t/db_dependent/Hold.t OK
t/db_dependent/Item OK
t/db_dependent/Koha_Misc_Files.t (OK)
t/db_dependent/Patron.t OK
t/db_dependent/Patrons.t OK
t/db_dependent/Virtualshelves.t OK
t/Patron.t OK
t/db_dependent/Accounts.t OK
t/db_dependent/BiblioObject.t OK
t/db_dependent/Circulation/AnonymiseIssueHistory.t (Fails with and without patch, could be related to my test server)
t/db_dependent/Creators/Lib.t (OK)
t/db_dependent/Hold.t (OK)
t/db_dependent/Koha_borrower_modifications.t (OK)
t/db_dependent/Borrower_Debarments.t OK
t/db_dependent/Borrower_Discharge.t OK
t/db_dependent/Borrower_Files.t OK
t/db_dependent/Review.t
t/db_dependent/api/v1/patrons.t (Fails with and without patch)


I think somebody else should run all the tests as well (especially the two failing without and with patches).
Comment 3 Héctor Eduardo Castro Avalos 2016-01-12 03:11:10 UTC
Hi Marc and Jonathan

For me all involved tests works successfully

t/Patron.t
t/db_dependent/Accounts.t
t/db_dependent/BiblioObject.t
t/db_dependent/Circulation/AnonymiseIssueHistory.t
t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t
t/db_dependent/Creators/Lib.t
t/db_dependent/Hold.t
t/db_dependent/Koha_borrower_modifications.t
t/db_dependent/Patron.t
t/db_dependent/Patron/Borrower_Debarments.t
t/db_dependent/Patron/Borrower_Discharge.t
t/db_dependent/Patron/Borrower_Files.t
t/db_dependent/Patrons.t
t/db_dependent/Review.t
t/db_dependent/api/v1/patrons.t

The next was renamed/removed
Renamed t/Borrower.t
Renamed t/db_dependent/Borrower.t
Renamed t/db_dependent/Borrowers.t

git grep Koha::Borrower showed entries in release notes for 3.18 only.

I will start to test the modules implied. It seems to be that you only replaced "borrower" to "patron" but IMHO I think this needs a rigorous test plan since you changed a lot in this bug.
Comment 4 Marc Véron 2016-01-12 08:39:33 UTC
(In reply to Héctor Eduardo Castro Avalos from comment #3)
> (...)
> 
> For me all involved tests works successfully
> 

Glad to hear. I'm quite sure that the two fails in my tests were related to some local issue on my VM.
Comment 5 Héctor Eduardo Castro Avalos 2016-01-14 04:13:43 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2016-01-14 08:24:15 UTC
(In reply to Jonathan Druart from comment #0)
> As previously agreed, we need to standardize our wording, Borrower should be
> Patron.

Was this announced somewhere?
Comment 8 Marcel de Rooy 2016-01-14 09:38:55 UTC
(In reply to Jonathan Druart from comment #7)
> http://lists.koha-community.org/pipermail/koha-devel/2015-June/041551.html
> http://wiki.koha-community.org/wiki/Terminology

Agreed seems to be a strong word, if I am searching back.
It was only discussed on the second part of a dev meeting on June 24 with just three or four attending. 
http://irc.koha-community.org/koha/2015-06-24#i_1694243

This means that the list on the wiki does not have community consensus yet. We cannot definitely say here that Patron e.g. is the new standard as decided by the community (on a regular IRC meeting).

Apart from that, I am all for consistency but not for refactoring code just to reword. It will most probably generate new errors.
Comment 9 Jonathan Druart 2016-01-14 09:50:22 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > http://lists.koha-community.org/pipermail/koha-devel/2015-June/041551.html
> > http://wiki.koha-community.org/wiki/Terminology
> 
> Agreed seems to be a strong word, if I am searching back.
> It was only discussed on the second part of a dev meeting on June 24 with
> just three or four attending. 
> http://irc.koha-community.org/koha/2015-06-24#i_1694243
> 
> This means that the list on the wiki does not have community consensus yet.
> We cannot definitely say here that Patron e.g. is the new standard as
> decided by the community (on a regular IRC meeting).
> 
> Apart from that, I am all for consistency but not for refactoring code just
> to reword. It will most probably generate new errors.

It's not "only to reword", it's to keep consistency.
We have now 2 packages Koha::Borrowers and Koha::Patrons, which represent the same thing. To make things clear, it's important to have only 1 word to represent the same entity. A new dev will know where (s)he should add new code patron-related.
I am pretty sure that the use of the "patron" term has been voted years ago for the interface.
Comment 10 Kyle M Hall (khall) 2016-01-15 18:44:14 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall (khall) 2016-01-15 18:44:22 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall (khall) 2016-01-15 18:52:46 UTC
RM Note:
git grep Koha::Borrower

should only result in references to:
misc/release_notes/release_notes_3_18_0.txt
Comment 13 Jonathan Druart 2016-01-19 12:43:49 UTC
I have added a new test in the QA tools:

commit 352357f88d1225075b555723f9135e91eb76571c
    Koha::Borrower has been moved by bug 15548
Comment 14 Jonathan Druart 2016-01-21 10:24:29 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-01-21 10:24:33 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-01-21 10:25:02 UTC
Patches rebased, easy conflicts with bug 15295 fixed.
Comment 17 Jonathan Druart 2016-02-16 11:09:09 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-02-16 11:09:12 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-02-16 11:09:30 UTC
Just another rebase.
Comment 20 Kyle M Hall (khall) 2016-02-16 15:36:34 UTC Comment hidden (obsolete)
Comment 21 Brendan Gallagher 2016-03-02 22:44:26 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 22 Jonathan Druart 2016-03-03 08:24:25 UTC
Created attachment 48588 [details] [review]
Bug 15548: Move new patron related code to Patron*

The 'borrower' should not be used anymore, especially for new code.
This patch move files and rename variables newly pushed (i.e. in the Koha
namespace).

Test plan:
1/
  git grep Koha::Borrower
should not return code in use.

2/
Prove the different modified test files

3/ Do some clicks in the member^Wpatron module to be sure there is not
an obvious error.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as described. Tested with Circulation, Members/Patrons, Discharge,
Restrictions modules and the must common functionalities

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Jonathan Druart 2016-03-03 08:24:31 UTC
Created attachment 48589 [details] [review]
Bug 15548 [QA Followup] - Catch a couple new Koha::Borrower uses

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Jonathan Druart 2016-03-03 08:24:34 UTC
Created attachment 48590 [details] [review]
Bug 15548 [QA Followup] - More new uses of Koha::Borrower

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jesse Weaver 2016-03-03 21:54:48 UTC
Pushed to master, thanks for your work!