Bug 22318 - Extend Koha news feature to include other content areas
Summary: Extend Koha news feature to include other content areas
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 22544 22880 23093 23253 22783
  Show dependency treegraph
 
Reported: 2019-02-12 17:01 UTC by Owen Leonard
Modified: 2019-07-02 13:47 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement begins work to move system preferences that include displayed text to the 'News' module - this allows the user to define text in various languages and add ability for these preferences to show correctly in translated OPACs. Specifically, this patch set moves the 'OPACNavRight' preference into the 'News' module.
Version(s) released in:
19.05.00


Attachments
Bug 22318: Extend Koha news feature to include other content areas (4.05 KB, patch)
2019-02-14 16:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22318: Example implementation: OpacNavRight system preference (9.86 KB, patch)
2019-02-14 16:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22318: Example implementation: OpacNavRight system preference (9.83 KB, patch)
2019-03-18 18:26 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22318: Extend Koha news feature to include other content areas (4.16 KB, patch)
2019-03-18 21:07 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22318: Example implementation: OpacNavRight system preference (9.96 KB, patch)
2019-03-18 21:07 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22318: Extend Koha news feature to include other content areas (4.22 KB, patch)
2019-03-19 08:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22318: Example implementation: OpacNavRight system preference (9.97 KB, patch)
2019-03-19 08:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22318: (follow-up) Add new options to new table filter form (6.50 KB, patch)
2019-03-19 14:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22318: (follow-up) Add new options to new table filter form (6.56 KB, patch)
2019-03-20 07:42 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22318: Extend Koha news feature to include other content areas (4.27 KB, patch)
2019-04-08 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22318: Example implementation: OpacNavRight system preference (9.99 KB, patch)
2019-04-08 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22318: (follow-up) Add new options to new table filter form (6.62 KB, patch)
2019-04-08 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22318: (QA follow-up) Correct name of atomicupdate (932 bytes, patch)
2019-04-08 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2019-02-12 17:01:57 UTC
There are many content areas in Koha where we currently use a single system preference to define a block of HTML which is embedded in the OPAC or Staff Client. Using system preferences for this requires that we either use only one language for the content or we use a convoluted system of CSS hiding/showing based on language.

We already have a system in place for handling this better: The Koha news feature. We could add additional "display locations" for the various system preferences which could be migrated here: OpacNavRight, opacheader, OpacMainUserBlock, OPACMySummaryNote, OpacNav, OpacNavBottom, etc.

This would allow libraries to define both language-specific and library-specific content for these areas of the interface. Being able to define start and end dates for content display would be really useful too.

I imagine it would be inefficient to add multiple lines to, for instance, opac-main.pl for each "location," like so:

my $OpacNavRight = GetNewsToDisplay( "OpacNavRight_".$news_lang, $homebranch );

Perhaps a template plugin and a block like this?

[% PROCESS koha_news news => GetKohaNews( location => news_lang, library => homebranch ) %]

I'd love to hear what y'all think.
Comment 1 Owen Leonard 2019-02-14 16:55:49 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2019-02-14 16:55:52 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2019-02-14 16:58:34 UTC
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.
Comment 4 Owen Leonard 2019-02-14 17:00:58 UTC
...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?
Comment 5 Owen Leonard 2019-02-15 18:51:51 UTC
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?
Comment 6 Michal Denar 2019-03-15 14:10:49 UTC
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
Comment 7 Owen Leonard 2019-03-18 18:26:38 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2019-03-18 18:27:42 UTC
Rebased on top of Bug 14272. Thanks for testing!
Comment 9 Michal Denar 2019-03-18 21:07:40 UTC Comment hidden (obsolete)
Comment 10 Michal Denar 2019-03-18 21:07:43 UTC Comment hidden (obsolete)
Comment 11 Josef Moravec 2019-03-19 08:29:55 UTC Comment hidden (obsolete)
Comment 12 Josef Moravec 2019-03-19 08:29:58 UTC Comment hidden (obsolete)
Comment 13 Josef Moravec 2019-03-19 10:20:17 UTC
Sorry, I found one issue here:

On news list page, the filter above datatable does not contain OpacNavRight location.
Comment 14 Owen Leonard 2019-03-19 14:04:00 UTC Comment hidden (obsolete)
Comment 15 Josef Moravec 2019-03-20 07:42:07 UTC Comment hidden (obsolete)
Comment 16 Josef Moravec 2019-03-20 07:43:28 UTC
(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
Comment 17 Nick Clemens 2019-03-28 17:49:13 UTC
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?
Comment 18 Martin Renvoize 2019-04-08 09:30:24 UTC
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>
Comment 19 Martin Renvoize 2019-04-08 09:30:27 UTC
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>
Comment 20 Martin Renvoize 2019-04-08 09:30:31 UTC
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>
Comment 21 Martin Renvoize 2019-04-08 09:30:34 UTC
Created attachment 87493 [details] [review]
Bug 22318: (QA follow-up) Correct name of atomicupdate

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-04-08 10:15:46 UTC
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)
Comment 23 Katrin Fischer 2019-04-08 17:24:56 UTC
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.
Comment 24 Katrin Fischer 2019-04-08 17:27:26 UTC
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 :)
Comment 25 Martin Renvoize 2019-04-08 17:31:48 UTC
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)
Comment 26 Katrin Fischer 2019-04-08 18:21:11 UTC
I really like Code Mirror for SQL, I think it would be great to have it as an option here.
Comment 27 Nick Clemens 2019-04-18 12:07:36 UTC
Awesome work all!

Pushed to master for 19.05
Comment 28 paxed 2019-04-23 06:54:40 UTC
This is not picked up by the translation toolchain:

koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt:                    [% SET location = "OPAC news" %]
Comment 29 Jonathan Druart 2019-04-24 20:16:40 UTC
Just a note: there is bug 20307 that could have handled the translation part.
Comment 30 paxed 2019-04-25 04:04:32 UTC
or something like

[% SET location = BLOCK %]OPAC news[% END %]

would let the text be translatable while still being assigned into a variable.
Comment 31 Martin Renvoize 2019-04-26 13:47:37 UTC
Great new enhancement, will not be backported to 18.11.x series.
Comment 32 Kris Sinnaeve 2019-06-11 09:35:54 UTC
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.
Comment 33 Owen Leonard 2019-06-11 12:40:18 UTC
(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.