Description
HKS3 Tadeusz Sośnierz
2025-04-29 12:20:24 UTC
Created attachment 181648 [details] [review] Bug 39772: Display package names for plugin jobs rather than "unknown" messages This slightly improves the current situation, where on background_jobs.pl each plugin job would be displayed as "Unknown job type 'plugin_...'". After this patch, we display the implementing package name (sourced from plugins' background_tasks()), which is slightly more human-friendly and significantly less error-looking. Test plan: 1. In KTD, install a plugin implementing background jobs (e.g. the KitchenSink) 2. On http://localhost:8081/cgi-bin/koha/plugins/plugins-home.pl, enable the plugin and run a tool/report from it 3. Observe ugly job types column on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl 4. Apply the patch 5. Observe the improved job types on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl Happy to discuss other possible improvements – perhaps job-specific metadata with human readable (and translatable?) names in them. This, however should be a relatively non-invasive and yet noticeable improvement. Using the kitchen sink plugin (https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases) no longer seems to work for me: 1. Actions > Run report: Error 500 page 2. Actions > Run tool: 403 CSRF token error after "Click me" 3. Actions > Run admin tool: no background job I added an assignee. (In reply to HKS3 Tadeusz Sośnierz from comment #2) > Happy to discuss other possible improvements – perhaps job-specific metadata > with human readable (and translatable?) names in them. This, however should > be a relatively non-invasive and yet noticeable improvement. I think this is a good first step. Ideally, we should either add a new hook for plugins to provide descriptions for background job types (e.g. `background_tasks_descriptions`), or even change the `background_tasks` return data structure to include descriptions. In such scenario, what you do here would be the fallback for when the plugin doesn't provide descriptions. So move forward. Created attachment 181712 [details] [review] Bug 39772: Extend background_tasks API to allow for human-readable job names Any suggestions for another plugin to test this with? The kitchen sink plugin doesn't seem to work for me - see comment #3. https://github.com/HKS3/koha-normalize-marc2db is a good testcase. Its https://github.com/HKS3/koha-normalize-marc2db/tree/tadzik/custom-job-names branch utilizes the new API for customized job names too. Created attachment 181853 [details] [review] Bug 39772: Display package names for plugin jobs rather than "unknown" messages This slightly improves the current situation, where on background_jobs.pl each plugin job would be displayed as "Unknown job type 'plugin_...'". After this patch, we display the implementing package name (sourced from plugins' background_tasks()), which is slightly more human-friendly and significantly less error-looking. Test plan: 1. In KTD, install a plugin implementing background jobs (e.g. the KitchenSink) 2. On http://localhost:8081/cgi-bin/koha/plugins/plugins-home.pl, enable the plugin and run a tool/report from it 3. Observe ugly job types column on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl 4. Apply the patch 5. Observe the improved job types on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl Signed-off-by: David Nind <david@davidnind.com> Created attachment 181854 [details] [review] Bug 39772: Extend background_tasks API to allow for human-readable job names Signed-off-by: David Nind <david@davidnind.com> (In reply to HKS3 Tadeusz Sośnierz from comment #7) > https://github.com/HKS3/koha-normalize-marc2db is a good testcase. Its > https://github.com/HKS3/koha-normalize-marc2db/tree/tadzik/custom-job-names > branch utilizes the new API for customized job names too. Thanks! Created attachment 181866 [details] [review] Bug 39772: Display package names for plugin jobs rather than "unknown" messages This slightly improves the current situation, where on background_jobs.pl each plugin job would be displayed as "Unknown job type 'plugin_...'". After this patch, we display the implementing package name (sourced from plugins' background_tasks()), which is slightly more human-friendly and significantly less error-looking. Test plan: 1. In KTD, install a plugin implementing background jobs (e.g. the KitchenSink) 2. On http://localhost:8081/cgi-bin/koha/plugins/plugins-home.pl, enable the plugin and run a tool/report from it 3. Observe ugly job types column on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl 4. Apply the patch 5. Observe the improved job types on http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 181867 [details] [review] Bug 39772: Extend background_tasks API to allow for human-readable job names Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Nice improvement, no regressions, Passing QA Pushed for 25.05! Well done everyone, thank you! in: ``` for my $plugin (@plugins) { my $tasks = $plugin->background_tasks; ``` probably we should have: ``` for my $plugin (@plugins) { next unless $plugin->can('background_tasks'); my $tasks = $plugin->background_tasks; ``` if plugin does not have background_tasks method. (In reply to Andrii Nugged from comment #15) > in: > > ``` > for my $plugin (@plugins) { > my $tasks = $plugin->background_tasks; > ``` > > probably we should have: > > ``` > for my $plugin (@plugins) { > next unless $plugin->can('background_tasks'); > my $tasks = $plugin->background_tasks; > ``` > > if plugin does not have background_tasks method. Strictly speaking, that shouldn't be necessary since only plugins that have registered a 'background_tasks' at install time will be in @plugins. It looks like this causes a regression. See bug 40033 for the fix. (In reply to David Cook from comment #16) > (In reply to Andrii Nugged from comment #15) > > in: > > > > ``` > > for my $plugin (@plugins) { > > my $tasks = $plugin->background_tasks; > > ``` > > > > probably we should have: > > > > ``` > > for my $plugin (@plugins) { > > next unless $plugin->can('background_tasks'); > > my $tasks = $plugin->background_tasks; > > ``` > > > > if plugin does not have background_tasks method. > > Strictly speaking, that shouldn't be necessary since only plugins that have > registered a 'background_tasks' at install time will be in @plugins. That is, @plugins wasn't being properly populated. But Tad has fixed it in the other bug report Pushed to 24.11.x for 24.11.06 Will be pushed to 24.05.x for 24.05.12 Not backporting to 22.11.x |