Bug 34307 - Update plugin wrapper to use template wrapper for breadcrumbs
Summary: Update plugin wrapper to use template wrapper for breadcrumbs
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: David Cook
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on: 31339
Blocks:
  Show dependency treegraph
 
Reported: 2023-07-18 23:20 UTC by David Cook
Modified: 2023-08-31 10:36 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04


Attachments
Bug 34307: Update plugin template to match master conventions (3.64 KB, patch)
2023-07-19 05:37 UTC, David Cook
Details | Diff | Splinter Review
test plugin (2.00 KB, application/zip)
2023-07-19 05:37 UTC, David Cook
Details
Bug 34307: Update plugin template to match master conventions (3.69 KB, patch)
2023-08-09 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 34307: Update plugin template to match master conventions (3.74 KB, patch)
2023-08-20 02:30 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34307: (QA follow-up) add <span> to have cleaner strings for i18n (1.21 KB, patch)
2023-08-20 02:30 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34307: (QA follow-up) add <span> to have cleaner strings for i18n (1.26 KB, patch)
2023-08-21 12:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34307: (follow-up) Use template wrapper for title tag (2.15 KB, patch)
2023-08-21 12:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34307: (follow-up) Use template wrapper for title tag (2.21 KB, patch)
2023-08-21 15:23 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-07-18 23:20:24 UTC
The staff side wrapper template for tool plugins needs an update to create breadcrumbs using the template wrapper for breadcrumbs.
Comment 1 David Cook 2023-07-19 05:37:16 UTC
Created attachment 153623 [details] [review]
Bug 34307: Update plugin template to match master conventions

This change uses WRAPPER to build the breadcrumbs and
updates the plugin template to follow some other master conventions
like differentiating between Administration and Tools for plugins.

Test plan:
0. Apply patch
1. Upload koha-plugin-test-wrapper
2. Enable the plugin
3. Click "Actions" and click "Run tool"
4. Note how the plugin page looks like a perfect Koha Tools page
5. Note that the plugin only contains 6 lines of template code
to achieve this effect
Comment 2 David Cook 2023-07-19 05:37:38 UTC
Created attachment 153624 [details]
test plugin
Comment 3 David Nind 2023-08-09 00:30:30 UTC
Created attachment 154324 [details] [review]
Bug 34307: Update plugin template to match master conventions

This change uses WRAPPER to build the breadcrumbs and
updates the plugin template to follow some other master conventions
like differentiating between Administration and Tools for plugins.

Test plan:
0. Apply patch
1. Upload koha-plugin-test-wrapper
2. Enable the plugin
3. Click "Actions" and click "Run tool"
4. Note how the plugin page looks like a perfect Koha Tools page
5. Note that the plugin only contains 6 lines of template code
to achieve this effect

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Victor Grousset/tuxayo 2023-08-20 01:38:55 UTC
> 4. Note how the plugin page looks like a perfect Koha Tools page

Is there anything to do or check about the left menu? Or is that normal that plugins can't insert themselves there? Or maybe they can and it has nothing to do with wrapper-staff-tool-plugin.inc ?
Comment 5 Victor Grousset/tuxayo 2023-08-20 02:30:02 UTC
Created attachment 154673 [details] [review]
Bug 34307: Update plugin template to match master conventions

This change uses WRAPPER to build the breadcrumbs and
updates the plugin template to follow some other master conventions
like differentiating between Administration and Tools for plugins.

Test plan:
0. Apply patch
1. Upload koha-plugin-test-wrapper
2. Enable the plugin
3. Click "Actions" and click "Run tool"
4. Note how the plugin page looks like a perfect Koha Tools page
5. Note that the plugin only contains 6 lines of template code
to achieve this effect

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2023-08-20 02:30:04 UTC
Created attachment 154674 [details] [review]
Bug 34307: (QA follow-up) add <span> to have cleaner strings for i18n

Cleaner strings are extracted this way.
Comment 7 Victor Grousset/tuxayo 2023-08-20 02:34:11 UTC
It works! So second signoff. I've reached for help about reviewing the code.

follow-up to improve i18n

But there is something weird, there is a part I couldn't change to improve the extracted strings.

I did this:
```
<title>
    [% IF ( plugin_title ) %][% plugin_title | html %] &rsaquo; [% END %]
    <span>Plugins</span> &rsaquo;
    [% IF ( method == 'tool' ) %]
        <span>Tools</span>
    [% ELSE %]
        <span>Administration</span>
    [% END %]
    &rsaquo; <span>Koha</span>
 </title>
```

and got this as a diff of the translation files
```
misc/translator/Koha-staff-prog.pot
+msgid "%s%s &rsaquo; %s <span>Plugins</span> &rsaquo; %s <span>Tools</span> %s <span>Administration</span> %s &rsaquo; <span>Koha</span> "


misc/translator/po/fr-FR-staff-prog.po
+"%s%s &rsaquo; %s <span>Plugins</span> &rsaquo; %s <span>Tools</span> %s "
+"<span>Administration</span> %s &rsaquo; <span>Koha</span> "
+msgstr "Plugins &rsaquo; Outils &rsaquo; Koha "
```

But I see in the diff that I have individual occurrences of Plugins, Tools, Administration, Koha linked with wrapper-staff-tool-plugin.inc:7

So it's liked it worked, and didn't work at the same time. I checked and the bad occurences aren't in the commented out section of the .po.
And they contain the <span> so it's not the remains of the previous version.

Any hint?
Comment 8 David Cook 2023-08-20 22:59:07 UTC
(In reply to Victor Grousset/tuxayo from comment #4)
> > 4. Note how the plugin page looks like a perfect Koha Tools page
> 
> Is there anything to do or check about the left menu? Or is that normal that
> plugins can't insert themselves there? Or maybe they can and it has nothing
> to do with wrapper-staff-tool-plugin.inc ?

Doesn't really have anything to do with the wrapper.
Comment 9 David Cook 2023-08-20 23:00:12 UTC
(In reply to Victor Grousset/tuxayo from comment #7)
> But I see in the diff that I have individual occurrences of Plugins, Tools,
> Administration, Koha linked with wrapper-staff-tool-plugin.inc:7
> 
> So it's liked it worked, and didn't work at the same time. I checked and the
> bad occurences aren't in the commented out section of the .po.
> And they contain the <span> so it's not the remains of the previous version.
> 
> Any hint?

I have no idea about translations. I just followed the pattern that I saw.
Comment 10 Owen Leonard 2023-08-21 12:40:05 UTC
Created attachment 154678 [details] [review]
Bug 34307: (QA follow-up) add <span> to have cleaner strings for i18n

Cleaner strings are extracted this way.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2023-08-21 12:40:18 UTC
Created attachment 154679 [details] [review]
Bug 34307: (follow-up) Use template wrapper for title tag

This patch updates the title tag to use a wrapper too, since Bug 33906
is in the process of getting fixed.

The patch also removes an extra <span> tag: Only the translatable string
needs the span, not the entire line.
Comment 12 Victor Grousset/tuxayo 2023-08-21 15:23:50 UTC
Created attachment 154683 [details] [review]
Bug 34307: (follow-up) Use template wrapper for title tag

This patch updates the title tag to use a wrapper too, since Bug 33906
is in the process of getting fixed.

The patch also removes an extra <span> tag: Only the translatable string
needs the span, not the entire line.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Victor Grousset/tuxayo 2023-08-21 15:30:48 UTC
i18n is nice now, thanks Owen for the follow-up :D

And thanks for confirming that the code is ok and the adjustments.

So we are good to go: it works, makes sense, QA script happy, code looks good, passing QA :)
Comment 14 Tomás Cohen Arazi 2023-08-30 12:03:59 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 15 Fridolin Somers 2023-08-30 20:13:57 UTC
Pushed to 23.05.x for 23.05.04
Comment 16 Matt Blenkinsop 2023-08-31 10:36:22 UTC
Dependency missing in 22.11.x - not backporting

Nice work everyone!