Bug 23095 - Circulation rules not displayed (empty vs null)
Summary: Circulation rules not displayed (empty vs null)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 18925
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-11 15:44 UTC by Jonathan Druart
Modified: 2020-06-04 20:33 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 23095: Correctly display circulation rules (0 vs undef) (2.02 KB, patch)
2019-06-11 15:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23095: Correctly display circulation rules (0 vs undef) (2.07 KB, patch)
2019-06-11 16:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 23095: Correctly display circulation rules (0 vs undef) (2.12 KB, patch)
2019-06-12 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-06-11 15:44:02 UTC
From the following commit:
  commit d1303ca834dd0b08fc1d44476cfbbb8b729b6165
  Bug 18925: (follow-up) Fix null/empty behavior

The global test must have been adjusted as well to catch empty strings.

Actually we are expecting the plugin to return undef but the template variable contains an empty string.

So the test should only be [% IF var != '' %] instead of [% IF var.defined && var != '' %] but I prefer to keep it as it for now.
Comment 1 Jonathan Druart 2019-06-11 15:47:16 UTC
Created attachment 90502 [details] [review]
Bug 23095: Correctly display circulation rules (0 vs undef)

From the following commit:
  commit d1303ca834dd0b08fc1d44476cfbbb8b729b6165
  Bug 18925: (follow-up) Fix null/empty behavior

The global test must have been adjusted as well to catch empty strings.

Actually we are expecting the plugin to return undef but the template variable contains an empty string.

So the test should only be [% IF var != '' %] instead of [% IF var.defined && var != '' %]
but I prefer to keep it as it for now.

Test plan:
In the section "Default checkout, hold policy by patron category"
Set total checkouts = blank
    total on-site checkouts = blank
    total holds = 0
Save
=> Without this patch the line will not appear
=> With this patch applied there must be "unlimited, unlimited, 0"
Comment 2 Mark Tompsett 2019-06-11 16:53:56 UTC
Created attachment 90510 [details] [review]
Bug 23095: Correctly display circulation rules (0 vs undef)

From the following commit:
  commit d1303ca834dd0b08fc1d44476cfbbb8b729b6165
  Bug 18925: (follow-up) Fix null/empty behavior

The global test must have been adjusted as well to catch empty strings.

Actually we are expecting the plugin to return undef but the template variable contains an empty string.

So the test should only be [% IF var != '' %] instead of [% IF var.defined && var != '' %]
but I prefer to keep it as it for now.

Test plan:
In the section "Default checkout, hold policy by patron category"
Set total checkouts = blank
    total on-site checkouts = blank
    total holds = 0
Save
=> Without this patch the line will not appear
=> With this patch applied there must be "unlimited, unlimited, 0"

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 Jonathan Druart 2019-06-12 14:02:19 UTC
(In reply to Jonathan Druart from comment #1)
> Actually we are expecting the plugin to return undef but the template
> variable contains an empty string.

That's wrong, it's an empty string in DB.
Comment 4 Tomás Cohen Arazi 2019-06-12 18:53:22 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2019-06-12 20:04:28 UTC
Created attachment 90540 [details] [review]
Bug 23095: Correctly display circulation rules (0 vs undef)

From the following commit:
  commit d1303ca834dd0b08fc1d44476cfbbb8b729b6165
  Bug 18925: (follow-up) Fix null/empty behavior

The global test must have been adjusted as well to catch empty strings.

Actually we are expecting the plugin to return undef but the template variable contains an empty string.

So the test should only be [% IF var != '' %] instead of [% IF var.defined && var != '' %]
but I prefer to keep it as it for now.

Test plan:
In the section "Default checkout, hold policy by patron category"
Set total checkouts = blank
    total on-site checkouts = blank
    total holds = 0
Save
=> Without this patch the line will not appear
=> With this patch applied there must be "unlimited, unlimited, 0"

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Martin Renvoize 2019-06-13 11:07:49 UTC
Nice work!

Pushed to master for 19.11.00
Comment 7 Fridolin Somers 2019-06-17 07:51:43 UTC
Pushed to 19.05.x for 19.05.01
Comment 8 Lucas Gass 2019-06-26 14:28:59 UTC
no backport for 18.11.x series as it is missing the dependency