The simplify and make uniform the code, the controller scripts should send a Koha::Patron object to the templates and not all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone.
Created attachment 64211 [details] [review] Bug 18789: Send Koha::Patron object to the templates In order to simplify and make uniform the code, the controller scripts send a Koha::Patron object to the templates instead of all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone. The variable "patron" sent to the templates is supposed to represent the patron the librarian is editing the detail. In the members module and some scripts of the circulation module, the patron's detail are sent one by one to the template. That leads to frustration from developpers (making sure everything is passed from all scripts) and to regression (we got tone of bugs in the last year because of this way to do). With this patch set it will be easy access patron's detail, passing only 1 variable from the controllers. Test plan: Play with the patron and circulation module and make sur the detail of the patron you are editing/seeing info are correctly displayed.
Created attachment 64212 [details] [review] Bug 18789: Pass a Koha::Patron object from returns.pl This is a specific patch for returns.pl Test plan: Do some checkin for a patron and make sure the info are correctly displayed (and info from someone that has placed a hold on this item)
Created attachment 64213 [details] [review] Bug 18789: Use Koha::Patron->image from the templates Now that we have the 'patron' variable in all our templates, we can call Koha::Patron->image and do the check from the templates. Test plan: On the different pages of the patron module, you should see the default image if there is no image attached or the one that has been attached (see pref patronimages)
Created attachment 64214 [details] [review] Bug 18789: add tests for is_adult and is_child This patch add 2 methods to the Koha::Patron module: is_adult and is_child. That way the business logic is at only one place and covered by tests.
Created attachment 64215 [details] [review] Bug 18789: Use Koha::Patron->is_child where needed Test plan: When you are on a page related to a child you should see the "Update child to adult patron" button
Created attachment 64216 [details] [review] Bug 18789: Use Koha::Patron->is_adult where needed Test plan: When editing adult's info you must see the "Add child" button
Created attachment 64218 [details] [review] Bug 18789: Remove useless IS_ADULT variable Test plan: git grep IS_ADULT must not return any results
Created attachment 64219 [details] [review] Bug 18789: Remove adultborrower from the pay* scripts This flag is not used in the templates
Created attachment 64221 [details] [review] Bug 18789: Update other occurences where is->child should be used Trivial changes here, just do a code review on the QA step
Created attachment 64229 [details] [review] Bug 18789: (follow-up)Send Koha::Patron object to the templates
Patch did not apply: $ git bz apply 18789 Bug 18789 - Send a Koha::Patron object to the templates 64211 - Bug 18789: Send Koha::Patron object to the templates 64212 - Bug 18789: Pass a Koha::Patron object from returns.pl 64213 - Bug 18789: Use Koha::Patron->image from the templates 64214 - Bug 18789: add tests for is_adult and is_child 64215 - Bug 18789: Use Koha::Patron->is_child where needed 64216 - Bug 18789: Use Koha::Patron->is_adult where needed 64218 - Bug 18789: Remove useless IS_ADULT variable 64219 - Bug 18789: Remove adultborrower from the pay* scripts 64221 - Bug 18789: Update other occurences where is->child should be used 64229 - Bug 18789: (follow-up)Send Koha::Patron object to the templates Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 18789: Send Koha::Patron object to the templates fatal: sha1 information is lacking or useless (acqui/booksellers.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 18789: Send Koha::Patron object to the templates
You need to apply the dependencies first.
Created attachment 71301 [details] [review] Bug 18789: Send Koha::Patron object to the templates In order to simplify and make uniform the code, the controller scripts send a Koha::Patron object to the templates instead of all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone. The variable "patron" sent to the templates is supposed to represent the patron the librarian is editing the detail. In the members module and some scripts of the circulation module, the patron's detail are sent one by one to the template. That leads to frustration from developpers (making sure everything is passed from all scripts) and to regression (we got tone of bugs in the last year because of this way to do). With this patch set it will be easy access patron's detail, passing only 1 variable from the controllers. Test plan: Play with the patron and circulation module and make sur the detail of the patron you are editing/seeing info are correctly displayed.
Created attachment 71302 [details] [review] Bug 18789: Pass a Koha::Patron object from returns.pl This is a specific patch for returns.pl Test plan: Do some checkin for a patron and make sure the info are correctly displayed (and info from someone that has placed a hold on this item)
Created attachment 71303 [details] [review] Bug 18789: Use Koha::Patron->image from the templates Now that we have the 'patron' variable in all our templates, we can call Koha::Patron->image and do the check from the templates. Test plan: On the different pages of the patron module, you should see the default image if there is no image attached or the one that has been attached (see pref patronimages)
Created attachment 71304 [details] [review] Bug 18789: add tests for is_adult and is_child This patch add 2 methods to the Koha::Patron module: is_adult and is_child. That way the business logic is at only one place and covered by tests.
Created attachment 71305 [details] [review] Bug 18789: Use Koha::Patron->is_child where needed Test plan: When you are on a page related to a child you should see the "Update child to adult patron" button
Created attachment 71306 [details] [review] Bug 18789: Use Koha::Patron->is_adult where needed Test plan: When editing adult's info you must see the "Add child" button
Created attachment 71307 [details] [review] Bug 18789: Remove useless IS_ADULT variable Test plan: git grep IS_ADULT must not return any results
Created attachment 71308 [details] [review] Bug 18789: Remove adultborrower from the pay* scripts This flag is not used in the templates
Created attachment 71309 [details] [review] Bug 18789: Update other occurences where is->child should be used Trivial changes here, just do a code review on the QA step
Created attachment 71310 [details] [review] Bug 18789: (follow-up)Send Koha::Patron object to the templates
This is very hard to rebase and will introduce a lot of issues if not pushed quickly.
Created attachment 71311 [details] [review] Bug 18789: (follow-up) Use Koha::Patron->image from the templates
Created attachment 71485 [details] [review] Bug 18789: Send Koha::Patron object to the templates In order to simplify and make uniform the code, the controller scripts send a Koha::Patron object to the templates instead of all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone. The variable "patron" sent to the templates is supposed to represent the patron the librarian is editing the detail. In the members module and some scripts of the circulation module, the patron's detail are sent one by one to the template. That leads to frustration from developpers (making sure everything is passed from all scripts) and to regression (we got tone of bugs in the last year because of this way to do). With this patch set it will be easy access patron's detail, passing only 1 variable from the controllers. Test plan: Play with the patron and circulation module and make sur the detail of the patron you are editing/seeing info are correctly displayed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71486 [details] [review] Bug 18789: Pass a Koha::Patron object from returns.pl This is a specific patch for returns.pl Test plan: Do some checkin for a patron and make sure the info are correctly displayed (and info from someone that has placed a hold on this item) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71487 [details] [review] Bug 18789: Use Koha::Patron->image from the templates Now that we have the 'patron' variable in all our templates, we can call Koha::Patron->image and do the check from the templates. Test plan: On the different pages of the patron module, you should see the default image if there is no image attached or the one that has been attached (see pref patronimages) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71488 [details] [review] Bug 18789: add tests for is_adult and is_child This patch add 2 methods to the Koha::Patron module: is_adult and is_child. That way the business logic is at only one place and covered by tests. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71489 [details] [review] Bug 18789: Use Koha::Patron->is_child where needed Test plan: When you are on a page related to a child you should see the "Update child to adult patron" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71490 [details] [review] Bug 18789: Use Koha::Patron->is_adult where needed Test plan: When editing adult's info you must see the "Add child" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71491 [details] [review] Bug 18789: Remove useless IS_ADULT variable Test plan: git grep IS_ADULT must not return any results Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71492 [details] [review] Bug 18789: Remove adultborrower from the pay* scripts This flag is not used in the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71493 [details] [review] Bug 18789: Update other occurences where is->child should be used Trivial changes here, just do a code review on the QA step Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71494 [details] [review] Bug 18789: (follow-up)Send Koha::Patron object to the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71495 [details] [review] Bug 18789: (follow-up) Use Koha::Patron->image from the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71496 [details] [review] Bug 18789: Fix number of unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71679 [details] [review] Bug 18789: Send Koha::Patron object to the templates In order to simplify and make uniform the code, the controller scripts send a Koha::Patron object to the templates instead of all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone. The variable "patron" sent to the templates is supposed to represent the patron the librarian is editing the detail. In the members module and some scripts of the circulation module, the patron's detail are sent one by one to the template. That leads to frustration from developpers (making sure everything is passed from all scripts) and to regression (we got tone of bugs in the last year because of this way to do). With this patch set it will be easy access patron's detail, passing only 1 variable from the controllers. Test plan: Play with the patron and circulation module and make sur the detail of the patron you are editing/seeing info are correctly displayed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71680 [details] [review] Bug 18789: Pass a Koha::Patron object from returns.pl This is a specific patch for returns.pl Test plan: Do some checkin for a patron and make sure the info are correctly displayed (and info from someone that has placed a hold on this item) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71681 [details] [review] Bug 18789: Use Koha::Patron->image from the templates Now that we have the 'patron' variable in all our templates, we can call Koha::Patron->image and do the check from the templates. Test plan: On the different pages of the patron module, you should see the default image if there is no image attached or the one that has been attached (see pref patronimages) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71682 [details] [review] Bug 18789: add tests for is_adult and is_child This patch add 2 methods to the Koha::Patron module: is_adult and is_child. That way the business logic is at only one place and covered by tests. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71683 [details] [review] Bug 18789: Use Koha::Patron->is_child where needed Test plan: When you are on a page related to a child you should see the "Update child to adult patron" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71684 [details] [review] Bug 18789: Use Koha::Patron->is_adult where needed Test plan: When editing adult's info you must see the "Add child" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71685 [details] [review] Bug 18789: Remove useless IS_ADULT variable Test plan: git grep IS_ADULT must not return any results Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71686 [details] [review] Bug 18789: Remove adultborrower from the pay* scripts This flag is not used in the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71687 [details] [review] Bug 18789: Update other occurences where is->child should be used Trivial changes here, just do a code review on the QA step Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71688 [details] [review] Bug 18789: (follow-up)Send Koha::Patron object to the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71689 [details] [review] Bug 18789: (follow-up) Use Koha::Patron->image from the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 71690 [details] [review] Bug 18789: add tests for is_adult and is_chil
Created attachment 71691 [details] [review] Bug 18789: Use the patron variable in circulation.[pl|tt]
Created attachment 71713 [details] [review] Bug 18789: Send Koha::Patron object to the templates In order to simplify and make uniform the code, the controller scripts send a Koha::Patron object to the templates instead of all attributes of a patron. That will make the code much more easier to maintain and will be less error-prone. The variable "patron" sent to the templates is supposed to represent the patron the librarian is editing the detail. In the members module and some scripts of the circulation module, the patron's detail are sent one by one to the template. That leads to frustration from developpers (making sure everything is passed from all scripts) and to regression (we got tone of bugs in the last year because of this way to do). With this patch set it will be easy access patron's detail, passing only 1 variable from the controllers. Test plan: Play with the patron and circulation module and make sur the detail of the patron you are editing/seeing info are correctly displayed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71714 [details] [review] Bug 18789: Pass a Koha::Patron object from returns.pl This is a specific patch for returns.pl Test plan: Do some checkin for a patron and make sure the info are correctly displayed (and info from someone that has placed a hold on this item) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71715 [details] [review] Bug 18789: Use Koha::Patron->image from the templates Now that we have the 'patron' variable in all our templates, we can call Koha::Patron->image and do the check from the templates. Test plan: On the different pages of the patron module, you should see the default image if there is no image attached or the one that has been attached (see pref patronimages) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71716 [details] [review] Bug 18789: Add tests for is_adult and is_child This patch add 2 methods to the Koha::Patron module: is_adult and is_child. That way the business logic is at only one place and covered by tests. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71717 [details] [review] Bug 18789: Use Koha::Patron->is_child where needed Test plan: When you are on a page related to a child you should see the "Update child to adult patron" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71718 [details] [review] Bug 18789: Use Koha::Patron->is_adult where needed Test plan: When editing adult's info you must see the "Add child" button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71719 [details] [review] Bug 18789: Remove useless IS_ADULT variable Test plan: git grep IS_ADULT must not return any results Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71720 [details] [review] Bug 18789: Remove adultborrower from the pay* scripts This flag is not used in the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71721 [details] [review] Bug 18789: Update other occurences where is_child should be used Trivial changes here, just do a code review on the QA step Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71722 [details] [review] Bug 18789: (follow-up) Send Koha::Patron object to the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71723 [details] [review] Bug 18789: (follow-up) Use Koha::Patron->image from the templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71724 [details] [review] Bug 18789: Add tests for is_adult and is_child Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71725 [details] [review] Bug 18789: Use the patron variable in circulation.[pl|tt] Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71726 [details] [review] Bug 18789: (QA follow-up) Fix tab char Test plan: run koha qa tools, should be green Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71727 [details] [review] Bug 18789: (QA follow-up) Fix borrowernumber in menu item links Test plan: Go to patrons routing list Try to click Holds history or ILL requests menu link --> without page blank page --> with page desired page Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71728 [details] [review] Bug 18789: (QA follow-up) Fix showing the name of basket creator on booksellers page Test plan: 1) Go to Acquisition and search for a vendor, you are presented with vendors and their baskets 2) Look to Created by column --> without patch only firstname is shown --> with patch you should see full name Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71729 [details] [review] Bug 18789: (QA follow-up) Use first_valid_email_address on Tranfers to receive page For consistancy with waitng reserves Test plan: 1) Have a hold for patron which need to be transfered, inititate transfer and go to target library 2) Go to Circulation -> transfers to receive page and you should see any first valid email for tha patron in column "On hold for" Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71730 [details] [review] Bug 18789: (QA follow-up) Fix loading saved address data to edit patron form Test plan: 1) Have patron with address filled in 2) Edit the patron -- without this patch the fields for address are blank -- with patch the fields are filled with actual data Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71731 [details] [review] Bug 18789: (QA follow-up) Fix showing patron name when checking in Test plan: 1) Have a patron with more then one checkouts 2) Check in one of the checkouts --> without patch patron name is not shown in checked-in items table --> with patch the patron is shown with name, category, number of checkouts and both links are working Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71732 [details] [review] Bug 18789: (QA follow-up) Fix showing cardnumber when creating a hold Test plan: 1) Try to create a hold request for a patron 2) On place hold form on the first line is shown the patron name --> without patch the cardnumber is not shown --> with patch there is cardnumber right after the patron name Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71733 [details] [review] Bug 18789: (QA follow-up) Use patron object in batch checkout template 1) Enable batch checkouts in system preferences and set it on for some patron categories 2) Try to batch checkout for some patron with one of these categories --> without patch the batch checkout page is even not shown --> with patch batch checking out is working as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71734 [details] [review] Bug 18789: (QA follow-up) Use is_child in circulation.pl Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71735 [details] [review] Bug 18789: (QA follow-up) Update holdshistory.pl, fix is_child, is_adult, patron->image, clear template params Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71736 [details] [review] Bug 18789: (QA follow-up) Fix viewlog when accessed via circulation Test plan: Go to any patron and then to Modification log of this patron -- without patch you will not see the left side menu and patron information -- with patch menu and patron information is here, and should be fully working (links) Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Good job Jonathan, I just added some small fixes. I think that memberentry.pl and memberentrygen.tt will need some refactoring too, but I am for to do it on another report, so passing qa.
Created attachment 71827 [details] [review] Bug 18789: Add POD for is_adult and is_child
Pushed to master for 18.05, thanks to everybody involved!
Created attachment 71838 [details] [review] Bug 18789: Fix perlcritic issue - housebound.pl "$branch" is declared but not used at line 65, column 1. Unused variables clutter code and make it harder to read. (Severity: 3)
Created attachment 71841 [details] [review] Bug 18789: (follow-up) Fix page title when creating a new patron This has been caught by selenium test, the category name must be displayed when we are creating a new patron, and so does not depends on the "patron" variable
Enhancement, not backported for 17.11
Created attachment 73293 [details] [review] Bug 18789: (follow-up) Fix place hold page This patch fixes very weird behaviours introduced by commit 0ab22e1c7c6b6e8d14a4fb7478ab2a3d42da347d Bug 18789: Send Koha::Patron object to the templates The patron variable was only set when no action was defined. This patch restores the feature the easiest way possible (less changes) even if it is not the best one.
Last patch pushed to master
Created attachment 73890 [details] [review] Bug 18789: (follow-up) Pass a Koha::Patron object from returns.pl
Created attachment 75237 [details] [review] Bug 18789: Fix borrowernumber on members/files.tt
(In reply to Jonathan Druart from comment #84) > Created attachment 75237 [details] [review] [review] > Bug 18789: Fix borrowernumber on members/files.tt Pushed to master!