Bug 24745

Summary: OPAC news block plugin should evaluate as false if there are no items
Product: Koha Reporter: Owen Leonard <oleonard>
Component: OPACAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: 1joynelson, bgkriegel, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24746
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 23796, 24223    
Attachments: Bug 24745: OPAC news block plugin should evaluate as false if there are no items
Bug 24745: OPAC news block plugin should evaluate as false if there are no items
Bug 24745: OPAC news block plugin should evaluate as false if there are no items
Bug 24745: (follow-up) Correct perl syntax
Bug 24745: OPAC news block plugin should evaluate as false if there are no items
Bug 24745: (follow-up) Correct return statement in news plugin

Description Owen Leonard 2020-02-27 17:12:30 UTC
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.
Comment 1 Owen Leonard 2020-02-27 17:26:22 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2020-03-03 12:37:31 UTC
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)
Comment 3 Owen Leonard 2020-03-04 18:55:56 UTC
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>
Comment 4 Owen Leonard 2020-03-04 18:57:51 UTC
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.
Comment 5 Bernardo Gonzalez Kriegel 2020-03-04 19:15:28 UTC
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.
Comment 6 Marcel de Rooy 2020-03-06 08:53:05 UTC
+    } 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 ?
Comment 7 Owen Leonard 2020-03-06 12:55:55 UTC
(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.
Comment 8 Marcel de Rooy 2020-03-09 07:39:02 UTC
(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.
Comment 9 Owen Leonard 2020-03-24 12:52:46 UTC
Created attachment 101558 [details] [review]
Bug 24745: (follow-up) Correct perl syntax

Subroutine should "return;" instead of "return 0;"
Comment 10 Owen Leonard 2020-03-24 12:53:06 UTC
Thanks Marcel!
Comment 11 Marcel de Rooy 2020-03-24 14:06:23 UTC
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>
Comment 12 Marcel de Rooy 2020-03-24 14:06:28 UTC
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.
Comment 13 Martin Renvoize 2020-03-25 09:43:36 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-03 22:17:45 UTC
patches do not apply to 19.11.x  not backported.
please rebase if needed.