Bug 17557

Summary: Move GetAge to Koha::Patron->get_age (and remove SetAge)
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17521    
Bug Blocks: 16846    
Attachments: Bug 17557: Revised patron age calculation tests
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)
[SIGNED-OFF] Bug 17557: Revised patron age calculation tests
[SIGNED-OFF] Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)
Bug 17557: Revised patron age calculation tests
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)
Bug 17557: Revised patron age calculation tests
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)

Description Jonathan Druart 2016-11-04 16:50:59 UTC

    
Comment 1 Jonathan Druart 2016-11-04 17:04:15 UTC
Created attachment 57218 [details] [review]
Bug 17557: Revised patron age calculation tests

The SetAge and GetAge test coverage are excessive.
First the SetAge subroutine was only created for testing purpose.
The goal of GetAge is quite simple and it seems quite easy to provide
corect test coverage using DateTime->add using negative numbers.
Comment 2 Jonathan Druart 2016-11-04 17:04:19 UTC
Created attachment 57219 [details] [review]
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)

As said in the previous commit, I considered SetAge as unnecessary and
removed it.

Test plan:
1/ Edit a patron using the different 'Edit' links
2/ Play with the patron category limited to age ranges, and date of
birth
3/ You should get the expected warning if the date of birth is inside
the patron category date range.

To finish:
  prove t/Circulation/AgeRestrictionMarkers.t t/db_dependent/Reserves.t \
        t/db_dependent/Koha/Patrons.t t/db_dependent/Members.t
should return green
Comment 3 Josef Moravec 2016-11-04 17:20:56 UTC
Created attachment 57220 [details] [review]
[SIGNED-OFF] Bug 17557: Revised patron age calculation tests

The SetAge and GetAge test coverage are excessive.
First the SetAge subroutine was only created for testing purpose.
The goal of GetAge is quite simple and it seems quite easy to provide
corect test coverage using DateTime->add using negative numbers.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Josef Moravec 2016-11-04 17:21:02 UTC
Created attachment 57221 [details] [review]
[SIGNED-OFF] Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)

As said in the previous commit, I considered SetAge as unnecessary and
removed it.

Test plan:
1/ Edit a patron using the different 'Edit' links
2/ Play with the patron category limited to age ranges, and date of
birth
3/ You should get the expected warning if the date of birth is inside
the patron category date range.

To finish:
  prove t/Circulation/AgeRestrictionMarkers.t t/db_dependent/Reserves.t \
        t/db_dependent/Koha/Patrons.t t/db_dependent/Members.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Tomás Cohen Arazi 2016-12-01 17:40:16 UTC
Created attachment 57862 [details] [review]
Bug 17557: Revised patron age calculation tests

The SetAge and GetAge test coverage are excessive.
First the SetAge subroutine was only created for testing purpose.
The goal of GetAge is quite simple and it seems quite easy to provide
corect test coverage using DateTime->add using negative numbers.

Edit: rebased so it applies

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2016-12-01 17:40:27 UTC
Created attachment 57863 [details] [review]
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)

As said in the previous commit, I considered SetAge as unnecessary and
removed it.

Test plan:
1/ Edit a patron using the different 'Edit' links
2/ Play with the patron category limited to age ranges, and date of
birth
3/ You should get the expected warning if the date of birth is inside
the patron category date range.

To finish:
  prove t/Circulation/AgeRestrictionMarkers.t t/db_dependent/Reserves.t \
        t/db_dependent/Koha/Patrons.t t/db_dependent/Members.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Tomás Cohen Arazi 2016-12-01 17:42:27 UTC
Passing rebased patches.
Comment 8 Jonathan Druart 2016-12-12 10:09:28 UTC
Created attachment 58104 [details] [review]
Bug 17557: Revised patron age calculation tests

The SetAge and GetAge test coverage are excessive.
First the SetAge subroutine was only created for testing purpose.
The goal of GetAge is quite simple and it seems quite easy to provide
corect test coverage using DateTime->add using negative numbers.

Edit: rebased so it applies

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Jonathan Druart 2016-12-12 10:09:32 UTC
Created attachment 58105 [details] [review]
Bug 17557: Koha::Patrons - Move GetAge to ->set_age (and remove SetAge)

As said in the previous commit, I considered SetAge as unnecessary and
removed it.

Test plan:
1/ Edit a patron using the different 'Edit' links
2/ Play with the patron category limited to age ranges, and date of
birth
3/ You should get the expected warning if the date of birth is inside
the patron category date range.

To finish:
  prove t/Circulation/AgeRestrictionMarkers.t t/db_dependent/Reserves.t \
        t/db_dependent/Koha/Patrons.t t/db_dependent/Members.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Kyle M Hall 2016-12-16 12:02:18 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2016-12-18 20:02:48 UTC
Should this go into 16.11?
Comment 12 Katrin Fischer 2016-12-18 20:39:07 UTC
Hm, missed the second patch looking at git - this is a true enhancement and not just new unit tests. So:

This won't get backported to 16.11.x as it is an enhancement.