In the OPAC we do this to get a news block for display: [% SET OpacHeader = KohaNews.get( location => "opacheader", lang => lang, library => branchcode, blocktitle => 0 ) %] From this we're supposed to be able to check for the existence of [% OpacHeader %], but the way the data is returned from KohaNews.pm it evaluates as true.
Created attachment 99708 [details] [review] Bug 24745: OPAC news block plugin should evaluate as false if there are no items This patch changes the way news is returned from GetNewsToDisplay in KohaNews.pm so that the template variable will evaluate as false instead of simply being an empty data structure. To reproduce the bug, remove your opacheader news entry and add this to OpacUserCSS: Go to the OPAC and you should see an empty blue-bordered box at the top of the page. To test the fix, apply the patch and reload the OPAC page. The box should disappear. Add content to the opacheader news item and confirm that it displays correctly.
Hi Owen, can't reproduce. Could you please add the missing block of CSS in the test plan? The <div id="opacheader"> is not showing if there are now news (for opacheader)
Whoops, the "#" turned my CSS into a commented line in the commit message :( Also my test plan depended on Bug 24746 still existing! Now to verify the bug you have to look at the page source for a couple of empty <div>s in the header, right after '</div> <!-- /navbar -->' <div class="container-fluid"> <div class="row-fluid"> </div> </div>
Created attachment 100151 [details] [review] Bug 24745: OPAC news block plugin should evaluate as false if there are no items This patch changes the way news is returned from GetNewsToDisplay in KohaNews.pm so that the template variable will evaluate as false instead of simply being an empty data structure. To verify the bug, remove your opacheader news entry and view source on any page in the OPAC. You should find some empty markup right after '</div> <!-- /navbar -->' <div class="container-fluid"> <div class="row-fluid"> </div> </div> To test the fix, apply the patch and reload the OPAC page. The empty divs should be gone. Add content to the opacheader news item and confirm that it displays correctly.
Created attachment 100152 [details] [review] Bug 24745: OPAC news block plugin should evaluate as false if there are no items This patch changes the way news is returned from GetNewsToDisplay in KohaNews.pm so that the template variable will evaluate as false instead of simply being an empty data structure. To verify the bug, remove your opacheader news entry and view source on any page in the OPAC. You should find some empty markup right after '</div> <!-- /navbar -->' <div class="container-fluid"> <div class="row-fluid"> </div> </div> To test the fix, apply the patch and reload the OPAC page. The empty divs should be gone. Add content to the opacheader news item and confirm that it displays correctly. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Divs gone, no errors.
+ } else { + return 0; + } Wouldnt it be better to return undef or empty string in case of template use? If it is concatenated somewhere, we get a 0 in between ?
(In reply to Marcel de Rooy from comment #6) > Wouldnt it be better to return undef or empty string in case of template use? > If it is concatenated somewhere, we get a 0 in between ? Returning undef triggered this error in the QA tool: "# Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s)." I don't think there is a scenario where this variable would be concatenated with something else, but if you think it's worth preparing for I would be happy to sign off on a follow-up.
(In reply to Owen Leonard from comment #7) > (In reply to Marcel de Rooy from comment #6) > > Wouldnt it be better to return undef or empty string in case of template use? > > If it is concatenated somewhere, we get a 0 in between ? > > Returning undef triggered this error in the QA tool: > > "# Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s)." This has to do with syntax. You should not write "return undef" but you write "return;". In scalar context you return undef, in list context empty list.
Created attachment 101558 [details] [review] Bug 24745: (follow-up) Correct perl syntax Subroutine should "return;" instead of "return 0;"
Thanks Marcel!
Created attachment 101586 [details] [review] Bug 24745: OPAC news block plugin should evaluate as false if there are no items This patch changes the way news is returned from GetNewsToDisplay in KohaNews.pm so that the template variable will evaluate as false instead of simply being an empty data structure. To verify the bug, remove your opacheader news entry and view source on any page in the OPAC. You should find some empty markup right after '</div> <!-- /navbar -->' <div class="container-fluid"> <div class="row-fluid"> </div> </div> To test the fix, apply the patch and reload the OPAC page. The empty divs should be gone. Add content to the opacheader news item and confirm that it displays correctly. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Divs gone, no errors. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 101587 [details] [review] Bug 24745: (follow-up) Correct return statement in news plugin Subroutine should "return;" instead of "return 0;" We also prevent ProhibitExplicitReturnUndef by not returning "undef" explicitly. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Commit message edited.
Nice work everyone! Pushed to master for 20.05
patches do not apply to 19.11.x not backported. please rebase if needed.