Bug 23050 - Add hook to add a tab in intranet biblio details page
Summary: Add hook to add a tab in intranet biblio details page
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Julian Maurice
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-05 12:30 UTC by Matthias Meusburger
Modified: 2019-11-14 11:49 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23050: Plugin hook to add tabs in intranet biblio details page (2.51 KB, patch)
2019-06-05 14:29 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 23050: Plugin hook to add tabs in intranet biblio details page (2.57 KB, patch)
2019-06-06 08:02 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 23050: Plugin hook to add tabs in intranet biblio details page (2.57 KB, patch)
2019-07-15 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 23050: Add missing template filters (963 bytes, patch)
2019-07-15 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 23050: Fix tab's id to avoid compatibility issues (1.22 KB, patch)
2019-07-15 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 23050: Fix creation of biblio tab's id (906 bytes, patch)
2019-09-30 08:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 23050: Plugin hook to add tabs in intranet biblio details page (2.62 KB, patch)
2019-11-04 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23050: Add missing template filters (1018 bytes, patch)
2019-11-04 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23050: Fix tab's id to avoid compatibility issues (1.27 KB, patch)
2019-11-04 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23050: Fix creation of biblio tab's id (962 bytes, patch)
2019-11-04 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23050: (QA follow-up) Add Koha::Plugins::Tab class (6.78 KB, patch)
2019-11-04 15:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23050: (RM follow-up) Add Try::Tiny to controller (664 bytes, patch)
2019-11-13 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2019-06-05 12:30:58 UTC
Add a plugin hook that will allow plugins to add a new tab in intranet biblio details page (catalogue/detail.pl)
Comment 1 Julian Maurice 2019-06-05 14:29:14 UTC
Created attachment 90337 [details] [review]
Bug 23050: Plugin hook to add tabs in intranet biblio details page

Test Plan:
1) Enable plugins
2) Download and install the latest version of this plugin
https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook
3) Browse to catalogue/detail.pl for a record
4) Note you see two new tabs with content
Comment 2 Claire Gravely 2019-06-06 08:02:59 UTC
Created attachment 90348 [details] [review]
Bug 23050: Plugin hook to add tabs in intranet biblio details page

Test Plan:
1) Enable plugins
2) Download and install the latest version of this plugin
https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook
3) Browse to catalogue/detail.pl for a record
4) Note you see two new tabs with content

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 3 Katrin Fischer 2019-07-11 06:07:39 UTC
Hi Julian,

nice patch!

1) Some complaints form the QA script:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
   FAIL	  filters
		wrong_html_filter at line 173 (    <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)
		missing_filter at line 173 (    <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)
		missing_filter at line 173 (    <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)

2) Wondering: What happens if the tab name consists non-latin characters, diacritics etc. Will this still work ok as id?

+            $tab->{id} = $tab->{title};
+            $tab->{id} =~ s/[^\w]+/-/g;

I've found this looking for valid characters in ids:

he value must not contain any space characters. HTML 4: ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").

3) Can you please document the new hooks on the wiki (once we have this in PQA)?
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 4 Julian Maurice 2019-07-15 12:52:04 UTC
Created attachment 91523 [details] [review]
Bug 23050: Plugin hook to add tabs in intranet biblio details page

Test Plan:
1) Enable plugins
2) Download and install the latest version of this plugin
https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook
3) Browse to catalogue/detail.pl for a record
4) Note you see two new tabs with content

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 5 Julian Maurice 2019-07-15 12:52:07 UTC
Created attachment 91524 [details] [review]
Bug 23050: Add missing template filters
Comment 6 Julian Maurice 2019-07-15 12:52:11 UTC
Created attachment 91525 [details] [review]
Bug 23050: Fix tab's id to avoid compatibility issues

MDN says:

  Using characters except ASCII letters, digits, '_', '-' and '.' may
  cause compatibility problems, as they weren't allowed in HTML 4.
  Though this restriction has been lifted in HTML5, an ID should start
  with a letter for compatibility.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
Comment 7 Julian Maurice 2019-07-15 12:54:51 UTC
All issues mentioned in comment 3 should be fixed.

I also modified the https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks page to include the bug number and the first Koha version that includes the hooks.
Comment 8 Tomás Cohen Arazi 2019-09-06 19:21:17 UTC
Julian, I think this is a pretty cool idea.
I don't like it that you generate content on the .pl (tab id, title, etc). It feels like all that could be done on the templates if we pass the plugins like:

Why not just:

    my @tab_plugins = Koha::Plugins->new()->GetPlugins({
        method => 'intranet_catalog_biblio_tab',
    });
     $template->param(
        plugins     => \@plugins,
        tab_plugins => \@tab_pluginss,
     );

Also, (for a later enhancement) there should be a way to define the order in which the tabs will be placed. Not sure how/where.
Comment 9 Julian Maurice 2019-09-30 08:08:26 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> Julian, I think this is a pretty cool idea.
> I don't like it that you generate content on the .pl (tab id, title, etc).
> It feels like all that could be done on the templates

It could be done in templates, but, as we need the tab id in two different places, it means it would duplicate the code that creates that id, right ?
Comment 10 Julian Maurice 2019-09-30 08:11:53 UTC
Created attachment 93222 [details] [review]
Bug 23050: Fix creation of biblio tab's id

(Too much javascript in my head...)
Comment 11 Tomás Cohen Arazi 2019-11-04 13:29:59 UTC
Created attachment 95004 [details] [review]
Bug 23050: Plugin hook to add tabs in intranet biblio details page

Test Plan:
1) Enable plugins
2) Download and install the latest version of this plugin
https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook
3) Browse to catalogue/detail.pl for a record
4) Note you see two new tabs with content

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2019-11-04 13:30:04 UTC
Created attachment 95005 [details] [review]
Bug 23050: Add missing template filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2019-11-04 13:30:08 UTC
Created attachment 95006 [details] [review]
Bug 23050: Fix tab's id to avoid compatibility issues

MDN says:

  Using characters except ASCII letters, digits, '_', '-' and '.' may
  cause compatibility problems, as they weren't allowed in HTML 4.
  Though this restriction has been lifted in HTML5, an ID should start
  with a letter for compatibility.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2019-11-04 13:30:13 UTC
Created attachment 95007 [details] [review]
Bug 23050: Fix creation of biblio tab's id

(Too much javascript in my head...)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2019-11-04 15:03:25 UTC
Created attachment 95013 [details] [review]
Bug 23050: (QA follow-up) Add Koha::Plugins::Tab class

This minimal class encapsulates the tabs to be passed around to the
templates, so error checking on missing bits is done in a single place.

It throws exceptions on errors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Katrin Fischer 2019-11-04 21:38:20 UTC
Thx, Tomas!
Comment 17 Martin Renvoize 2019-11-05 08:14:22 UTC
Nice work!

Pushed to master for 19.11.00
Comment 18 Matthias Meusburger 2019-11-05 10:13:40 UTC
Tomás, didn't you forgot to "use Try::Tiny;" in catalogue/detail.pl for your last patch? (attachment 95013 [details] [review])

Without the use statement, I get the following error when trying to display content:

Can't locate object method "catch" via package "1" (perhaps you forgot to load "1"?) at /home/koha/src/catalogue/detail.pl line 89.

With the use statement, it works properly.
Comment 19 Tomás Cohen Arazi 2019-11-05 10:19:54 UTC
(In reply to Matthias Meusburger from comment #18)
> Tomás, didn't you forgot to "use Try::Tiny;" in catalogue/detail.pl for your
> last patch? (attachment 95013 [details] [review] [review])
> 
> Without the use statement, I get the following error when trying to display
> content:
> 
> Can't locate object method "catch" via package "1" (perhaps you forgot to
> load "1"?) at /home/koha/src/catalogue/detail.pl line 89.
> 
> With the use statement, it works properly.

I did
Comment 20 Martin Renvoize 2019-11-13 16:20:10 UTC
Created attachment 95406 [details] [review]
Bug 23050: (RM follow-up) Add Try::Tiny to controller

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>