| Summary: | Add more options to 'AllowSelfCheckReturns' syspref | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Mason James <mtj> |
| Component: | Self checkout | Assignee: | Mason James <mtj> |
| Status: | Failed QA --- | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | bob, costalc, gitbot, jonathan.druart, kyle, sandboxes |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10358 | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 10358 | ||
| Attachments: |
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref
Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref |
||
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 Created attachment 19495 [details] [review] Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref 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 ---------- 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 http://bugs.koha-community.org/show_bug.cgi?id=10544 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. 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. Created attachment 21109 [details] [review] Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref 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 ---------- 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 http://bugs.koha-community.org/show_bug.cgi?id=10544 rebased on master 6fcb671 (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 :) 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". Created attachment 21221 [details] [review] Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref 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 ---------- 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 http://bugs.koha-community.org/show_bug.cgi?id=10544 Signed-off-by: Mason James <mtj@kohaaloha.com> Patch applied cleanly, go forth and signoff 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. Patch applied cleanly, go forth and signoff 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? Patch tested with a sandbox, by Pierre Lorgue <pierre.lorgue@ens-cachan.fr> Created attachment 28618 [details] [review] Bug 10554 - Add more options to 'AllowSelfCheckReturns' syspref 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 ---------- 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 Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Pierre Lorgue <pierre.lorgue@ens-cachan.fr> I added the signoff line. Mason, Could you please answer to Mark's comment please? Marked as Failed QA. 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 *** Bug 10358 has been marked as a duplicate of this bug. *** (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 (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 > > @@ +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
(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? 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. Created attachment 42267 [details] [review] Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref (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 (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 (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 (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) Created attachment 42272 [details] [review] Bug 10554: Add more options to 'AllowSelfCheckReturns' syspref 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 (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? |
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