Description
Owen Leonard
2019-02-12 17:01:57 UTC
Created attachment 85132 [details] [review] Bug 22318: Extend Koha news feature to include other content areas This patch lays the groundwork for using the Koha news tool to replace OPAC system preferences which embed HTML content in the Koha interface. This will allow multiple translations of these content blocks to be created. This adds a new template plugin, KohaNews. It has a single subroutine, get(), which takes three parameters: - "location" -- the area of the template where the content will appear. This will correspond to the system preference it replaces: OpacMainUserBlock, OpacNavRight, etc. - "lang" -- the user's currently-selected language. - "library" -- the user's home library (if they are logged in) A new "koha_news_block" block is added to the OPAC templates' html_helpers include. The content area is activated in the template using this syntax: [% PROCESS koha_news_block news => KohaNews.get( location => "OpacNavRight", lang => news_lang, library => branchcode ) %] Created attachment 85133 [details] [review] Bug 22318: Example implementation: OpacNavRight system preference This patch implements a Koha news system replacement of the OpacNavRight system preference. The changes to koha-news.tt enable the selection of "OpacNavRight" as a location for the content. opac-main.pl is modified to ensure that the user's selected language is passed to the template for use by the KohaNews plugin. The database update process takes the contents of Koha's OpacNavRight system preference and adds it to the Koha news system. The OpacNavRight system preference is then removed from the database. When the new entry is added to Koha news, the language is set based on the top selected language in the "opaclanguages" preference. The entry is added to "All libraries" without an author, title, or expiration date. Note that this patch doesn't change the way OPAC news is displayed. I think it's best to keep existing markup and code for that since it handles details like branch selection and RSS. To test, apply the patch and run the database update. - In the staff client go to Tools -> News and confirm that there is now an entry with the location "OpacNavRight_en" (assuming an "en" English interface). - Confirm that the previous contents of OpacNavRight were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNavRight preference has been removed. - In the OPAC, confirm that the correct content is displayed in the region previously defined in the OpacNavRight system preference. - Test that the language and library-specific nature of news items is reflected in the behavior of this content area: - Define different text based on language and location. Confirm that switching translations and logging in to the OPAC both trigger the correct display of different content. One issue which these patches don't address is that if we move a system preference into the news system we lose the preference's description. That could probably be solved by adding some help information to the news entry template. ...Another issue up for discussion is how the news system should be "rebranded" if we're adding general content management as a component. This will require a follow-up for that. "Content management" may carry too strong an implication that you can create whole pages. Content areas? Content blocks? But wait there's more: Some content blocks in the OPAC affect the layout of the page in such a way that it's necessary to check for the existence of the content in the template. How should that be handled? Another plugin method for counting results? Hi, I tested patches on current master and get this errors: Falling back to patching base and 3-way merge... No changes -- Patch already applied. Applying: Bug 22318: Example implementation: OpacNavRight system preference Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt M opac/opac-main.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-main.pl CONFLICT (content): Merge conflict in opac/opac-main.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt Auto-merging installer/data/mysql/sysprefs.sql error: Failed to merge in the changes. Owen, can you rebase patches na master? Thank you Created attachment 86728 [details] [review] Bug 22318: Example implementation: OpacNavRight system preference This patch implements a Koha news system replacement of the OpacNavRight system preference. The changes to koha-news.tt enable the selection of "OpacNavRight" as a location for the content. opac-main.pl is modified to ensure that the user's selected language is passed to the template for use by the KohaNews plugin. The database update process takes the contents of Koha's OpacNavRight system preference and adds it to the Koha news system. The OpacNavRight system preference is then removed from the database. When the new entry is added to Koha news, the language is set based on the top selected language in the "opaclanguages" preference. The entry is added to "All libraries" without an author, title, or expiration date. Note that this patch doesn't change the way OPAC news is displayed. I think it's best to keep existing markup and code for that since it handles details like branch selection and RSS. To test, apply the patch and run the database update. - In the staff client go to Tools -> News and confirm that there is now an entry with the location "OpacNavRight_en" (assuming an "en" English interface). - Confirm that the previous contents of OpacNavRight were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNavRight preference has been removed. - In the OPAC, confirm that the correct content is displayed in the region previously defined in the OpacNavRight system preference. - Test that the language and library-specific nature of news items is reflected in the behavior of this content area: - Define different text based on language and location. Confirm that switching translations and logging in to the OPAC both trigger the correct display of different content. Created attachment 86730 [details] [review] Bug 22318: Extend Koha news feature to include other content areas This patch lays the groundwork for using the Koha news tool to replace OPAC system preferences which embed HTML content in the Koha interface. This will allow multiple translations of these content blocks to be created. This adds a new template plugin, KohaNews. It has a single subroutine, get(), which takes three parameters: - "location" -- the area of the template where the content will appear. This will correspond to the system preference it replaces: OpacMainUserBlock, OpacNavRight, etc. - "lang" -- the user's currently-selected language. - "library" -- the user's home library (if they are logged in) A new "koha_news_block" block is added to the OPAC templates' html_helpers include. The content area is activated in the template using this syntax: [% PROCESS koha_news_block news => KohaNews.get( location => "OpacNavRight", lang => news_lang, library => branchcode ) %] Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 86731 [details] [review] Bug 22318: Example implementation: OpacNavRight system preference This patch implements a Koha news system replacement of the OpacNavRight system preference. The changes to koha-news.tt enable the selection of "OpacNavRight" as a location for the content. opac-main.pl is modified to ensure that the user's selected language is passed to the template for use by the KohaNews plugin. The database update process takes the contents of Koha's OpacNavRight system preference and adds it to the Koha news system. The OpacNavRight system preference is then removed from the database. When the new entry is added to Koha news, the language is set based on the top selected language in the "opaclanguages" preference. The entry is added to "All libraries" without an author, title, or expiration date. Note that this patch doesn't change the way OPAC news is displayed. I think it's best to keep existing markup and code for that since it handles details like branch selection and RSS. To test, apply the patch and run the database update. - In the staff client go to Tools -> News and confirm that there is now an entry with the location "OpacNavRight_en" (assuming an "en" English interface). - Confirm that the previous contents of OpacNavRight were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNavRight preference has been removed. - In the OPAC, confirm that the correct content is displayed in the region previously defined in the OpacNavRight system preference. - Test that the language and library-specific nature of news items is reflected in the behavior of this content area: - Define different text based on language and location. Confirm that switching translations and logging in to the OPAC both trigger the correct display of different content. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 86733 [details] [review] Bug 22318: Extend Koha news feature to include other content areas This patch lays the groundwork for using the Koha news tool to replace OPAC system preferences which embed HTML content in the Koha interface. This will allow multiple translations of these content blocks to be created. This adds a new template plugin, KohaNews. It has a single subroutine, get(), which takes three parameters: - "location" -- the area of the template where the content will appear. This will correspond to the system preference it replaces: OpacMainUserBlock, OpacNavRight, etc. - "lang" -- the user's currently-selected language. - "library" -- the user's home library (if they are logged in) A new "koha_news_block" block is added to the OPAC templates' html_helpers include. The content area is activated in the template using this syntax: [% PROCESS koha_news_block news => KohaNews.get( location => "OpacNavRight", lang => news_lang, library => branchcode ) %] Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86734 [details] [review] Bug 22318: Example implementation: OpacNavRight system preference This patch implements a Koha news system replacement of the OpacNavRight system preference. The changes to koha-news.tt enable the selection of "OpacNavRight" as a location for the content. opac-main.pl is modified to ensure that the user's selected language is passed to the template for use by the KohaNews plugin. The database update process takes the contents of Koha's OpacNavRight system preference and adds it to the Koha news system. The OpacNavRight system preference is then removed from the database. When the new entry is added to Koha news, the language is set based on the top selected language in the "opaclanguages" preference. The entry is added to "All libraries" without an author, title, or expiration date. Note that this patch doesn't change the way OPAC news is displayed. I think it's best to keep existing markup and code for that since it handles details like branch selection and RSS. To test, apply the patch and run the database update. - In the staff client go to Tools -> News and confirm that there is now an entry with the location "OpacNavRight_en" (assuming an "en" English interface). - Confirm that the previous contents of OpacNavRight were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNavRight preference has been removed. - In the OPAC, confirm that the correct content is displayed in the region previously defined in the OpacNavRight system preference. - Test that the language and library-specific nature of news items is reflected in the behavior of this content area: - Define different text based on language and location. Confirm that switching translations and logging in to the OPAC both trigger the correct display of different content. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Sorry, I found one issue here: On news list page, the filter above datatable does not contain OpacNavRight location. Created attachment 86743 [details] [review] Bug 22318: (follow-up) Add new options to new table filter form This patch adds the new language/location options to the form for filtering entries in the table of news items. Since this markup is duplicated twice in the template, its contents have been moved to a BLOCK. To test, apply the patch and go to Tools -> News. At the top of the table of news items the "Display location" form should show all options including OpacNavRight for all your enabled languages. Confirm that location/language selection still works correctly upon adding and editing news items. Created attachment 86771 [details] [review] Bug 22318: (follow-up) Add new options to new table filter form This patch adds the new language/location options to the form for filtering entries in the table of news items. Since this markup is duplicated twice in the template, its contents have been moved to a BLOCK. To test, apply the patch and go to Tools -> News. At the top of the table of news items the "Display location" form should show all options including OpacNavRight for all your enabled languages. Confirm that location/language selection still works correctly upon adding and editing news items. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> (In reply to Owen Leonard from comment #14) > Created attachment 86743 [details] [review] [review] > Bug 22318: (follow-up) Add new options to new table filter form > Thanks for quick and nice follow-up Owen. Looking good. Passing I like this one, but I just want to make sure it has had enough eyes on it, can I get some more viewpoints on this one? Created attachment 87490 [details] [review] Bug 22318: Extend Koha news feature to include other content areas This patch lays the groundwork for using the Koha news tool to replace OPAC system preferences which embed HTML content in the Koha interface. This will allow multiple translations of these content blocks to be created. This adds a new template plugin, KohaNews. It has a single subroutine, get(), which takes three parameters: - "location" -- the area of the template where the content will appear. This will correspond to the system preference it replaces: OpacMainUserBlock, OpacNavRight, etc. - "lang" -- the user's currently-selected language. - "library" -- the user's home library (if they are logged in) A new "koha_news_block" block is added to the OPAC templates' html_helpers include. The content area is activated in the template using this syntax: [% PROCESS koha_news_block news => KohaNews.get( location => "OpacNavRight", lang => news_lang, library => branchcode ) %] Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87491 [details] [review] Bug 22318: Example implementation: OpacNavRight system preference This patch implements a Koha news system replacement of the OpacNavRight system preference. The changes to koha-news.tt enable the selection of "OpacNavRight" as a location for the content. opac-main.pl is modified to ensure that the user's selected language is passed to the template for use by the KohaNews plugin. The database update process takes the contents of Koha's OpacNavRight system preference and adds it to the Koha news system. The OpacNavRight system preference is then removed from the database. When the new entry is added to Koha news, the language is set based on the top selected language in the "opaclanguages" preference. The entry is added to "All libraries" without an author, title, or expiration date. Note that this patch doesn't change the way OPAC news is displayed. I think it's best to keep existing markup and code for that since it handles details like branch selection and RSS. To test, apply the patch and run the database update. - In the staff client go to Tools -> News and confirm that there is now an entry with the location "OpacNavRight_en" (assuming an "en" English interface). - Confirm that the previous contents of OpacNavRight were added correctly. - Go to Administration -> System preferences -> OPAC and verify that the OpacNavRight preference has been removed. - In the OPAC, confirm that the correct content is displayed in the region previously defined in the OpacNavRight system preference. - Test that the language and library-specific nature of news items is reflected in the behavior of this content area: - Define different text based on language and location. Confirm that switching translations and logging in to the OPAC both trigger the correct display of different content. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87492 [details] [review] Bug 22318: (follow-up) Add new options to new table filter form This patch adds the new language/location options to the form for filtering entries in the table of news items. Since this markup is duplicated twice in the template, its contents have been moved to a BLOCK. To test, apply the patch and go to Tools -> News. At the top of the table of news items the "Display location" form should show all options including OpacNavRight for all your enabled languages. Confirm that location/language selection still works correctly upon adding and editing news items. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87493 [details] [review] Bug 22318: (QA follow-up) Correct name of atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This is really nice.. I've given it a second QA run as Nick requested.. I agree with everything Owen has said, it would be nice to rebrand 'News' to 'Content' or something as we're extending it's use.. but that can be a followup. It would be also be nice to add additional help to show where a 'location' is displayed.. I'm thinking a diagram with blocks highlighted.. either in the manual or right there in the news form template (but again, this could be a followup) Generally I really like this and it's a great start that could lead on to bigger things. It would be great to have a tree of bugs to convert each of the relevant sysprefs to this system. Passed QA (Again) I took a quick look and I like it. 2 concerns maybe: The HTML editor of news editor is not very nice. For people now working with HTML and having complicated content in the prefs this will be a little harder to handle than before. I think once we move more prefs we should split display location into location and language, so the list is not as long. Having the location as 'group' and the languages below instead of the language as group could also help. The thing I dislike about the editor is that it has the modal for the HTML editor that requires you to 'update' and then 'save'. That adds a lot of steps for just seeing what you did. Not sure what we can do about it, but noting. Maybe we coudl add an option to work in plain text HTML not using the editor and Code Mirror highlighting instead? That's all for separate bugs - willing to file them later too if you agree :) I was secretly hoping a codemirror html editing mode might come as a followup for this.. I'm sure Owen was already working on using codemirror for the relevent sysprefs so it sort of follows they we could do that here too (again in a followup) I really like Code Mirror for SQL, I think it would be great to have it as an option here. Awesome work all! Pushed to master for 19.05 This is not picked up by the translation toolchain: koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt: [% SET location = "OPAC news" %] Just a note: there is bug 20307 that could have handled the translation part. or something like [% SET location = BLOCK %]OPAC news[% END %] would let the text be translatable while still being assigned into a variable. Great new enhancement, will not be backported to 18.11.x series. Moving the contents of OpacNavRight to opac_news fails due to improper (no?) escaping: DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'http':'https';if(!d.getElementById(id)){js=d.createElement(s);js.id=id;js.src=p+' at line 1 [for Statement "INSERT INTO opac_news (branchcode, lang, title, content ) VALUES (NULL, 'OpacNavRight_en', '', ' <a class="twitter-timeline" href="https://twitter.com/ETFLibrary" data-widget-id="702160486524186624">Tweets by @ETFLibrary</a> <script>!function(d,s,id){var js,fjs=d.getElementsByTagName(s)[0],p=/^http:/.test(d.location)?'http':'https';if(!d.getElementById(id)){js=d.createElement(s);js.id=id;js.src=p+"://platform.twitter.com/widgets.js";fjs.parentNode.insertBefore(js,fjs);}}(document,"script","twitter-wjs");</script> ')"] at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 18220. In this example, OpacNavRight contains a script to embed a Twitter timeline. (In reply to Kris Sinnaeve from comment #32) > Moving the contents of OpacNavRight to opac_news fails due to improper (no?) > escaping This issue belongs in a new bug report since this one has already been pushed to master: Bug 23093. *** Bug 11251 has been marked as a duplicate of this bug. *** While I agree about moving the HTML system preferences out of the system preferences, wouldn't it make sense to re-brand the "News" to "Customize user interface" or "HTML Content Editor" or something like that? I imagine putting things under the "News" banner might just confuse library staff more than they're already confused. Bug added: 25001 (In reply to Martin Renvoize from comment #36) > Bug added: 25001 Cheers, Martin. You're a legend :). *** Bug 8067 has been marked as a duplicate of this bug. *** |