Building on 22318, the OpacNav system preference should be converted to a Koha news block.
Created attachment 104362 [details] [review] Bug 23223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation.
Created attachment 104364 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation.
This patch has a couple of issues I know of which I could use help with: - It doesn't work on the reading history page and I don't know why. The "lang" and "branchcode" template variables on that page seem to be wrong. - There are warnings from the QA tool which I think are false positives but I'm not sure.
Created attachment 104453 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation.
Everything works well here for me, I test with translation and all is good. When I run the QA tool I do get this failure: FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/suggestion.tt OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid Bareword found where operator expected Bareword found where operator expected (Missing operator before KohaNews?) Bareword found where operator expected (Missing operator before OpacNav?) Unquoted string "lang" may clash with future reserved word Unquoted string "branchcode" may clash with future reserved word Bareword found where operator expected Bareword found where operator expected Bareword found where operator expected Bareword found where operator expected
Created attachment 109856 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation.
Created attachment 110843 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com>
Hi Owen. The only thing I noticed when testing was for this step: - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. The content appears correctly under Tools > News. In the list it doesn't include the publication date and title. It doesn't appear on the OPAC until you edit the item(s) (the current date is added) and add a title. Once I added this information and saved it, then it appeared in the OPAC in the expected places: - OPAC Home page - Authority search - Tag cloud - Libraries - Logged in account (before your summary) I'm not sure whether this is the expected behavour or not. David
Created attachment 110853 [details] [review] Bug 24223: (follow-up) Add date to OpacNav news entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content.
Created attachment 110897 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 110898 [details] [review] Bug 24223: (follow-up) Add date to OpacNav news entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com>
Hi Owen, can you please check this one? FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/suggestion.tt FAIL valid Bareword found where operator expected Bareword found where operator expected (Missing operator before KohaNews?) Bareword found where operator expected (Missing operator before OpacNav?) Unquoted string "lang" may clash with future reserved word Unquoted string "branchcode" may clash with future reserved word Bareword found where operator expected Bareword found where operator expected Bareword found where operator expected
The functionality of the svc/suggestion.tt template seems to work. If I enable the AuthorityFile "Did you mean" plugin I get valid results. I wonder if the QA tool expects file ins /svc/* to be something other than template files?
(In reply to Owen Leonard from comment #13) > I wonder if the QA tool expects file ins /svc/* to be something other than > template files? Yes, it's a "bug" in qa-test-tools. Files in a path contained "/svc/" are considered perl files. You can ignore it.
(In reply to Jonathan Druart from comment #14) > (In reply to Owen Leonard from comment #13) > > I wonder if the QA tool expects file ins /svc/* to be something other than > > template files? > > Yes, it's a "bug" in qa-test-tools. Files in a path contained "/svc/" are > considered perl files. You can ignore it. See https://gitlab.com/koha-community/qa-test-tools/-/issues/43
Created attachment 113130 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 113131 [details] [review] Bug 24223: (follow-up) Add date to OpacNav news entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 113132 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav news item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display.
The patch no longer applies 8-(
Created attachment 115758 [details] [review] Bug 24223: Convert OpacNav system preference to news block This patch builds on Bug 22318 to move the OpacNav system preference into the Koha news system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> News and verify that the content from OpacMainUserBlock is now stored in news items. There should be one entry for each of the enabled translations in your system, for instance 'OpacNav_en', 'OpacNav_fr-FR', 'OpacNav_cs-CZ' - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 115759 [details] [review] Bug 24223: (follow-up) Add date to OpacNav news entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 115760 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav news item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display.
Patch no longer applies 8-(..
Created attachment 124935 [details] [review] Bug 24223: Convert OpacNav system preference to additional content This patch builds on Bug 22318 to move the OpacNav system preference into the "Additional contents" system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> HTML customizations and verify that the content from OpacNav is now stored there. When you edit the entry you should see content for each of the enabled translations in your system under each tab. - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124936 [details] [review] Bug 24223: (follow-up) Add date to OpacNav additional content entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124937 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav additional contents item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display.
Created attachment 124938 [details] [review] Bug 24223: (follow-up) Updates to conform to new practices - The patch now works with new "Additional contents" system - The database update follows new skeleton.pl - Add "OpacNav" option to the additional contents template
Created attachment 125300 [details] [review] Bug 24223: Convert OpacNav system preference to additional content This patch builds on Bug 22318 to move the OpacNav system preference into the "Additional contents" system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> HTML customizations and verify that the content from OpacNav is now stored there. When you edit the entry you should see content for each of the enabled translations in your system under each tab. - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 125301 [details] [review] Bug 24223: (follow-up) Add date to OpacNav additional content entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 125302 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav additional contents item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display. Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 125303 [details] [review] Bug 24223: (follow-up) Updates to conform to new practices - The patch now works with new "Additional contents" system - The database update follows new skeleton.pl - Add "OpacNav" option to the additional contents template Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 125304 [details] [review] Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc Id attribut will be defined by 'koha_news_block'.
I signed existing patches. But also added one : Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc Like in Bug 28927, no need anymore to create a div with id 'opacnav' : it will generate a div with id="opacnav" and one inside with id="OpacNav".
Created attachment 125767 [details] [review] Bug 24223: Convert OpacNav system preference to additional content This patch builds on Bug 22318 to move the OpacNav system preference into the "Additional contents" system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> HTML customizations and verify that the content from OpacNav is now stored there. When you edit the entry you should see content for each of the enabled translations in your system under each tab. - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125768 [details] [review] Bug 24223: (follow-up) Add date to OpacNav additional content entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125769 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav additional contents item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display.
Created attachment 125770 [details] [review] Bug 24223: (follow-up) Updates to conform to new practices - The patch now works with new "Additional contents" system - The database update follows new skeleton.pl - Add "OpacNav" option to the additional contents template
Created attachment 125771 [details] [review] Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc Id attribut will be defined by 'koha_news_block'. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
*** Bug 28650 has been marked as a duplicate of this bug. ***
Skeleton should include a return of a hash like: return { bug_number => "BUG_NUMBER", description => "A single line description", up => sub { my ($args) = @_; my ($dbh, $out) = @$args{qw(dbh out)}; # Do you stuffs here $dbh->do(q{}); # Print useful stuff here say $out "Update is going well so far"; }, } You do not return a hash, but you start a block with { }
s/skeleton/atomic update/
Explaining: FAIL valid Useless use of anonymous hash ({}) in void context
No blocker, but you might wanna take a look at: Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt line 264. Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 300. Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 303. Processing files after patches Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt line 266. Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 302. Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 305.
(In reply to Marcel de Rooy from comment #40) > You do not return a hash, but you start a block with Hmm. At least it is not right :)
Created attachment 126089 [details] [review] Bug 24223: (follow-up) Add missing "return" to atomicupdate
(In reply to Marcel de Rooy from comment #43) > No blocker, but you might wanna take a look at: > > Argument "" isn't numeric in numeric gt (>) at > /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt > line 264. > Argument "" isn't numeric in numeric gt (>) at > /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt > line 300. > Argument "" isn't numeric in numeric gt (>) at > /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt > line 303. I don't think this is related to the changes made by my patch.
(In reply to Owen Leonard from comment #46) > I don't think this is related to the changes made by my patch. Confirmed, warnings appear before the patches are applied as well.
Created attachment 126147 [details] [review] Bug 24223: Get languages for OPAC There were 2 warnings Use of uninitialized value $interface in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121. Use of uninitialized value $theme in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121.
Coming back here soon
QAing
Opened bug 29245 for improving getTranslatedLanguages
This pref description needs attention: Show the following HTML on the left hand column of the main page and patron account on the OPAC, after OpacNav, and before patron account links if available:
This is weird: Dates got mixed up somewhere. I have 2026-08-01 in additional_contents.published_on.
The installer only created a default entry for me. Why didnt he add english too?
The following observations are broader than the scope of this report. But I am mentioning them here anyhow: Listview shows: See HTML customizations. This 'See' is confusing. The toggle between news and contents could be improved imo. List view on tools/additional-contents.pl: Shows me one title, altough I have titles per language. Display location on tools/additional-contents.pl: Bit strange to see lc and uc here like OpacNav and opacheader. ArticleRequestsDisclaimerText at the end. We should sort this list. Sometimes it seems like the Contents textarea only expands when it gets focus (even with a very small delay or so)? Going back from an additional contents item, I should be returning to additional contents, not news.
(In reply to Marcel de Rooy from comment #55) > The following observations are broader than the scope of this report. But I > am mentioning them here anyhow: > > Listview shows: See HTML customizations. This 'See' is confusing. The toggle > between news and contents could be improved imo. How? What do you suggest? > List view on tools/additional-contents.pl: Shows me one title, altough I > have titles per language. I don't understand, can you post a screenshot? > Display location on tools/additional-contents.pl: Bit strange to see lc and > uc here like OpacNav and opacheader. ArticleRequestsDisclaimerText at the > end. We should sort this list. Yes, sorted would be good. The uc/lc mismatch is because of the original syspref's names. Ideally we should rename them but keep the code in parenthesis (?) > Sometimes it seems like the Contents textarea only expands when it gets > focus (even with a very small delay or so)? Yes, it's not nice. I think it's a bug in tinymce, or we are doing something wrong with our css. > Going back from an additional contents item, I should be returning to > additional contents, not news. Indeed, an that's an easy to fix. Please report on a separate bug report and link it with bug 24387.
(In reply to Marcel de Rooy from comment #54) > The installer only created a default entry for me. Why didnt he add english > too? I think adding only the default would make sense to me. This way you can edit one entry to affect all your installed languages for a start and we can't tell which language people want to start in. Maybe I am missing something?
(In reply to Marcel de Rooy from comment #53) > This is weird: > Dates got mixed up somewhere. I have 2026-08-01 in > additional_contents.published_on. See bug 29254
(In reply to Jonathan Druart from comment #56) > (In reply to Marcel de Rooy from comment #55) > > Going back from an additional contents item, I should be returning to > > additional contents, not news. > > Indeed, an that's an easy to fix. > > > Please report on a separate bug report and link it with bug 24387. Bug 29263
(In reply to Katrin Fischer from comment #57) > I think adding only the default would make sense to me. For what it's worth, the idea behind adding to all available languages is to ensure that the content doesn't disappear. Before this patch, anything in OpacNav is displayed for all languages. After this patch OpacNav might not appear for everyone.
(In reply to Owen Leonard from comment #60) > (In reply to Katrin Fischer from comment #57) > > > I think adding only the default would make sense to me. > > For what it's worth, the idea behind adding to all available languages is to > ensure that the content doesn't disappear. Before this patch, anything in > OpacNav is displayed for all languages. After this patch OpacNav might not > appear for everyone. Maybe I am misunderstanding - I thought the new default option would always show up for all available languages if no other language content was set and that would allow us to migrate them without having to add all the entries.
If it's "en" it must be the default. If it has a specific language prior the upgrade we should use it and copy the content for default (so two entries in the table). It's what we did on bug 22544.
(In reply to Jonathan Druart from comment #62) > If it's "en" it must be the default. If it has a specific language prior the > upgrade we should use it and copy the content for default (so two entries in > the table). It's what we did on bug 22544. Can you explain that? Why must en be default and a specific entry be copied? If we only had default, woudl it not display for all installed languages?
(In reply to Katrin Fischer from comment #63) > (In reply to Jonathan Druart from comment #62) > > If it's "en" it must be the default. If it has a specific language prior the > > upgrade we should use it and copy the content for default (so two entries in > > the table). It's what we did on bug 22544. > > Can you explain that? > > Why must en be default and a specific entry be copied? If we only had > default, woudl it not display for all installed languages? Sorry, it was too early. I was referring to the upgrade from bug 24387, where we moved data from the old table structure.
So yes, only 1 entry as "default" would make sense to me as well.
(In reply to Jonathan Druart from comment #65) > So yes, only 1 entry as "default" would make sense to me as well. Well, no changes needed for that then.
Resuming QA now.
(In reply to Marcel de Rooy from comment #66) > (In reply to Jonathan Druart from comment #65) > > So yes, only 1 entry as "default" would make sense to me as well. > > Well, no changes needed for that then. Actually yes, we need to only insert default. So update atomic update. Will do.
Created attachment 126484 [details] [review] Bug 24223: Convert OpacNav system preference to additional content This patch builds on Bug 22318 to move the OpacNav system preference into the "Additional contents" system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> HTML customizations and verify that the content from OpacNav is now stored there. When you edit the entry you should see content for each of the enabled translations in your system under each tab. - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126485 [details] [review] Bug 24223: (follow-up) Add date to OpacNav additional content entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126486 [details] [review] Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav additional contents item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126487 [details] [review] Bug 24223: (follow-up) Updates to conform to new practices - The patch now works with new "Additional contents" system - The database update follows new skeleton.pl - Add "OpacNav" option to the additional contents template Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126488 [details] [review] Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc Id attribut will be defined by 'koha_news_block'. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126489 [details] [review] Bug 24223: (follow-up) Add missing "return" to atomicupdate Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126490 [details] [review] Bug 24223: Get languages for OPAC There were 2 warnings Use of uninitialized value $interface in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121. Use of uninitialized value $theme in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126491 [details] [review] Bug 24223: (QA follow-up) Do not copy to other languages By majority on Bugzilla, we dont want to have the existing text in all translatable languages. Note: Removing the news_123 as code here, switching to OpacNav now. Another report should handle/improve the use of code, assuming that it is meant to provide some form of grouping. There is now confusion between the fields code and location. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126492 [details] [review] Bug 24223: (QA follow-up) Link OpacNav to HTML customizations now In the pref description of OpacNavBottom. (Before obsoleting that one too somewhere soon.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #56) > (In reply to Marcel de Rooy from comment #55) Coming back to open questions: > > The following observations are broader than the scope of this report. But I > > am mentioning them here anyhow: > > > > Listview shows: See HTML customizations. This 'See' is confusing. The toggle > > between news and contents could be improved imo. > > How? What do you suggest? Perhaps just a See also would be already better. Or Switch to HTML. Now it reads funny: News see HTML. Which might confuse. It is either News or HTML. But no big deal. We all get used to things. > > List view on tools/additional-contents.pl: Shows me one title, altough I > > have titles per language. > > I don't understand, can you post a screenshot? This is not something hard to catch. The title in the listview (the main HTML page with 'all' entries) shows me the title of the default entry. But title is no single value anymore. I have added an English entry and could have another title.
Some patches will be squashed when pushed.
1. I may be missing something but I don't understand why we are setting OpacNav from the .tt when we need it in .inc. Why don't we AdditionalContents.get from there? 2. opac/opac-readingrecord.pl -$template->param(%{$borr}); See bug 28768, we must fix it separately and on its own bug report.
(In reply to Jonathan Druart from comment #80) > 1. I may be missing something but I don't understand why we are setting > OpacNav from the .tt when we need it in .inc. Why don't we > AdditionalContents.get from there? Is there any best practice about the place of TT's [% USE %] directives? Note that we still didnt tackle the load aborted errors on template plugins where we also use includes..
(In reply to Marcel de Rooy from comment #81) > (In reply to Jonathan Druart from comment #80) > > 1. I may be missing something but I don't understand why we are setting > > OpacNav from the .tt when we need it in .inc. Why don't we > > AdditionalContents.get from there? > > Is there any best practice about the place of TT's [% USE %] directives? > Note that we still didnt tackle the load aborted errors on template plugins > where we also use includes.. For me it's: use it from where you need it.
Created attachment 127338 [details] [review] Bug 24223: Fix borrowernumber from opac-readingrecord.tt
Created attachment 127343 [details] [review] Bug 24223: Convert OpacNav system preference to additional content This patch builds on Bug 22318 to move the OpacNav system preference into the "Additional contents" system, making it possible to have language- and library-specific content. To test you should have some content in the OpacNav system preference. To make testing easier it's also a good idea to have content in the OpacNavBottom preference. - Apply the patch and run the database update process. - In the staff client, go to Tools -> HTML customizations and verify that the content from OpacNav is now stored there. When you edit the entry you should see content for each of the enabled translations in your system under each tab. - Confirm that the previous contents of OpacNav were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNav preference has been removed. - Update and reinstall active translations (for instance fr-FR): - cd misc/translator/ - perl translate update fr-FR - perl translate install fr-FR - View the OPAC and confirm that the contents of the OpacNav system preference displays in the left-hand sidebar. - Test as many pages as possible to confirm that wherever the OpacNavBottom content is displayed, OpacNav is also displayed. - Test also using the updated translation. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24223: (follow-up) Add date to OpacNav additional content entries This follow-up modifies the atomic update so that it inserts today's date as the "Published on" date. Without that information the system considers the item unpublished and will not display the content. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24223: (follow-up) Make logged-in user's branchcode available to template This patch makes a couple of corrections for problems I discovered while working on Bug 24224: The variables required for correct display of news items by branch and language were not consistently available to the templates. In opac-readingrecord.pl, the "lang" variable was being overwritten. On this and other pages the logged-in patron's brachcode was not made available. I've corrected instances of KohaNews.get() to use the globally-available "logged_in_user.branchcode" instead. To test, apply the patch and follow the original test plan, with the addition of instances of the OpacNav additional contents item which have a specific library selected. Confirm that such items are correctly displayed according to the logged-in user's home library. If there is no logged-in user the library-specific OpacNav should not display. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24223: (follow-up) Updates to conform to new practices - The patch now works with new "Additional contents" system - The database update follows new skeleton.pl - Add "OpacNav" option to the additional contents template Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24223: (follow-up) Add missing "return" to atomicupdate Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24223: (QA follow-up) Link OpacNav to HTML customizations now In the pref description of OpacNavBottom. (Before obsoleting that one too somewhere soon.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127344 [details] [review] Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc Id attribut will be defined by 'koha_news_block'. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127345 [details] [review] Bug 24223: Get languages for OPAC There were 2 warnings Use of uninitialized value $interface in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121. Use of uninitialized value $theme in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127346 [details] [review] Bug 24223: (QA follow-up) Do not copy to other languages By majority on Bugzilla, we dont want to have the existing text in all translatable languages. Note: Removing the news_123 as code here, switching to OpacNav now. Another report should handle/improve the use of code, assuming that it is meant to provide some form of grouping. There is now confusion between the fields code and location. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Patches rebased and some squashed.
(In reply to Marcel de Rooy from comment #81) > (In reply to Jonathan Druart from comment #80) > > 1. I may be missing something but I don't understand why we are setting > > OpacNav from the .tt when we need it in .inc. Why don't we > > AdditionalContents.get from there? > > Is there any best practice about the place of TT's [% USE %] directives? > Note that we still didnt tackle the load aborted errors on template plugins > where we also use includes.. The correct answer was: to avoid double fetch. We are actually doing: SET OpacNav = AdditionalContents.get [...] [% IF OpacNav %] [% INCLUDE navigation.inc %] [% END %] And reusing OpacNav from navigation.inc
Is that weird to have OpacNav without OpacNavBottom part of 21.11?
(In reply to Jonathan Druart from comment #90) > Is that weird to have OpacNav without OpacNavBottom part of 21.11? Well, we have OpacNavRight already.
(In reply to Jonathan Druart from comment #90) > Is that weird to have OpacNav without OpacNavBottom part of 21.11? Yes, at least a bit. But these refactoring operations cross release borders. So defendable.
Pushed to master for 21.11, thanks to everybody involved!