Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip.
Created attachment 31638 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the ODUE slip 4) Try Print > Print overdues
Hm, wouldn't it be better to have a separate template for a slip? I think the print transport type for ODUEs is thought for printing letters mostly and this could confuse things a bit. Also, not all libraries use ODUE - it's not hardcoded as the notice triggers will let you pick any slip that is in the circulation module.
Created attachment 31667 [details] [review] Bug 12933 [Followup] - Switch from DBI to DBIC
I tried to run the test plan, on step 4, I got a 404: http:// ... /cgi-bin/koha/members/print_overdues.pl?borrowernumber=220 The file print_overdues.pl does not exist.
Created attachment 34456 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues
Created attachment 34457 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter
Created attachment 34467 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues
Created attachment 34468 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter
Created attachment 34469 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 2) Locate a patron with overdues 4) Try Print > Print overdues
Created attachment 34470 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 2) Locate a patron with overdues 4) Try Print > Print overdues
Created attachment 34471 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter
Test Plan: 1) Apply this patch 2) Run ./installer/data/mysql/updatedatabase.pl 3) Locate a patron with overdues 4) Try Print > Print overdues
Created attachment 34474 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 34475 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
QA comment: I am quite confuse by this patch. IMO there are some inconsistencies: 1/ I don't understand the usefulness to pass by the template plugin to know if a borrower has overdues or not. 2/ Why did you move parse_letter to the pm file? A subroutine with the same name exists in advance_notices.pl cronjob script (which uses C4::Overdues too, so introduces a redefinion of it)? 3/ Same for C4::Members::GetOverdues, we already have a GetOverdues subroutine in C4::Overdues. 4/ Tests are missing for changes in packages. Marked as Failed QA.
(In reply to Jonathan Druart from comment #15) > QA comment: > I am quite confuse by this patch. > IMO there are some inconsistencies: > 1/ I don't understand the usefulness to pass by the template plugin to know > if a borrower has overdues or not. By using a TT plugin, we don't have to modify each and every script that uses the members toolbar. > 2/ Why did you move parse_letter to the pm file? A subroutine with the same > name exists in advance_notices.pl cronjob script (which uses C4::Overdues > too, so introduces a redefinion of it)? I moved it so we could use the same subroutine in both the cron script and the new script. I'll submit a followup to rename it to parse_overdues_letter > 3/ Same for C4::Members::GetOverdues, we already have a GetOverdues > subroutine in C4::Overdues. C4::Members::GetOverdues deals with getting a specific borrowers overdues, and gets more data than C4::Overdues::GetOverdues. They really do two different jobs. > 4/ Tests are missing for changes in packages. Will do!
Created attachment 34884 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 34885 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 34886 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 34887 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 34888 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 34889 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
(In reply to Kyle M Hall from comment #16) > (In reply to Jonathan Druart from comment #15) I agree with Joubu that I don't like having a second GetOverdues added to the code.. it's not DRY.. if you really want to be able to access the function from C4::Members rather than directly from C4::Overdues, then it should be a sugar function that refers to C4::Overdues in my mind, else it's confusing to future people working on the code. If it does perform a different function.. then surely that should be made clear by naming it something completely different, or enahcning the C4::Overdues function with the required extras and a filter perhaps? Just my thoughts
Created attachment 35533 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 35534 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 35535 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 35536 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 35537 [details] [review] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron
Created attachment 35538 [details] [review] Bug 12933 [QA Followup] - Remove trailing whitespace from Members.pm
(In reply to Kyle M Hall from comment #16) > (In reply to Jonathan Druart from comment #15) > > QA comment: > > I am quite confuse by this patch. > > IMO there are some inconsistencies: > > 1/ I don't understand the usefulness to pass by the template plugin to know > > if a borrower has overdues or not. > > By using a TT plugin, we don't have to modify each and every script that > uses the members toolbar. Yes, but it's the way it's done everywhere into Koha. I still really don't like to see an other overhead to get this kind of info. Other QA pov needed here please.
(In reply to Kyle M Hall from comment #29) > Created attachment 35538 [details] [review] [review] > Bug 12933 [QA Followup] - Remove trailing whitespace from Members.pm There are 26 patches in the queue modifying C4/Members.pm. IMO this patch does not give enough benefit to be required.
I'll write a followup to remove the plugin sub and use of it, replacing it with something more common. Until then, setting to Failed QA
Created attachment 35918 [details] [review] Bug 12933 [QA Followup] - Remove Template plugin and use thereof
Created attachment 41606 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 41607 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 41608 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 41609 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 41610 [details] [review] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron
Created attachment 41611 [details] [review] Bug 12933 [QA Followup] - Remove Template plugin and use thereof
> > By using a TT plugin, we don't have to modify each and every script that > > uses the members toolbar. > > Yes, but it's the way it's done everywhere into Koha. > I still really don't like to see an other overhead to get this kind of info. There is no sacrifice or performance using this method. In fact, based on my followup, using the plugin results in somewhat better performance! The key here is that just because we've always done something in a particular way doesn't mean we should keep doing it that way. In fact, we have precedence for this very pattern. Once we introduced the plugin for getting system preferences, we began to remove the template parameters that only passed system preferences. There is no need to fetch the values in the script and pass them to the template, because the script has absolutely no need for them. Doing so only added unnecessary complexity and by using a plugin we greatly reduce that complexity. This is essentially the exact same issue.
There is something wrong in the second patch: installer/data/mysql/updatedatabase.pl | 1157 +-------------------
Created attachment 43514 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43515 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43516 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 43517 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 43518 [details] [review] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron
Created attachment 43519 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43520 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43521 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 43522 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 43523 [details] [review] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron
(In reply to Jonathan Druart from comment #41) > There is something wrong in the second patch: > > installer/data/mysql/updatedatabase.pl | 1157 > +------------------- Fixed!
Sorry, patch doesn't apply, please rebase: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 12933 - Add ability to print overdue slip from staff intranet Using index info to reconstruct a base tree... M C4/Overdues.pm Falling back to patching base and 3-way merge... Auto-merging C4/Overdues.pm CONFLICT (content): Merge conflict in C4/Overdues.pm Failed to merge in the changes. Patch failed at 0001 Bug 12933 - Add ability to print overdue slip from staff intranet The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-12933---Add-ability-to-print-overdue-slip-from-IujNUN.patch
Created attachment 43904 [details] [review] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43905 [details] [review] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org>
Created attachment 43906 [details] [review] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter
Created attachment 43907 [details] [review] Bug 12933 [QA Followup] - Add Unit Tests
Created attachment 43908 [details] [review] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron
Created attachment 43925 [details] [review] [PASSED QA] Bug 12933 - Add ability to print overdue slip from staff intranet Some librarians would like to be able to print an overdues slip from the staff intranet. This slip would be defined as the print transport version of the ODUE slip. Test Plan: 1) Apply this patch 2) Locate a patron with overdues 3) Define a print version of the OVERDUES_SLIP slip 4) Try Print > Print overdues Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43926 [details] [review] [PASSED QA] Bug 12933 - Add new OVERDUES_SLIP letter Signed-off-by: Amy Purvis <APurvis@galencollege.edu> Signed-off-by: Laurie McKee <lmckee@littleelm.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43927 [details] [review] [PASSED QA] Bug 12933 [QA Followup] - Rename parse_letter to parse_overdues_letter Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43928 [details] [review] [PASSED QA] Bug 12933 [QA Followup] - Add Unit Tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43929 [details] [review] [PASSED QA] Bug 12933 [QA Followup] - Rename GetOverdues to GetOverduesForPatron Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43930 [details] [review] Bug 12933: QA Followq-up - Add missing notice templates and other small fixes - Fixes page title of slip print window: Members > Patrons - Fixes error message if no notice template is found - Fixes a stray template variable that resulted in the borrower number showing in the 'Print' pull down - Fixes xt/sample_notices.t by adding the sample notice to it-IT and es-ES
Created attachment 43953 [details] [review] Bug 12933: QA Followq-up - Add missing notice templates and other small fixes - Fixes page title of slip print window: Members > Patrons - Fixes error message if no notice template is found - Fixes a stray template variable that resulted in the borrower number showing in the 'Print' pull down - Fixes xt/sample_notices.t by adding the sample notice to it-IT and es-ES Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Patches pushed to master. Thanks Kyle!
(In reply to Jonathan Druart from comment #15) > QA comment: > 3/ Same for C4::Members::GetOverdues, we already have a GetOverdues > subroutine in C4::Overdues. Was valid for HasOverdues too. See bug 15135.
(In reply to Jonathan Druart from comment #67) > (In reply to Jonathan Druart from comment #15) > > QA comment: > > 3/ Same for C4::Members::GetOverdues, we already have a GetOverdues > > subroutine in C4::Overdues. > > Was valid for HasOverdues too. > See bug 15135. This is still valid, please fix. returns.pl: Subroutine HasOverdues redefined at /home/koha/src/Koha/Template/Plugin/Borrowers.pm line 52