Bug 10626

Summary: Remove doubled up TT plugins
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, josef.moravec, kyle
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8004, 9478    
Attachments: Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup
[PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
[PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup
[PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
[PASSED QA] Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup
Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2
Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches
Bug 10626 - Remove doubled up TT plugins - Merge KohaBranchName and Branches - Followup
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup
Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues
Bug 10626 - Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup
Bug 10626: Remove doubled up TT plugins - Merge KohaAuthorisedValues and AuthorisedValues - Followup 2

Description Katrin Fischer 2013-07-22 18:28:58 UTC
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.
Comment 1 Kyle M Hall 2013-07-23 13:23:58 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-23 13:25:39 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-07-23 13:56:08 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-07-23 13:57:09 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2013-07-24 06:26:20 UTC
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?
Comment 6 Kyle M Hall 2013-07-24 09:47:06 UTC
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?
Comment 7 Chris Cormack 2013-08-08 23:43:45 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2013-08-08 23:43:56 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-08-25 20:09:01 UTC
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?
Comment 10 Kyle M Hall 2013-08-27 17:14:29 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-08-27 17:14:40 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2013-09-07 16:28:49 UTC
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.
Comment 13 Katrin Fischer 2013-09-07 17:06:08 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-09-07 17:06:17 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-09-07 17:46:28 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2013-09-07 17:46:38 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-09-07 17:46:49 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-09-10 14:38:19 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-10-24 15:08:04 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-10-24 15:08:40 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-10-24 15:09:00 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-10-24 15:09:23 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-10-24 15:09:27 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-10-24 15:09:32 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-10-24 15:10:22 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-10-24 15:10:32 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2013-10-24 15:10:55 UTC
Rebased first patch to fix merge conflicts.
Comment 28 Katrin Fischer 2013-11-17 19:09:02 UTC
Just a note - this is a bug, because the branches plugin to be removed breaks diacritics in branch names.
Comment 29 Galen Charlton 2013-12-14 01:09:42 UTC
I've pushed the two patches merging the branches plugins to master.  Thanks, Kyle!
Comment 30 Galen Charlton 2013-12-14 01:17:07 UTC
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.
Comment 31 Jonathan Druart 2013-12-16 10:24:15 UTC
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)
Comment 32 Kyle M Hall 2013-12-19 17:14:15 UTC
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.
Comment 33 Kyle M Hall 2013-12-19 17:16:12 UTC
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.
Comment 34 Kyle M Hall 2013-12-19 17:16:26 UTC
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>
Comment 35 Galen Charlton 2013-12-20 04:35:05 UTC
Pushed to master, along with a follow-up updating the Bootstrap theme.  Thanks, Kyle!
Comment 36 Fridolin Somers 2013-12-30 09:51:26 UTC
Patches pushed to 3.14.x, will be in 3.14.2