| Summary: | Consolidate patron toolbar include files | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Owen Leonard <oleonard> |
| Component: | Templates | Assignee: | Owen Leonard <oleonard> |
| Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | jonathan.druart, kyle, veron, vfernandes |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Small patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | 5986 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 6261 - Consolidate patron toolbar include files
Bug 6261 [Revised] Consolidate patron toolbar include files Bug 6261 [Revised] Consolidate patron toolbar include files [SIGNED-OFF] Bug 6261 [Revised] Consolidate patron toolbar include files Bug 6261 [Revised] Consolidate patron toolbar include files |
||
|
Description
Owen Leonard
2011-04-25 13:01:13 UTC
Created attachment 13486 [details] [review] Bug 6261 - 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. To test, perform various operations from the toolbar on different patron-related pages. 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." Created attachment 13787 [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 different patron-related pages. 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? ----- (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. Created attachment 14203 [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. Created attachment 14235 [details] [review] [SIGNED-OFF] 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> 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 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> This patch has been pushed to master. *** Bug 9279 has been marked as a duplicate of this bug. *** |