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.

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.

(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.

Patches rebased.

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>

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>

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>

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>

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>

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>

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>

Created attachment 55424 [details] [review]
Bug 16911 [QA Followup] - Fix number of categories to look for after deleting one

Pushed to master for 16.11, thanks Jonathan!

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>

(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.

(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.

(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 :-/

(In reply to Kyle M Hall from comment #36)
> Pushed to master for 16.11, thanks Jonathan!

Enhancement, skipping for 16.05.x branch