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

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

(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.

However we should have the USE raw on top, in case the block is called from a file that does not have it yet.

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.

(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?

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.

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!

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!

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>

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>

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>

QAing

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.

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.

(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?

> > 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.

(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)?

(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.

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.

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>

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 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 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.

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>

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>

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>

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>

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>

Still on this one Marcelr..?

There we go

Hmm bit of a struggle to get this rebased.. Hang on

 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

Removing Academy

(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 ..

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>

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>

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>

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>

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>

(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..

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.

(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.

Created attachment 99367 [details] [review]
Bug 22880: (QA follow-up) Fix "koha_news_block: not found"

To make qa tools happy again.

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>

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>

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>

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>

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>

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>

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>

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>

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>

Nice work everyone!

Pushed to master for 20.05

Enhancement not backported to 19.11.x