Bug 6261 - Consolidate patron toolbar include files
Summary: Consolidate patron toolbar include files
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Jonathan Druart
URL:
Keywords:
: 9279 (view as bug list)
Depends on: 5986
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-25 13:01 UTC by Owen Leonard
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

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


Attachments
Bug 6261 - Consolidate patron toolbar include files (25.38 KB, patch)
2012-11-16 15:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6261 [Revised] Consolidate patron toolbar include files (26.30 KB, patch)
2012-11-29 19:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6261 [Revised] Consolidate patron toolbar include files (26.49 KB, patch)
2012-12-19 21:14 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6261 [Revised] Consolidate patron toolbar include files (26.51 KB, patch)
2012-12-21 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6261 [Revised] Consolidate patron toolbar include files (26.57 KB, patch)
2012-12-26 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2011-04-25 13:01:13 UTC
We have two include files which are almost identical, a remnant I think of earlier version of Koha in which it was necessary that they be different. They should be consolidated.
Comment 1 Owen Leonard 2012-11-16 15:02:40 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2012-11-18 08:51:05 UTC
With following testplan I get different results:

a) Create a new patron
b) Change registration date of patron (e.g. back one month)
c) Go to More->Renew patron

Without patch, I get a message like "Patron's account has been renewed until 18/02/2021" with the patron's details below.

With patch applied, I get following message: "This patron does not exist."
Comment 3 Owen Leonard 2012-11-29 19:43:33 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2012-11-29 21:42:09 UTC
Tested again with testplan from comment #2:

a) Create a new patron
b) Change registration date of patron (e.g. back one month)
c) Go to More->Renew patron

Without patch, I get a message like "a message like "Patron's account has been renewed until 18/02/2021"

With patch, I get no message. Updated expiration date displays in "Library use" section

I tested the scenario with circulation.pl (Checkout screen) and with moremember.pl (Detail screen)

On the Detail screen it is OK for me to have no message, since the expiration date is displayed on this screen.

On the Checkout screen, after renewing the screen switches to the Detail screen. This behaviour is for me OK as well and I suppose it behaves like this by design.

-----
Question 1: What other scenarios should be tested?
-----
Question 2: Bugzilla asks for patch cmplexity. Is ths a trivial or a small one?
-----
Comment 5 Owen Leonard 2012-12-19 20:59:32 UTC
(In reply to comment #4)
>
> On the Detail screen it is OK for me to have no message, since the
> expiration date is displayed on this screen.

This matches the behavior before the patch.
 
> On the Checkout screen, after renewing the screen switches to the Detail
> screen.

This is incorrect and will be fixed by a revised patch.

> Question 1: What other scenarios should be tested?

In order to be thorough one should really test all the toolbar button operations from at least two pages: circulation.pl and moremember.pl. These two pages cover both categories of pages, circ and patrons, which differently affect the behavior of the patch.

Operations: Edit, add child, duplicate, change password, print (all options), search to hold, renew, set permissions, delete, update child to adult, and export checked-in barcodes.

> Question 2: Bugzilla asks for patch complexity. Is this a trivial or a small
> one?

It's not trivial, so I guess it's small.
Comment 6 Owen Leonard 2012-12-19 21:14:16 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2012-12-21 15:34:54 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-12-26 09:48:37 UTC
QA Comments:

Looks good to me, no regression found.
The patch merges 2 include template files into 1 (members-toolbar.inc), changes the permission for printing (from 'borrowers => 1' to 'circulate => circulate_remaining_permissions') and fixes a typo in the setstatus.pl script (bornum with borrowernumber).

A future improvement could be done by managing the different files which use this include file specifying the destination variable (in order to be redirected to the page where the user was).

Marked to Passed QA
Comment 9 Jonathan Druart 2012-12-26 09:49:25 UTC
Created attachment 14258 [details] [review]
Bug 6261 [Revised] Consolidate patron toolbar include files

The staff client has two almost identical include files which
can be consolidated: circ-toolbar.inc and members-toolbar.inc.
This patch marges the slight differences between them and
eliminates circ-toolbar.inc, that being the one which was used
on fewer pages.

In order to accommodate the different "destination" variable
for operations which redirect back either to circ or patrons,
circulation.pl defines "destination" in the template.

Revision corrects a redirect error in setstatus.pl which
predates this patch but which never showed up until now.

To test, perform various operations from the toolbar on at least
two pages: circulation.pl and moremember.pl.

Operations: Edit, add child, duplicate, change password, print (all
options), search to hold, renew, set permissions, delete, update child
to adult, and export checked-in barcodes. In most cases simply
confirming that the link takes you to the right place is enough.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jared Camins-Esakov 2012-12-27 23:37:45 UTC
This patch has been pushed to master.
Comment 11 Jonathan Druart 2013-01-04 13:40:05 UTC
*** Bug 9279 has been marked as a duplicate of this bug. ***