This bug will add Koha Plugin hooks into catalogue related code allowing integration into areas such as koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
Created attachment 76374 [details] [review] Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button
Created attachment 76375 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
I see a few issues the shouldn't be hard to resolve: 1) Unit tests ( they are super simple, but should be there anyway ) 2) Why test for the method intranet_catalog_biblio_enhancements, but then call the method get_toolbar_button? A) You can check for the method get_toolbar_button directly, and B) You are calling get_toolbar_button twice per plugin, which seems like waste of cycles. 3) After resolving 1 and 2, could you also make pull request to the kitchen sink plugin (https://github.com/bywatersolutions/koha-plugin-kitchen-sink) to add an example to it? That plugin is basically out documentation for the plugin system's functionality at this time.
4) I forgot to make explicit that it seems like the method intranet_catalog_biblio_enhancements is not really necessary and should be removed and replaced with a GetPlugins call for get_toolbar_button instead. 5) Can you make the method names a bit more explicit? I think get_opac_toolbar_button would be better than get_toolbar_button so future developers have a bit better an idea what it does without further research.
On further inspection, I go with something even more specific, get_opac_catalog_toolbar_button, which completely disambiguates where this button will appear.
Hi Kyle Many thanks for this feedback, very helpful indeed. 1) Yes indeed, I completely overlooked tests, I'll address this. 2) Good point, well spotted. I'll fix this. 3) Yes, will do 4) I'm not clear whether you're suggesting we should ditch the intranet_catalog_biblio_enhancements method entirely. I don't think we can do this as we need to be able to test in plugins-home.pl/.tt for plugins that are within this class. If you just meant I should drop it from detail.pm as mentioned in 2), yes, I will do this. 5) Yes, good idea, will do Thanks again. I'll update here once this is all done.
Hi Kyle Looking at 1), I'm not sure writing tests for this tiny amount of core integration is even possible. All we're doing is populating a template parameter under a certain condition, I don't think we can meaningfully test that. Any thoughts?
Created attachment 76530 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button
Created attachment 76531 [details] [review] Bug 20968: Add unit tests
Created attachment 76532 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button
> 4) I'm not clear whether you're suggesting we should ditch the > intranet_catalog_biblio_enhancements method entirely. I don't think we can > do this as we need to be able to test in plugins-home.pl/.tt for plugins > that are within this class. If you just meant I should drop it from > detail.pm as mentioned in 2), yes, I will do this. I didn't fully understand it's purpose at first. What you've done in the followup is excellent. I've added to more patches for you, one that add's those unit tests, and another that renames get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button I know that's quite wordy, but it keeps the naming convention to be in line with the naming convention for the opac payment methods. If you these followups work for you, I think we'll be good to go! Just sign off on my patches, I'll sign off on yours and we can let someone else QA. Once it's passed qa, I'll add those methods to the Kitchen Sink plugin as well!
Apologies for all the typos in my last reply. It's just one of those days I guess ;)
Created attachment 76540 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button
Created attachment 76541 [details] [review] Bug 20968: Add unit tests
Hi Kyle Perfect, thanks very much for that. I've tested and signed off (I think) your patches. Cheers
Created attachment 76555 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces
Created attachment 76564 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters" https://bugs.koha-community.org/show_bug.cgi?id=20968 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 76565 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 76566 [details] [review] Bug 20968: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 76567 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 76568 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Applying: Bug 20968: (follow-up) Replace tabs with spaces error: sha1 information is lacking or useless (catalogue/detail.pl). error: could not build fake ancestor Patch failed at 0001 Bug 20968: (follow-up) Replace tabs with spaces
Any clue as to why this patch isn't applying for you Jonathan? It's literally replacing tabs with spaces, so no idea why it wouldn't apply. Presumably since Kyle signed it off, it applied for him?
(In reply to Andrew Isherwood from comment #23) > Any clue as to why this patch isn't applying for you Jonathan? It's > literally replacing tabs with spaces, so no idea why it wouldn't apply. > Presumably since Kyle signed it off, it applied for him? It longer applies for me either. Here's the conflict that needs resolved in detail.pl: 313 <<<<<<< HEAD 314 315 316 if ( $analyze ) { 317 # count if item is used in analytical bibliorecords 318 # The 'countanalytics' flag is only used in the templates if analyze is set 319 my $countanalytics = C4::Context->preference('EasyAnalyticalRecords') ? GetAnalyticsCount($item->{itemnumber}) : 0; 320 if ($countanalytics > 0){ 321 $analytics_flag=1; 322 $item->{countanalytics} = $countanalytics; 323 } 324 ======= 325 326 #count if item is used in analytical bibliorecords 327 my $countanalytics= GetAnalyticsCount($item->{itemnumber}); 328 if ($countanalytics > 0){ 329 $analytics_flag=1; 330 $item->{countanalytics} = $countanalytics; 331 >>>>>>> Bug 20968: (follow-up) Replace tabs with spaces
Created attachment 77782 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
Created attachment 77783 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button
Created attachment 77784 [details] [review] Bug 20968: Add unit tests
Created attachment 77785 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button
Created attachment 77786 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces
Rebased on top of master. To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
Created attachment 78457 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
Created attachment 78458 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button
Created attachment 78459 [details] [review] Bug 20968: Add unit tests
Created attachment 78460 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button
Created attachment 78461 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces
Created attachment 79804 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
Created attachment 79805 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button
Created attachment 79806 [details] [review] Bug 20968: Add unit tests
Created attachment 79807 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button
Created attachment 79808 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces
Rebased upon master
More detailed test plan: - Follow the installation and configuration instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin - Go to the bib details for a record, selecting the "Normal" tab - You should see a "CLA Permissions Check" button in the button bar above the bib details, click it - This should take you to a new screen and a modal should load containing the results of the permissions check => TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query - In order to perform the second test, it is necessary to remove the API key previously entered, so navigate back to the plugin configuration screen and remove the API, then click "Save configuration" - Navigate back to the bib record and click the "CLA permissions check" button => TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters"
Created attachment 80394 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters" https://bugs.koha-community.org/show_bug.cgi?id=20968 Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Created attachment 80395 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Created attachment 80396 [details] [review] Bug 20968: Add unit tests Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Created attachment 80397 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Created attachment 80398 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Created attachment 80615 [details] [review] Bug 20968: Add hooks into cat-toolbar.inc to display buttons This commit adds hooks into cat-toolbar.inc to allow the display of arbitrary buttons as supplied by plugins within the intranet_catalog_biblio_enhancements class that provide the method get_toolbar_button To test: 1) Download the plugin from https://github.com/PTFS-Europe/cla-permissions-check-plugin 2) Follow the installation instructions from: https://github.com/PTFS-Europe/cla-permissions-check-plugin 3) TEST: Observe that the "CLA Check Permissions" button takes the user to the Check Permissions page and a modal displays containing the results of the query 4) Remove the API key from the plugin "Configuration" page 5) TEST: Ensure that the "CLA Check Permissions" button takes the user to the Check Permissions page and message displays informing the user that "Did not receive required request parameters" https://bugs.koha-community.org/show_bug.cgi?id=20968 Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80616 [details] [review] Bug 20968: (follow-up) Respond to feedback This patch: - Removes the unecessary testing for intranet_catalog_biblio_enhancements method in detail.pl - Renames get_toolbar_button to get_intranet_catalog_toolbar_button Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80617 [details] [review] Bug 20968: Add unit tests Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80618 [details] [review] Bug 20968: Rename method get_intranet_catalog_toolbar_button to intranet_catalog_biblio_enhancements_toolbar_button Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80619 [details] [review] Bug 20968: (follow-up) Replace tabs with spaces Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.06 as per my policy of backporting easily backportable non-consumer facing plugin support changes (to allow for the greatest span of plugin support).
Before pushing to 17.11.x, I'd like to see it in kitchen sink plugin
Hi Fridolin - It seems that Kyle has added support for it in the Kitchen Sink plugin. The test plan for Bug 21905 tests this area and requires use of the Kitchen Sink
(In reply to Andrew Isherwood from comment #56) > Hi Fridolin - It seems that Kyle has added support for it in the Kitchen > Sink plugin. The test plan for Bug 21905 tests this area and requires use of > the Kitchen Sink Ok i see it today in plugin sources
Pushed to 17.11.x for 17.11.13 "\\//_ Live long and prosper"