Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm KohaBranchName.pm Branches.pm Also, it would be nice to add a usage example as documentation to the surviving plugins.
Created attachment 19866 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible
Created attachment 19867 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible
Created attachment 19869 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t'
Created attachment 19870 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t'
Comment on attachment 19870 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Review of attachment 19870 [details] [review]: ----------------------------------------------------------------- ::: Koha/Template/Plugin/Branches.pm @@ +32,4 @@ > my $sth = C4::Context->dbh->prepare($query); > $sth->execute($branchcode); > my $b = $sth->fetchrow_hashref(); > + return $b ? encode( 'UTF-8', $b->{'branchname'} ) : q{}; By switching our templates to use this plugin, instead of KohaBranchName.pm we have changed the behaviour. We are now not using the GetBranchName subroutine, but instead doing a db query ourselves (which is probably better we shouldnt be using C4:: modules in the Koha:: space) but we are also doing this encode. And I am not convinced encode is what we should be doing here. Could you please explain why we are doing the encode?
The use of encode is to make diacritics display correctly. This issue came up during the development of the Course Reserves ( Bug 10626 ) feature. You can see the patch where Paul added the encode to solve the issue here: http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18283&action=diff (In reply to Chris Cormack from comment #5) > Comment on attachment 19870 [details] [review] [review] > Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches > > Review of attachment 19870 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Template/Plugin/Branches.pm > @@ +32,4 @@ > > my $sth = C4::Context->dbh->prepare($query); > > $sth->execute($branchcode); > > my $b = $sth->fetchrow_hashref(); > > + return $b ? encode( 'UTF-8', $b->{'branchname'} ) : q{}; > > By switching our templates to use this plugin, instead of KohaBranchName.pm > we have changed the behaviour. > > We are now not using the GetBranchName subroutine, but instead doing a db > query ourselves (which is probably better we shouldnt be using C4:: modules > in the Koha:: space) but we are also doing this encode. > > And I am not convinced encode is what we should be doing here. Could you > please explain why we are doing the encode?
Created attachment 20204 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 20205 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t' Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Hi Kyle - view_holdsqueue.tt is still using 'USE KohaAuthorisedValues' - circulation.tt is still using 'USE KohaBranchName' after applying your patches. Could you add 2 follow ups please? Maybe we should also add a test to the QA script?
Created attachment 20659 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup
Created attachment 20660 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup
Sorry, Kyle, but one more for Authorised Values slipped in: /home/katrin/kohaclone/koha-tmpl/opac-tmpl/prog/en/includes/item-status-schema-org.inc I am continueing with QA for KohaBranchName.
Created attachment 20846 [details] [review] [PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t' Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Also tested with a branch name with umlauts.
Created attachment 20847 [details] [review] [PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> To test: Make sure - independentbranches is turned on - Your user doesn't have superlibrarian permission Then - Try to check out an item with home and holding branch != logged in branch Patch works alright.
Created attachment 20848 [details] [review] [PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Comments on second patch.
Created attachment 20849 [details] [review] [PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Trying to write my own test plan here: 1) AllowNotForLoanOverride = Allow Mark single item not for loan and try to check it out. Check item not for loan status shows up correctly in message: Item is normally not for loan (Staff Collection). Check out anyway? 2) Toggle AllowNotForLoanOverride = Not Allow Try to check out your not for loan item again. Message has changed, but not for loan status should still show: Item not for loan (Staff Collection). 3) Check out item that is marked damaged. Check damaged status shows correctly in list of checkouts. 4) Mark item as lost and check it out. Check lost status shows up correctly in message: This item has been lost with a status of "Lange überfällig (Verloren)". 5) Create your own custom SUGGEST_STATUS and check that it shows up correctly on the suggestion page. 6) Test item status show up correctly on OPAC detail page (lost, not for loan, damaged, etc). Requires follow-up patch to be applied. 7) Mark a suggestion with your custom status and make sure it shows up in the patron account in OPAC correctly. Passes tests and QA script - needs a follow up for item-status-schema-org.inc.
Created attachment 20850 [details] [review] Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2 Removes USE KohaAuthorisedValues plugin from item-status-schema-org.inc as it does not seem to be used in the file. Couldn't detect regressions removing it on OPAC detail page.
Created attachment 20962 [details] [review] Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2 Removes USE KohaAuthorisedValues plugin from item-status-schema-org.inc as it does not seem to be used in the file. Couldn't detect regressions removing it on OPAC detail page. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22361 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t' Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Also tested with a branch name with umlauts.
Created attachment 22363 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> To test: Make sure - independentbranches is turned on - Your user doesn't have superlibrarian permission Then - Try to check out an item with home and holding branch != logged in branch Patch works alright.
Created attachment 22364 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Comments on second patch.
Created attachment 22365 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaBranchName.pm Branches.pm Test Plan: 1) Apply this patch 2) View a basket group, note the branch name is displayed 3) View a subscription's details, note the branch name is displayed 4) View suggestions, note the branch names are displayed 5) Return an item that needs transfered, note the branch name is displayed 6) Run 'prove t/db_dependent/Koha_template_plugin_Branches.t' Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Also tested with a branch name with umlauts.
Created attachment 22366 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> To test: Make sure - independentbranches is turned on - Your user doesn't have superlibrarian permission Then - Try to check out an item with home and holding branch != logged in branch Patch works alright.
Created attachment 22367 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Comments on second patch.
Created attachment 22368 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Trying to write my own test plan here: 1) AllowNotForLoanOverride = Allow Mark single item not for loan and try to check it out. Check item not for loan status shows up correctly in message: Item is normally not for loan (Staff Collection). Check out anyway? 2) Toggle AllowNotForLoanOverride = Not Allow Try to check out your not for loan item again. Message has changed, but not for loan status should still show: Item not for loan (Staff Collection). 3) Check out item that is marked damaged. Check damaged status shows correctly in list of checkouts. 4) Mark item as lost and check it out. Check lost status shows up correctly in message: This item has been lost with a status of "Lange überfällig (Verloren)". 5) Create your own custom SUGGEST_STATUS and check that it shows up correctly on the suggestion page. 6) Test item status show up correctly on OPAC detail page (lost, not for loan, damaged, etc). Requires follow-up patch to be applied. 7) Mark a suggestion with your custom status and make sure it shows up in the patron account in OPAC correctly. Passes tests and QA script - needs a follow up for item-status-schema-org.inc.
Created attachment 22369 [details] [review] Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2 Removes USE KohaAuthorisedValues plugin from item-status-schema-org.inc as it does not seem to be used in the file. Couldn't detect regressions removing it on OPAC detail page. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased first patch to fix merge conflicts.
Just a note - this is a bug, because the branches plugin to be removed breaks diacritics in branch names.
I've pushed the two patches merging the branches plugins to master. Thanks, Kyle!
The authorized value patches no longer apply, most likely the result of a conflict with the patch for bug 11322. Kyle, you have my bless to set the status back to passed QA once you've resolved the conflict.
I added a commit in the QA tests for that: commit 32dcd4db9639bb976763ad92759850bf96c2a59d Author: Jonathan Druart <jonathan.druart@biblibre.com> Date: Mon Dec 16 11:02:46 2013 +0100 TT plugins have been merged (see bug 10626)
Created attachment 23693 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues Looking at the TT plugin directory I notice we have some plugins that seem to do the same thing: KohaAuthorisedValues.pm AuthorisedValues.pm Test Plan: 1) Apply this patch 2) View the pages that this patch has modified, make sure the branch name is still visible Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Comments on second patch.
Created attachment 23695 [details] [review] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Trying to write my own test plan here: 1) AllowNotForLoanOverride = Allow Mark single item not for loan and try to check it out. Check item not for loan status shows up correctly in message: Item is normally not for loan (Staff Collection). Check out anyway? 2) Toggle AllowNotForLoanOverride = Not Allow Try to check out your not for loan item again. Message has changed, but not for loan status should still show: Item not for loan (Staff Collection). 3) Check out item that is marked damaged. Check damaged status shows correctly in list of checkouts. 4) Mark item as lost and check it out. Check lost status shows up correctly in message: This item has been lost with a status of "Lange überfällig (Verloren)". 5) Create your own custom SUGGEST_STATUS and check that it shows up correctly on the suggestion page. 6) Test item status show up correctly on OPAC detail page (lost, not for loan, damaged, etc). Requires follow-up patch to be applied. 7) Mark a suggestion with your custom status and make sure it shows up in the patron account in OPAC correctly. Passes tests and QA script - needs a follow up for item-status-schema-org.inc.
Created attachment 23696 [details] [review] Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2 Removes USE KohaAuthorisedValues plugin from item-status-schema-org.inc as it does not seem to be used in the file. Couldn't detect regressions removing it on OPAC detail page. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master, along with a follow-up updating the Bootstrap theme. Thanks, Kyle!
Patches pushed to 3.14.x, will be in 3.14.2