Bug 24745 - OPAC news block plugin should evaluate as false if there are no items
Summary: OPAC news block plugin should evaluate as false if there are no items
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 23796 24223
  Show dependency treegraph
 
Reported: 2020-02-27 17:12 UTC by Owen Leonard
Modified: 2020-04-03 22:17 UTC (History)
3 users (show)

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


Attachments
Bug 24745: OPAC news block plugin should evaluate as false if there are no items (1.49 KB, patch)
2020-02-27 17:26 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24745: OPAC news block plugin should evaluate as false if there are no items (1.57 KB, patch)
2020-03-04 18:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24745: OPAC news block plugin should evaluate as false if there are no items (1.65 KB, patch)
2020-03-04 19:15 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24745: (follow-up) Correct perl syntax (847 bytes, patch)
2020-03-24 12:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24745: OPAC news block plugin should evaluate as false if there are no items (1.74 KB, patch)
2020-03-24 14:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24745: (follow-up) Correct return statement in news plugin (1.04 KB, patch)
2020-03-24 14:06 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 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.