Description
Owen Leonard
2019-05-09 15:10:32 UTC
Created attachment 89517 [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. 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. Created attachment 90495 [details] [review] Bug 22880: Use placeholders Always. 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. Created attachment 92207 [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> Created attachment 92208 [details] [review] Bug 22880: Use placeholders Always. Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> 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! Created attachment 92592 [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> Created attachment 92593 [details] [review] Bug 22880: Use placeholders Always. Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 92594 [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. 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 96209 [details] [review] Bug 22880: (follow-up) Use global language variable for news language This patch 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. 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. Owen, Please verify that my rebase did not introduce another problem in masthead or the atomic update. I'm having issues now getting the opacheader to work. Somehow it looks for opacheader while I add opacheader_en items into the database via the interface. select title,lang from opac_news; +-----------------+---------------+ | title | lang | +-----------------+---------------+ | Welcome to Koha | koha | | What's Next? | koha | | Test | opacheader_en | +-----------------+---------------+ When I deleted my database, the installer crashes now on the language step. So very soon. (Apparently, for some other reason..) DBD::mysql::st execute failed: Table 'koha_master.language_descriptions' doesn't exist [for Statement "SELECT description FROM language_descriptions WHERE subtag=? AND lang=? AND type=?" with ParamValues: 0='en', 1='en', 2='language'] at /usr/share/koha/C4/Languages.pm line 389. DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [for Statement "SELECT description FROM language_descriptions WHERE subtag=? AND lang=? AND type=?" with ParamValues: 0='en', 1='en', 2='language'] at /usr/share/koha/C4/Languages.pm line 390. Keeping this in BLOCKED. Will come back here next week. 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 |