Bug 22880 - Convert opacheader system preference to news block
Summary: Convert opacheader system preference to news block
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords: RM_priority
Depends on: 22318
Blocks: 23794 23795 23796 23797 24220 24221 24223 24224 24746 23798
  Show dependency treegraph
 
Reported: 2019-05-09 15:10 UTC by Owen Leonard
Modified: 2020-03-31 22:13 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 22880: Convert opacheader system preference to news block (8.50 KB, patch)
2019-05-09 16:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.28 KB, patch)
2019-06-11 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (8.62 KB, patch)
2019-08-14 23:53 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.34 KB, patch)
2019-08-14 23:54 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (7.76 KB, patch)
2019-09-04 17:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.34 KB, patch)
2019-09-04 17:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: (follow-up) Define OpacHeader variable for conditional (2.21 KB, patch)
2019-09-04 17:51 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (7.82 KB, patch)
2019-09-04 22:23 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.40 KB, patch)
2019-09-04 22:23 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22880: (follow-up) Define OpacHeader variable for conditional (2.27 KB, patch)
2019-09-04 22:24 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22880: (follow-up) Use global language variable for news language (3.51 KB, patch)
2019-12-11 13:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc. (12.62 KB, patch)
2019-12-11 15:42 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22880: (follow-up) Process all installed languages, not just enabled (1.62 KB, patch)
2019-12-12 13:33 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24366: Improve hashref construction (928 bytes, patch)
2020-02-12 19:56 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (928 bytes, patch)
2020-02-12 19:57 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (7.86 KB, patch)
2020-02-14 17:36 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc. (12.66 KB, patch)
2020-02-14 17:37 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: (follow-up) Define OpacHeader variable for conditional (2.33 KB, patch)
2020-02-14 17:38 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.46 KB, patch)
2020-02-14 17:38 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: (follow-up) Process all installed languages, not just enabled (1.67 KB, patch)
2020-02-14 17:38 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (7.90 KB, patch)
2020-02-21 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc. (12.56 KB, patch)
2020-02-21 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Define OpacHeader variable for conditional (2.48 KB, patch)
2020-02-21 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.55 KB, patch)
2020-02-21 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Process all installed languages, not just enabled (1.71 KB, patch)
2020-02-21 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (QA follow-up) Fix "koha_news_block: not found" (1.11 KB, patch)
2020-02-21 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: Convert opacheader system preference to news block (7.96 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc. (12.62 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Define OpacHeader variable for conditional (2.54 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: Use placeholders (1.61 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (follow-up) Process all installed languages, not just enabled (1.77 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (QA follow-up) Fix "koha_news_block: not found" (1.17 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (QA follow-up) Remove opacheader from C4::Auth (1.88 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (QA follow-up) Pass lang instead of news_lang in masthead (1.24 KB, patch)
2020-02-24 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22880: (QA follow-up) Replace use by require (1.25 KB, patch)
2020-02-24 12:17 UTC, Marcel de Rooy
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-05-09 15:10:32 UTC
Building on 22318, the opacheader system preference should be converted to a Koha news block.
Comment 1 Owen Leonard 2019-05-09 16:15:39 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2019-05-09 16:17:00 UTC
The QA tool is giving me a couple of errors which I could use some help understanding:

Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/html_helpers.inc line 23.

...and...

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
   OK     filters
   OK     forbidden patterns
   OK     git manipulation
   OK     js_in_body
   OK     spelling
   OK     tt_valid
   FAIL   valid_template
                koha_news_block: not found
Comment 3 Katrin Fischer 2019-05-11 23:14:25 UTC
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/html_helpers.inc line 23.
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/html_helpers.inc line 23.

This means that there are warnings with this script, but they are the same with and without your patch - so no blocker.

   FAIL	  valid_template
		koha_news_block: not found

I think this is a false positive. I think may the script doesn't manage the PROCESS and the BLOCK being in different files as it's here with the .inc
Comment 4 Jonathan Druart 2019-05-12 16:11:21 UTC
(In reply to Katrin Fischer from comment #3)
> Argument "" isn't numeric in numeric gt (>) at
> /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/
> html_helpers.inc line 23.
> Argument "" isn't numeric in numeric gt (>) at
> /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/
> html_helpers.inc line 23.
> 
> This means that there are warnings with this script, but they are the same
> with and without your patch - so no blocker.
> 
>    FAIL	  valid_template
> 		koha_news_block: not found
> 
> I think this is a false positive. I think may the script doesn't manage the
> PROCESS and the BLOCK being in different files as it's here with the .inc

Yes it is a false positive. html_helpers.inc is processed in doc-head-close, so we are good.
Comment 5 Jonathan Druart 2019-05-12 16:13:31 UTC
However we should have the USE raw on top, in case the block is called from a file that does not have it yet.
Comment 6 Jonathan Druart 2019-06-11 14:31:43 UTC Comment hidden (obsolete)
Comment 7 Lucas Gass 2019-08-02 21:11:49 UTC
Some libraries have very customized CSS in the OPACHeader syspref and specific CSS to go along with. This has the potential to impact how that displays and could problems, I think. Which could be problematic.
Comment 8 Katrin Fischer 2019-08-02 21:52:25 UTC
(In reply to Lucas Gass from comment #7)
> Some libraries have very customized CSS in the OPACHeader syspref and
> specific CSS to go along with. This has the potential to impact how that
> displays and could problems, I think. Which could be problematic.

It should convert nicely, but the CSS might not be visible straight away unless you use the HTML editor. Do you have an example that could be problematic?
Comment 9 Lucas Gass 2019-08-02 22:43:50 UTC
It does display well, once I manually add the HTML from the opacheader syspref. For me it does not copy the value from the old syspref to the 'opacheader_en.' area.
Comment 10 Hayley Mapley 2019-08-14 23:53:51 UTC Comment hidden (obsolete)
Comment 11 Hayley Mapley 2019-08-14 23:54:12 UTC Comment hidden (obsolete)
Comment 12 Hayley Mapley 2019-08-14 23:57:56 UTC
Signed off as all works as expected.

One minor thing, unsure if you'd want to do a follow-up for this or not:
After applying the patch, the header had slightly less left margin/left padding.

Everything else looks good!
Comment 13 Owen Leonard 2019-09-04 17:50:56 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2019-09-04 17:50:59 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2019-09-04 17:51:03 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2019-09-04 17:51:57 UTC
Rebased!

(In reply to Hayley Mapley from comment #12)

> After applying the patch, the header had slightly less left margin/left
> padding.

My follow-up addresses this. Thanks for testing!
Comment 17 Hayley Mapley 2019-09-04 22:23:23 UTC
Created attachment 92600 [details] [review]
Bug 22880: Convert opacheader system preference to news block

This patch builds on Bug 22318 to move the opacheader system preference
into the Koha news system, making it possible to have language- and
library-specific content.

It extends the original patch by adding an option to the template plugin
specifying whether the content title should be shown (probably should be
a flag in opac_news). It also adds a wrapper div with the content
location as the ID (e.g. 'opacheader'). This will make it slightly more
backwards-compatible with CSS customizations.

To test you should have some content in the opacheader system
preference. Apply the patch and run the database update process.

 - Go to the OPAC and confirm that the content which was previously in
   the opacheader system preference now displays correctly where it was
   before.
 - In the staff client, go to Tools -> News and verify that the content
   from opacheader is now stored in a news item with the location
   'opacheader_en.'
 - Go to Administration -> System preferences and confirm that the
   opacheader preference has been removed.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 18 Hayley Mapley 2019-09-04 22:23:48 UTC
Created attachment 92601 [details] [review]
Bug 22880: Use placeholders

Always.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 19 Hayley Mapley 2019-09-04 22:24:19 UTC
Created attachment 92602 [details] [review]
Bug 22880: (follow-up) Define OpacHeader variable for conditional

This follow-up uses the updates introduced by Bug 23253 as a model for
improving the display of opacheader. Adding the OpacHeader variable
allows the template to check for its existence and display some
container markup around opacheader if it exists. This will help ease the
transition for libraries who depend on the #opacheader id for styling.

To test, apply the patch and test the OPAC with an 'opacheader' item
defined. The content should be displayed on the page inside '<div
id="opacheader"></div>.'

Delete the 'opacheader' news item and reload the OPAC page. There should
no longer be an #opacheader div.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 20 Marcel de Rooy 2019-10-11 05:48:08 UTC
QAing
Comment 21 Marcel de Rooy 2019-10-11 05:58:08 UTC
Problem with required Title field in Tools/News. Leaving the title blank somehow is not a good idea. You can't edit the record now.

Opacheader shows more text than anticipated: I had this in the pref <h4>TEST</h4> But the contents of another news item (location All) is shown too. Probably not wanted.
Comment 22 Owen Leonard 2019-12-11 13:47:07 UTC Comment hidden (obsolete)
Comment 23 Owen Leonard 2019-12-11 15:42:48 UTC
Created attachment 96210 [details] [review]
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc.

This patch makes a number of improvements and ties up some loose ends
where the old system preference usage wasn't getting replaced.

 - Removes news-specific code for defining the language of news
   items queried for the OPAC home page. An identical language variable
   is already defined globally.

   Previous to this patch an 'opacheader' news item would only appear on
   the OPAC home page. Now it should appear on all OPAC pages.

 - Changes the database update so that 'opacheader' news
   items will be inserted with a default title, matching the interface's
   requirement that the title field be populated.

 - The database update will also now insert the old opacheader system
   preference contents into the news item for all active languages as
   defined in the 'opaclanguages' system preference. This helps match
   the previous behavior in which the opacheader contents were the same
   for all languages.

 - Adds support for the new opacheader news item to the self checkout,
   self check-in, and OPAC maintenance pages.

 - Updates sysprefs.t which was using the opacheader preference to test
   on. I've changed it to use URLLinkText instead.

 - Removes the addition of the opacheader system preference from the
   installation SQL file.
Comment 24 Owen Leonard 2019-12-11 15:47:24 UTC
(In reply to Marcel de Rooy from comment #21)
> Leaving the title blank somehow is not a good idea.

The revised db update fixes this by inserting some default text in the title (which isn't displayed anyway).


> Opacheader shows more text than anticipated: I had this in the pref
> <h4>TEST</h4> But the contents of another news item (location All) is shown
> too. Probably not wanted.

I can't reproduce this problem. Can you please test again?
Comment 25 Owen Leonard 2019-12-11 16:37:21 UTC
> > Opacheader shows more text than anticipated: I had this in the pref
> > <h4>TEST</h4> But the contents of another news item (location All) is shown
> > too.

I think I've discovered at least one way this can happen: My opaclanguages preference contained a language which wasn't actually installed and enabled. The DB update trusted the value for opaclanguages and inserted a news item using that location.

Upon edit, the Koha news interface tries to pre-select the location declared in the news item. Because the translation doesn't actually exist on the system it is unable to pre-select the right value. With no pre-selected option it defaults to blank.

If you don't notice this, upon saving your news item the location is set to blank and thus appears in all news-related locations.

The disconnect between what is in opaclanguages and what is really installed is probably a product of running a test environment. Is it an issue which need to be addressed in this bug?

Is there a reason to offer an "All" option for news items? I think the option probably hasn't been relevant since we added the "Slip" option for news items.
Comment 26 Marcel de Rooy 2019-12-12 07:38:44 UTC
(In reply to Owen Leonard from comment #25)
> The disconnect between what is in opaclanguages and what is really installed
> is probably a product of running a test environment. Is it an issue which
> need to be addressed in this bug?

Could we have similar problems when a language is disabled on the system at a certain point (while having news items connected to it)?
Comment 27 Owen Leonard 2019-12-12 13:08:29 UTC
(In reply to Marcel de Rooy from comment #26)

> Could we have similar problems when a language is disabled on the system at
> a certain point (while having news items connected to it)?

The news interface offers the ability to add items in any translation which is present, not just enabled. So in order for the problem to repeat I think you'd have to:

1. Delete a translation without disabling it in the OPAC first.
2. Run this database update.
3. Try to edit a news item in the previously-enabled language.

There is also the disconnect between languages enabled in the staff client and those enabled in the OPAC. It sounds like the db update should try to take into account installed languages rather than enabled languages.
Comment 28 Owen Leonard 2019-12-12 13:33:50 UTC
Created attachment 96219 [details] [review]
Bug 22880: (follow-up) Process all installed languages, not just enabled

This patch changes the database update so that the system preference is
copied to news items in all installed languages, not just those enabled
in the OPAC.
Comment 29 Lisette Scheer 2020-02-12 19:56:05 UTC
Created attachment 98773 [details] [review]
Bug 24366: Improve hashref construction

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Lisette Scheer 2020-02-12 19:57:16 UTC
Created attachment 98775 [details] [review]
Bug 22880: Convert opacheader system preference to news block

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

bsoletes: 98773 - Bug 24366: Improve hashref construction
Comment 31 Lisette Scheer 2020-02-12 20:02:12 UTC
Comment on attachment 98773 [details] [review]
Bug 24366: Improve hashref construction

My devbox attached an incorrect patch to this bug. Fixing it so I can try again.
Comment 32 Lisette Scheer 2020-02-12 20:02:43 UTC
Comment on attachment 98775 [details] [review]
Bug 22880: Convert opacheader system preference to news block

My devbox attached an incorrect patch to this bug. Fixing it so I can try again.
Comment 33 Lisette Scheer 2020-02-14 17:36:42 UTC
Created attachment 98985 [details] [review]
Bug 22880: Convert opacheader system preference to news block

This patch builds on Bug 22318 to move the opacheader system preference
into the Koha news system, making it possible to have language- and
library-specific content.

It extends the original patch by adding an option to the template plugin
specifying whether the content title should be shown (probably should be
a flag in opac_news). It also adds a wrapper div with the content
location as the ID (e.g. 'opacheader'). This will make it slightly more
backwards-compatible with CSS customizations.

To test you should have some content in the opacheader system
preference. Apply the patch and run the database update process.

 - Go to the OPAC and confirm that the content which was previously in
   the opacheader system preference now displays correctly where it was
   before.
 - In the staff client, go to Tools -> News and verify that the content
   from opacheader is now stored in a news item with the location
   'opacheader_en.'
 - Go to Administration -> System preferences and confirm that the
   opacheader preference has been removed.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 34 Lisette Scheer 2020-02-14 17:37:26 UTC
Created attachment 98986 [details] [review]
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc.

This patch makes a number of improvements and ties up some loose ends
where the old system preference usage wasn't getting replaced.

 - Removes news-specific code for defining the language of news
   items queried for the OPAC home page. An identical language variable
   is already defined globally.

   Previous to this patch an 'opacheader' news item would only appear on
   the OPAC home page. Now it should appear on all OPAC pages.

 - Changes the database update so that 'opacheader' news
   items will be inserted with a default title, matching the interface's
   requirement that the title field be populated.

 - The database update will also now insert the old opacheader system
   preference contents into the news item for all active languages as
   defined in the 'opaclanguages' system preference. This helps match
   the previous behavior in which the opacheader contents were the same
   for all languages.

 - Adds support for the new opacheader news item to the self checkout,
   self check-in, and OPAC maintenance pages.

 - Updates sysprefs.t which was using the opacheader preference to test
   on. I've changed it to use URLLinkText instead.

 - Removes the addition of the opacheader system preference from the
   installation SQL file.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 35 Lisette Scheer 2020-02-14 17:38:03 UTC
Created attachment 98987 [details] [review]
Bug 22880: (follow-up) Define OpacHeader variable for conditional

This follow-up uses the updates introduced by Bug 23253 as a model for
improving the display of opacheader. Adding the OpacHeader variable
allows the template to check for its existence and display some
container markup around opacheader if it exists. This will help ease the
transition for libraries who depend on the #opacheader id for styling.

To test, apply the patch and test the OPAC with an 'opacheader' item
defined. The content should be displayed on the page inside '<div
id="opacheader"></div>.'

Delete the 'opacheader' news item and reload the OPAC page. There should
no longer be an #opacheader div.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 36 Lisette Scheer 2020-02-14 17:38:29 UTC
Created attachment 98988 [details] [review]
Bug 22880: Use placeholders

Always.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 37 Lisette Scheer 2020-02-14 17:38:56 UTC
Created attachment 98989 [details] [review]
Bug 22880: (follow-up) Process all installed languages, not just enabled

This patch changes the database update so that the system preference is
copied to news items in all installed languages, not just those enabled
in the OPAC.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 38 Martin Renvoize 2020-02-21 08:36:46 UTC
Still on this one Marcelr..?
Comment 39 Marcel de Rooy 2020-02-21 09:16:57 UTC
There we go
Comment 40 Marcel de Rooy 2020-02-21 09:41:30 UTC
Hmm bit of a struggle to get this rebased.. Hang on
Comment 41 Marcel de Rooy 2020-02-21 09:41:49 UTC
 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
   OK     forbidden patterns
   OK     git manipulation
   SKIP   spelling
   OK     tt_valid
   FAIL   valid_template
                koha_news_block: not found
Comment 42 Marcel de Rooy 2020-02-21 09:42:19 UTC
Removing Academy
Comment 43 Marcel de Rooy 2020-02-21 09:54:12 UTC
(In reply to Marcel de Rooy from comment #40)
> Hmm bit of a struggle to get this rebased.. Hang on

Something with masthead.inc and the atomic update ..
Comment 44 Marcel de Rooy 2020-02-21 09:57:03 UTC
Created attachment 99362 [details] [review]
Bug 22880: Convert opacheader system preference to news block

This patch builds on Bug 22318 to move the opacheader system preference
into the Koha news system, making it possible to have language- and
library-specific content.

It extends the original patch by adding an option to the template plugin
specifying whether the content title should be shown (probably should be
a flag in opac_news). It also adds a wrapper div with the content
location as the ID (e.g. 'opacheader'). This will make it slightly more
backwards-compatible with CSS customizations.

To test you should have some content in the opacheader system
preference. Apply the patch and run the database update process.

 - Go to the OPAC and confirm that the content which was previously in
   the opacheader system preference now displays correctly where it was
   before.
 - In the staff client, go to Tools -> News and verify that the content
   from opacheader is now stored in a news item with the location
   'opacheader_en.'
 - Go to Administration -> System preferences and confirm that the
   opacheader preference has been removed.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 45 Marcel de Rooy 2020-02-21 09:57:08 UTC
Created attachment 99363 [details] [review]
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc.

This patch makes a number of improvements and ties up some loose ends
where the old system preference usage wasn't getting replaced.

 - Removes news-specific code for defining the language of news
   items queried for the OPAC home page. An identical language variable
   is already defined globally.

   Previous to this patch an 'opacheader' news item would only appear on
   the OPAC home page. Now it should appear on all OPAC pages.

 - Changes the database update so that 'opacheader' news
   items will be inserted with a default title, matching the interface's
   requirement that the title field be populated.

 - The database update will also now insert the old opacheader system
   preference contents into the news item for all active languages as
   defined in the 'opaclanguages' system preference. This helps match
   the previous behavior in which the opacheader contents were the same
   for all languages.

 - Adds support for the new opacheader news item to the self checkout,
   self check-in, and OPAC maintenance pages.

 - Updates sysprefs.t which was using the opacheader preference to test
   on. I've changed it to use URLLinkText instead.

 - Removes the addition of the opacheader system preference from the
   installation SQL file.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 46 Marcel de Rooy 2020-02-21 09:57:13 UTC
Created attachment 99364 [details] [review]
Bug 22880: (follow-up) Define OpacHeader variable for conditional

This follow-up uses the updates introduced by Bug 23253 as a model for
improving the display of opacheader. Adding the OpacHeader variable
allows the template to check for its existence and display some
container markup around opacheader if it exists. This will help ease the
transition for libraries who depend on the #opacheader id for styling.

To test, apply the patch and test the OPAC with an 'opacheader' item
defined. The content should be displayed on the page inside '<div
id="opacheader"></div>.'

Delete the 'opacheader' news item and reload the OPAC page. There should
no longer be an #opacheader div.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 47 Marcel de Rooy 2020-02-21 09:57:18 UTC
Created attachment 99365 [details] [review]
Bug 22880: Use placeholders

Always.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 48 Marcel de Rooy 2020-02-21 09:57:22 UTC
Created attachment 99366 [details] [review]
Bug 22880: (follow-up) Process all installed languages, not just enabled

This patch changes the database update so that the system preference is
copied to news items in all installed languages, not just those enabled
in the OPAC.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 49 Marcel de Rooy 2020-02-21 09:59:42 UTC
(In reply to Marcel de Rooy from comment #47)
> Created attachment 99365 [details] [review] [review]
> Bug 22880: Use placeholders
> 
> Always.

Not really the most descriptive commit title and message I've seen..
Comment 50 Marcel de Rooy 2020-02-21 10:11:04 UTC
DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "INSERT INTO opac_news ( timestamp,content,title,expirationdate,branchcode,lang,borrowernumber,number ) VALUES ( ?,?,?,?,?,?,?,? )" with ParamValues: 0='2020-02-21', 1="<p>TESTing</p>", 2="Welcome", 3=undef, 4=undef, 5="opacheader_en", 6="51", 7=""] at /usr/share/koha/C4/NewsChannels.pm line 68.
Comment 51 Marcel de Rooy 2020-02-21 10:35:33 UTC
(In reply to Marcel de Rooy from comment #50)
> DBD::mysql::st execute failed: Incorrect integer value: '' for column
> `koha_master`.`opac_news`.`number` at row 1 [for Statement "INSERT INTO
> opac_news (
> timestamp,content,title,expirationdate,branchcode,lang,borrowernumber,number
> ) VALUES ( ?,?,?,?,?,?,?,? )" with ParamValues: 0='2020-02-21',
> 1="<p>TESTing</p>", 2="Welcome", 3=undef, 4=undef, 5="opacheader_en",
> 6="51", 7=""] at /usr/share/koha/C4/NewsChannels.pm line 68.

Can be prevented by less strict sql mode, needs attention.
Comment 52 Marcel de Rooy 2020-02-21 10:49:38 UTC Comment hidden (obsolete)
Comment 53 Marcel de Rooy 2020-02-21 10:50:09 UTC
Created attachment 99367 [details] [review]
Bug 22880: (QA follow-up) Fix "koha_news_block: not found"

To make qa tools happy again.
Comment 54 Marcel de Rooy 2020-02-24 12:16:25 UTC
Created attachment 99490 [details] [review]
Bug 22880: Convert opacheader system preference to news block

This patch builds on Bug 22318 to move the opacheader system preference
into the Koha news system, making it possible to have language- and
library-specific content.

It extends the original patch by adding an option to the template plugin
specifying whether the content title should be shown (probably should be
a flag in opac_news). It also adds a wrapper div with the content
location as the ID (e.g. 'opacheader'). This will make it slightly more
backwards-compatible with CSS customizations.

To test you should have some content in the opacheader system
preference. Apply the patch and run the database update process.

 - Go to the OPAC and confirm that the content which was previously in
   the opacheader system preference now displays correctly where it was
   before.
 - In the staff client, go to Tools -> News and verify that the content
   from opacheader is now stored in a news item with the location
   'opacheader_en.'
 - Go to Administration -> System preferences and confirm that the
   opacheader preference has been removed.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2020-02-24 12:16:30 UTC
Created attachment 99491 [details] [review]
Bug 22880: (follow-up) Loose ends: Better DB update, global inclusion, etc.

This patch makes a number of improvements and ties up some loose ends
where the old system preference usage wasn't getting replaced.

 - Removes news-specific code for defining the language of news
   items queried for the OPAC home page. An identical language variable
   is already defined globally.

   Previous to this patch an 'opacheader' news item would only appear on
   the OPAC home page. Now it should appear on all OPAC pages.

 - Changes the database update so that 'opacheader' news
   items will be inserted with a default title, matching the interface's
   requirement that the title field be populated.

 - The database update will also now insert the old opacheader system
   preference contents into the news item for all active languages as
   defined in the 'opaclanguages' system preference. This helps match
   the previous behavior in which the opacheader contents were the same
   for all languages.

 - Adds support for the new opacheader news item to the self checkout,
   self check-in, and OPAC maintenance pages.

 - Updates sysprefs.t which was using the opacheader preference to test
   on. I've changed it to use URLLinkText instead.

 - Removes the addition of the opacheader system preference from the
   installation SQL file.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Marcel de Rooy 2020-02-24 12:16:35 UTC
Created attachment 99492 [details] [review]
Bug 22880: (follow-up) Define OpacHeader variable for conditional

This follow-up uses the updates introduced by Bug 23253 as a model for
improving the display of opacheader. Adding the OpacHeader variable
allows the template to check for its existence and display some
container markup around opacheader if it exists. This will help ease the
transition for libraries who depend on the #opacheader id for styling.

To test, apply the patch and test the OPAC with an 'opacheader' item
defined. The content should be displayed on the page inside '<div
id="opacheader"></div>.'

Delete the 'opacheader' news item and reload the OPAC page. There should
no longer be an #opacheader div.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2020-02-24 12:16:41 UTC
Created attachment 99493 [details] [review]
Bug 22880: Use placeholders

Always.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2020-02-24 12:16:45 UTC
Created attachment 99494 [details] [review]
Bug 22880: (follow-up) Process all installed languages, not just enabled

This patch changes the database update so that the system preference is
copied to news items in all installed languages, not just those enabled
in the OPAC.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Marcel de Rooy 2020-02-24 12:16:50 UTC
Created attachment 99495 [details] [review]
Bug 22880: (QA follow-up) Fix "koha_news_block: not found"

To make qa tools happy again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2020-02-24 12:16:55 UTC
Created attachment 99496 [details] [review]
Bug 22880: (QA follow-up) Remove opacheader from C4::Auth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2020-02-24 12:16:59 UTC
Created attachment 99497 [details] [review]
Bug 22880: (QA follow-up) Pass lang instead of news_lang in masthead

Might be a rebase problem. Not sure if I look at the third patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2020-02-24 12:17:04 UTC
Created attachment 99498 [details] [review]
Bug 22880: (QA follow-up) Replace use by require

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Martin Renvoize 2020-02-24 13:27:33 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 64 Joy Nelson 2020-03-31 22:13:12 UTC
Enhancement not backported to 19.11.x