Bug 16911 - Koha::Patrons - Move ExtendMemberSubscriptionTo to ->renew_account
Summary: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->renew_account
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 (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16846 16912 17307 17699
  Show dependency treegraph
 
Reported: 2016-07-12 18:40 UTC by Jonathan Druart
Modified: 2017-10-24 03:38 UTC (History)
4 users (show)

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


Attachments
Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date (2.54 KB, patch)
2016-07-12 18:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date (7.03 KB, patch)
2016-07-12 18:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Add tests for ->extend_subscription (3.42 KB, patch)
2016-07-12 18:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription (4.90 KB, patch)
2016-07-12 18:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Rename extend_subscription with renew_account (3.86 KB, patch)
2016-07-12 19:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date (2.54 KB, patch)
2016-07-20 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date (6.99 KB, patch)
2016-07-20 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Add tests for ->extend_subscription (3.42 KB, patch)
2016-07-20 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription (4.81 KB, patch)
2016-07-20 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Rename extend_subscription with renew_account (3.89 KB, patch)
2016-07-20 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: ->get_expiry_date accepts strings (2.30 KB, patch)
2016-07-20 18:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date (2.56 KB, patch)
2016-08-23 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date (6.99 KB, patch)
2016-08-23 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Add tests for ->extend_subscription (3.42 KB, patch)
2016-08-23 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription (4.84 KB, patch)
2016-08-23 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: Rename extend_subscription with renew_account (3.89 KB, patch)
2016-08-23 10:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16911: ->get_expiry_date accepts strings (2.33 KB, patch)
2016-08-23 10:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date (2.63 KB, patch)
2016-08-23 23:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date (7.06 KB, patch)
2016-08-23 23:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: Koha::Patrons - Add tests for ->extend_subscription (3.49 KB, patch)
2016-08-23 23:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription (4.91 KB, patch)
2016-08-23 23:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: Rename extend_subscription with renew_account (3.96 KB, patch)
2016-08-23 23:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16911: ->get_expiry_date accepts strings (2.40 KB, patch)
2016-08-23 23:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date (2.67 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date (7.10 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Add tests for ->extend_subscription (3.53 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription (4.95 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: Rename extend_subscription with renew_account (4.00 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: ->get_expiry_date accepts strings (2.44 KB, patch)
2016-09-05 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911: (QA followup) CGI->param should not be called in list context (1.02 KB, patch)
2016-09-05 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16911 [QA Followup] - Fix number of categories to look for after deleting one (951 bytes, patch)
2016-09-09 10:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16911 [QA Followup] - No need to 'use C4::Members' (701 bytes, patch)
2016-09-09 15:53 UTC, Kyle M Hall
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-07-12 18:40:12 UTC
The goal of these patches will be to move the 2 subroutines GetExpiryDate and ExtendMemberSubscriptionTo to the 2 methods of Koha::Patron::Category->get_expiry_date and Koha::Patron->get_expiry_date

This way, we will have a nice code like
  my $patron = Koha::Patrons->find( 42 );
  $patron->move_to_deleted; # From bug 16907
  $patron->delete; # Which will handle the different deletion to do before deleting a patron
Comment 1 Jonathan Druart 2016-07-12 18:56:14 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-07-12 18:56:17 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-07-12 18:56:20 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-07-12 18:56:24 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2016-07-12 19:19:26 UTC
Hm, just a note about the name: extend_subscription sounds more like a routine in serials. I was thinking maybe extend_membership or something like renew_account could work a bit better.
Comment 6 Jonathan Druart 2016-07-12 19:29:38 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-07-20 13:57:22 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-07-20 13:57:26 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-07-20 13:57:30 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-07-20 13:57:33 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-07-20 13:57:36 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2016-07-20 17:00:01 UTC
This seems to work correctly with a patron category which has a fixed expiration date, but when I try to add a patron with an enrollment period defined in months I get an error:

Can't locate object method "add" via package "2016-07-06" (perhaps you forgot to load "2016-07-06"?) at Koha/Patron/Category.pm line 185.

"2016-07-06" is the date I entered in the "registration date" field.
Comment 13 Jonathan Druart 2016-07-20 18:06:40 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-07-20 18:07:53 UTC
(In reply to Owen Leonard from comment #12)
> This seems to work correctly with a patron category which has a fixed
> expiration date, but when I try to add a patron with an enrollment period
> defined in months I get an error:
> 
> Can't locate object method "add" via package "2016-07-06" (perhaps you
> forgot to load "2016-07-06"?) at Koha/Patron/Category.pm line 185.
> 
> "2016-07-06" is the date I entered in the "registration date" field.

Good catch Owen, I did not try that case.
Comment 15 Jonathan Druart 2016-08-23 10:01:45 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-08-23 10:01:48 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-08-23 10:01:54 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-08-23 10:01:58 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-08-23 10:02:02 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-08-23 10:02:06 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-08-23 10:02:34 UTC
Patches rebased.
Comment 22 Aleisha Amohia 2016-08-23 23:21:46 UTC Comment hidden (obsolete)
Comment 23 Aleisha Amohia 2016-08-23 23:21:53 UTC Comment hidden (obsolete)
Comment 24 Aleisha Amohia 2016-08-23 23:22:01 UTC Comment hidden (obsolete)
Comment 25 Aleisha Amohia 2016-08-23 23:22:08 UTC Comment hidden (obsolete)
Comment 26 Aleisha Amohia 2016-08-23 23:22:15 UTC Comment hidden (obsolete)
Comment 27 Aleisha Amohia 2016-08-23 23:22:21 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2016-09-05 18:24:04 UTC
Created attachment 55191 [details] [review]
Bug 16911: Koha::Patron::Categories - Add tests for ->get_expiry_date

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2016-09-05 18:24:13 UTC
Created attachment 55192 [details] [review]
Bug 16911: Koha::Patron::Categories - Move GetExpiryDate to ->get_expiry_date

This patch moves the code from C4::Members::GetExpiryDate to
Koha::Patron::Categories.
Nothing much exiting, please follow test plan in next patches.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2016-09-05 18:24:23 UTC
Created attachment 55193 [details] [review]
Bug 16911: Koha::Patrons - Add tests for ->extend_subscription

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2016-09-05 18:24:37 UTC
Created attachment 55194 [details] [review]
Bug 16911: Koha::Patrons - Move ExtendMemberSubscriptionTo to ->extend_subscription

This patch moves the code from C4::Members::ExtendMemberSubscriptionTo
to Koha::Patron->extend_subscription.

The expected behavior is:
When a new patron is created, the enrolment period defined for the
patron category is used unless an enrolment period date is defined. In
that case, this date is used.
When an account is renewed, the pref BorrowerRenewalPeriodBase is used to
determine if the subscription is renewed from today or from the day when his/her
account has expired.

Test plan:
Confirm that the behavior is correct before this patch and that it's
still the same after this patchset applied.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2016-09-05 18:24:47 UTC
Created attachment 55195 [details] [review]
Bug 16911: Rename extend_subscription with renew_account

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2016-09-05 18:24:56 UTC
Created attachment 55196 [details] [review]
Bug 16911: ->get_expiry_date accepts strings

To make sure nothing else will be broken, it's certainly better to
handle date strings in a first phase.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2016-09-05 18:25:05 UTC
Created attachment 55197 [details] [review]
Bug 16911: (QA followup) CGI->param should not be called in list context

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Kyle M Hall 2016-09-09 10:47:11 UTC
Created attachment 55424 [details] [review]
Bug 16911 [QA Followup] - Fix number of categories to look for after deleting one
Comment 36 Kyle M Hall 2016-09-09 10:55:09 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 37 Kyle M Hall 2016-09-09 15:53:15 UTC
Created attachment 55447 [details] [review]
Bug 16911 [QA Followup] - No need to 'use C4::Members'

For some reason, this is causing t/db_dependent/Circulation.t to fail if
run independently.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Jonathan Druart 2016-09-12 15:11:33 UTC
(In reply to Kyle M Hall from comment #37)
> Created attachment 55447 [details] [review] [review]
> Bug 16911 [QA Followup] - No need to 'use C4::Members'
> 
> For some reason, this is causing t/db_dependent/Circulation.t to fail if
> run independently.
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

I do not understand this patch, the subroutine AddEnrolmentFeeIfNeeded from C4::Members is called from Koha::Patron. It would make sense to use it.
Comment 39 Kyle M Hall 2016-09-27 16:53:25 UTC
(In reply to Jonathan Druart from comment #38)
> (In reply to Kyle M Hall from comment #37)
> > Created attachment 55447 [details] [review] [review] [review]
> > Bug 16911 [QA Followup] - No need to 'use C4::Members'
> > 
> > For some reason, this is causing t/db_dependent/Circulation.t to fail if
> > run independently.
> > 
> > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> 
> I do not understand this patch, the subroutine AddEnrolmentFeeIfNeeded from
> C4::Members is called from Koha::Patron. It would make sense to use it.

Do the unit tests fail for you without that patch? If not, maybe it's a quirk of my environment. Because the call to AddEnrolmentFeeIfNeeded is fully qualified, the 'use' is not needed, and would be unnecessary overhead ( though incredibly minimal ).

If the tests pass for you without that patch, I'd be happy to revert that specific commit.
Comment 40 Jonathan Druart 2016-10-03 07:41:30 UTC
(In reply to Kyle M Hall from comment #39)
> (In reply to Jonathan Druart from comment #38)
> > (In reply to Kyle M Hall from comment #37)
> > > Created attachment 55447 [details] [review] [review] [review] [review]
> > > Bug 16911 [QA Followup] - No need to 'use C4::Members'
> > > 
> > > For some reason, this is causing t/db_dependent/Circulation.t to fail if
> > > run independently.
> > > 
> > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> > 
> > I do not understand this patch, the subroutine AddEnrolmentFeeIfNeeded from
> > C4::Members is called from Koha::Patron. It would make sense to use it.
> 
> Do the unit tests fail for you without that patch? If not, maybe it's a
> quirk of my environment. Because the call to AddEnrolmentFeeIfNeeded is
> fully qualified, the 'use' is not needed, and would be unnecessary overhead
> ( though incredibly minimal ).
> 
> If the tests pass for you without that patch, I'd be happy to revert that
> specific commit.

No, the tests don't pass, but I don't understand why they do with this patch.
I am suspecting there is bug hidden somewhere else :-/
Comment 41 Mason James 2017-10-24 03:38:35 UTC
(In reply to Kyle M Hall from comment #36)
> Pushed to master for 16.11, thanks Jonathan!

Enhancement, skipping for 16.05.x branch