Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
Summary: Add more options to 'AllowSelfCheckReturns' syspref
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mason James
QA Contact:
URL:
Keywords:
: 10358 (view as bug list)
Depends on:
Blocks: 10358
  Show dependency treegraph
 
Reported: 2013-07-09 01:37 UTC by Mason James
Modified: 2019-04-11 18:46 UTC (History)
7 users (show)

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


Attachments
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref (8.99 KB, patch)
2013-07-09 02:19 UTC, Mason James
Details | Diff | Splinter Review
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref (9.13 KB, patch)
2013-09-16 01:42 UTC, Mason James
Details | Diff | Splinter Review
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref (9.72 KB, patch)
2013-09-19 06:04 UTC, Mason James
Details | Diff | Splinter Review
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref (9.11 KB, patch)
2014-06-02 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref (7.32 KB, patch)
2015-09-03 07:17 UTC, Mason James
Details | Diff | Splinter Review
Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref (7.33 KB, patch)
2015-09-03 07:55 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2013-07-09 01:37:36 UTC
This patch modifies the options of AllowSelfCheckReturns syspref

from a 2 option selection
 - 'allow patrons', dont allow patrons'

to a 3 option selection
 - 'allow all patrons', 'allow non-blocked patrons' , allow no patrons'


currently, a problem with the syspref behavior in Koha is that enabling the 'AllowSelfCheckReturns' syspref does *not* allow patrons with a 'block' to return items via the SCO. (this is apparently by design, and not a bug)

however, some libraries do want to allow patrons to return items via the SCO even if they have a 'block'

this patch adds this additional behavior, by adding a new option to the syspref, but also retaining the existing syspref behavior

after the patch, there are 2 allow options
 - 'Allow all' allows all patrons to return items (including blocked patrons)
 - 'Allow non-blocked' allows only non-blocked patrons to return items 
     (note: this is the current 'allow' behavior)

the patch correctly 'upgrades' existing 'allow' selection to the new 'Allow non-blocked' value. so, any existing 'allow' selection works as before
Comment 1 Mason James 2013-07-09 01:58:07 UTC
to test these 3 syspref options...

1/
- set syspref to 'dont allow'
- issue an item to a patron
- attempt to return item via SCO,  return should FAIL

2/ 
- set syspref to 'allow all'
- issue an item to a patron
- attempt to return item via SCO, return should SUCCEED

3/
- set syspref to 'allow nonblocked'
- issue an item to a patron
- attempt to return item via SCO, return should SUCCEED

- issue an item to a patron, with a returndate of '2001-01-01'
(important: this will create an overdue item, that will put a 'block' on this patron's account)
- attempt to return item via SCO, return should FAIL
Comment 2 Mason James 2013-07-09 02:19:35 UTC Comment hidden (obsolete)
Comment 3 Bob Birchall 2013-07-17 01:52:53 UTC
Attempting sign-off against a biblibre sandbox (#8).  The patch hasn't applied.  I can't tell if the problem is the sandbox or the patch.  Will check with Mason.
Comment 4 Bob Birchall 2013-07-17 07:27:11 UTC
The earlier error was mine.

I have now tested against all three conditions and have observed the expected behaviour.  The overdue item can be returned when the sys pref is 'Allow all' but cannot be returned when the sys pref is 'Allow Non-Blocked' (as expected).

I was not able to test this aspect:
the patch correctly 'upgrades' existing 'allow' selection to the new 'Allow non-blocked' value. so, any existing 'allow' selection works as before

Maybe that needs a separate sign-off?

Signing off that the test plan executes correctly.
Comment 5 Mason James 2013-09-16 01:42:12 UTC Comment hidden (obsolete)
Comment 6 Mason James 2013-09-16 01:46:16 UTC
rebased on master 6fcb671
Comment 7 Mason James 2013-09-16 01:53:51 UTC
(In reply to Bob Birchall from comment #4)
> The earlier error was mine.
> 
> I have now tested against all three conditions and have observed the
> expected behaviour.  The overdue item can be returned when the sys pref is
> 'Allow all' but cannot be returned when the sys pref is 'Allow Non-Blocked'
> (as expected).
> 
> I was not able to test this aspect:
> the patch correctly 'upgrades' existing 'allow' selection to the new 'Allow
> non-blocked' value. so, any existing 'allow' selection works as before
> 

this step can be tested by setting the syspref to 'allow' before applying the patch - then apply the patch...

the syspref should now be set to 'allow-nonblocked' 

> Maybe that needs a separate sign-off?

OK with me :)
Comment 8 I'm just a bot 2013-09-18 11:28:11 UTC
Applying: Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
Using index info to reconstruct a base tree...
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Patch failed at 0001 Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 9 Mason James 2013-09-19 06:04:42 UTC Comment hidden (obsolete)
Comment 10 I'm just a bot 2013-09-29 06:11:31 UTC
Patch applied cleanly, go forth and signoff
Comment 11 Bob Birchall 2013-10-22 21:21:13 UTC
I am no longer able to prove the pre-existing behaviour, that is, that only non-blocked patrons can return via SCO.  

I checked an item out to a patron, setting the due date to a date prior to the current date (ie it is immediately overdue).  I set the patron to 'restricted' and I set the patrons membership expiry date to a date preceding the current date.

I was able to return the item via SCO.
Comment 12 I'm just a bot 2013-10-30 07:49:55 UTC
Patch applied cleanly, go forth and signoff
Comment 13 Mark Tompsett 2014-03-19 16:46:26 UTC
Comment on attachment 21221 [details] [review]
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref

Review of attachment 21221 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/sysprefs.sql
@@ +419,5 @@
>  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'),
>  ('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo')
>  ;
> +
> +installer/data/mysql/sysprefs.sql

I think this is a mistake.

::: koha-tmpl/opac-tmpl/prog/en/modules/sco/sco-main.tt
@@ +140,4 @@
>     [% END %]
>     Please see a member of the library staff.
>  </p>
> +[% IF  (AllowSelfCheckReturns == 'nonblocked' and returnitem ) or AllowSelfCheckReturns  == 'all' %]

Why not leave the
[% IF ( returnitem ) %]
line, and then just change the next line to:
[% IF AllowSelfCheckReturns == 'nonblocked' or AllowSelfCheckReturns == 'all' %]
Because returnitem should be set correctly, should it not? And if not, where's the logic for it that needs fixing?

::: opac/sco/sco-main.pl
@@ +83,4 @@
>  $template->param(SelfCheckTimeout => $selfchecktimeout);
>  
>  # Checks policy laid out by AllowSelfCheckReturns, defaults to 'on' if preference is undefined
> +my $allowselfcheckreturns = 'nonblocked';

Actually, I was thinking these 4 lines could be optimized to:
my $allowselfcheckreturns = C4::Context->preference('AllowSelfCheckReturns') // 'nonblocked';

@@ +152,4 @@
>              "circ_error_$issue_error" => 1,
>              title                     => $item->{title},
>              hide_main                 => 1,
> +            barcode                   => $barcode,

Is this a secondary bug that you discovered while fixing this?
Comment 14 Biblibre Sandboxes 2014-05-30 10:41:53 UTC
Patch tested with a sandbox, by Pierre Lorgue <pierre.lorgue@ens-cachan.fr>
Comment 15 Jonathan Druart 2014-06-02 14:53:05 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-06-02 14:53:40 UTC
I added the signoff line.
Comment 17 Jonathan Druart 2014-06-02 14:58:29 UTC
Mason,

Could you please answer to Mark's comment please?

Marked as Failed QA.
Comment 18 Jonathan Druart 2014-06-02 14:59:32 UTC
Comment on attachment 28618 [details] [review]
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref

Review of attachment 28618 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/updatedatabase.pl
@@ +8555,5 @@
> +if ( CheckVersion($DBversion) ) {
> +
> +    #get current value
> +    my $sth = $dbh->prepare("SELECT * from systempreferences where variable = 'AllowSelfCheckReturns'");
> +    $sth->execute;

Should be replaced with $dbh->selectcol_arrayref
Comment 19 Katrin Fischer 2014-11-29 09:09:38 UTC
*** Bug 10358 has been marked as a duplicate of this bug. ***
Comment 20 Mason James 2015-02-03 22:12:39 UTC
(In reply to M. Tompsett from comment #13)
> Comment on attachment 21221 [details] [review] [review]
> Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
> 
> Review of attachment 21221 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/sysprefs.sql
> @@ +419,5 @@
> >  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'),
> >  ('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo')
> >  ;
> > +
> > +installer/data/mysql/sysprefs.sql
> 
> I think this is a mistake.

yes, the addition of those two lines is a mistake

> 
> ::: koha-tmpl/opac-tmpl/prog/en/modules/sco/sco-main.tt
> @@ +140,4 @@
> >     [% END %]
> >     Please see a member of the library staff.
> >  </p>
> > +[% IF  (AllowSelfCheckReturns == 'nonblocked' and returnitem ) or AllowSelfCheckReturns  == 'all' %]
> 
> Why not leave the
> [% IF ( returnitem ) %]
> line, and then just change the next line to:
> [% IF AllowSelfCheckReturns == 'nonblocked' or AllowSelfCheckReturns ==
> 'all' %]
> Because returnitem should be set correctly, should it not? And if not,
> where's the logic for it that needs fixing?

if your suggestion tests OK, then i am happy

it makes no difference to me which syntax is used :)

> ::: opac/sco/sco-main.pl
> @@ +83,4 @@
> >  $template->param(SelfCheckTimeout => $selfchecktimeout);
> >  
> >  # Checks policy laid out by AllowSelfCheckReturns, defaults to 'on' if preference is undefined
> > +my $allowselfcheckreturns = 'nonblocked';
> 
> Actually, I was thinking these 4 lines could be optimized to:
> my $allowselfcheckreturns = C4::Context->preference('AllowSelfCheckReturns')
> // 'nonblocked';

yes, looks good…

> 
> @@ +152,4 @@
> >              "circ_error_$issue_error" => 1,
> >              title                     => $item->{title},
> >              hide_main                 => 1,
> > +            barcode                   => $barcode,
> 
> Is this a secondary bug that you discovered while fixing this?

yep, its a small secondary bug
Comment 21 Mason James 2015-02-03 22:13:33 UTC
(In reply to Jonathan Druart from comment #17)
> Mason,
> 
> Could you please answer to Mark's comment please?
> 
> Marked as Failed QA.

yep, answered above
Comment 22 Mason James 2015-02-03 22:16:07 UTC
> > @@ +152,4 @@
> > >              "circ_error_$issue_error" => 1,
> > >              title                     => $item->{title},
> > >              hide_main                 => 1,
> > > +            barcode                   => $barcode,
> > 
> > Is this a secondary bug that you discovered while fixing this?
> 
> yep, its a small secondary bug


the bug was that barcode info was not being passed to the template, therefore was not being displayed to the screen, as expected
Comment 23 Bob Birchall 2015-06-01 23:39:48 UTC
(In reply to Mason James from comment #22)
> > > @@ +152,4 @@
> > > >              "circ_error_$issue_error" => 1,
> > > >              title                     => $item->{title},
> > > >              hide_main                 => 1,
> > > > +            barcode                   => $barcode,
> > > 
> > > Is this a secondary bug that you discovered while fixing this?
> > 
> > yep, its a small secondary bug
> 
> 
> the bug was that barcode info was not being passed to the template,
> therefore was not being displayed to the screen, as expected

I'm not sure who has to take the next action here. Is 'Failed QA' still the right status?
Comment 24 Katrin Fischer 2015-06-02 06:16:14 UTC
It looks to me like some changes need to be made to the patch for this to continue - especially comment#18 and some things from comment#20.
Comment 25 Mason James 2015-09-03 07:17:47 UTC Comment hidden (obsolete)
Comment 26 Mason James 2015-09-03 07:20:08 UTC
(In reply to Jonathan Druart from comment #18)
> Comment on attachment 28618 [details] [review] [review]
> Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
> 
> Review of attachment 28618 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/updatedatabase.pl
> @@ +8555,5 @@
> > +if ( CheckVersion($DBversion) ) {
> > +
> > +    #get current value
> > +    my $sth = $dbh->prepare("SELECT * from systempreferences where variable = 'AllowSelfCheckReturns'");
> > +    $sth->execute;
> 
> Should be replaced with $dbh->selectcol_arrayref

this is done
Comment 27 Mason James 2015-09-03 07:27:55 UTC
(In reply to Mason James from comment #20)
> (In reply to M. Tompsett from comment #13)
> > Comment on attachment 21221 [details] [review] [review] [review]
> > Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
> > 
> > Review of attachment 21221 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: installer/data/mysql/sysprefs.sql
> > @@ +419,5 @@
> > >  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'),
> > >  ('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo')
> > >  ;
> > > +
> > > +installer/data/mysql/sysprefs.sql
> > 
> > I think this is a mistake.
> 
> yes, the addition of those two lines is a mistake

this mistake has been corrected
Comment 28 Mason James 2015-09-03 07:32:56 UTC
(In reply to Katrin Fischer from comment #24)
> It looks to me like some changes need to be made to the patch for this to
> continue - especially comment#18 and some things from comment#20.

comments have been answered

i've left Mark's syntactic suggestions, as they are non-critical
Comment 29 Mason James 2015-09-03 07:36:31 UTC
(In reply to Jonathan Druart from comment #17)
> Mason,
> 
> Could you please answer to Mark's comment please?
> 
> Marked as Failed QA.

..setting patch back to 'ready for QA' again, (hope thats OK)
Comment 30 Mason James 2015-09-03 07:55:30 UTC
Created attachment 42272 [details] [review]
Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref
Comment 31 Jonathan Druart 2015-09-03 08:17:51 UTC
I don't understand the changes at all and after a few test I understood why.
It does not work as expected.
For instance:
Current behavior:
1/ Staff side: Check item I out to patron P1
2/ Sco: Try to check the item out to patron P2
you will get "Item cannot be checked out." with a button "Return to account".

With your patch and the pref set to "all", you get 2 buttons:
"Return this item" and "Return to account"

Which does not make any sense.

Next time, please set the status back to Needs Signoff
Comment 32 Claudio Costales 2019-04-11 18:46:36 UTC
(In reply to Mason James from comment #30)
> Created attachment 42272 [details] [review] [review]
> Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref

This is a great enhancement. Any chance to continue with it?