Bug 33528

Summary: Use template wrapper for tabs: Patron details and circulation
Product: Koha Reporter: Owen Leonard <oleonard>
Component: TemplatesAssignee: Owen Leonard <oleonard>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: minor    
Priority: P5 - low CC: lucas, martin.renvoize, pedro.amorim, samalau, testopia, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35112
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02
Bug Depends on: 32571, 33945    
Bug Blocks: 34294, 35987, 36292, 34402, 34891    
Attachments: Bug 33528: Use template wrapper for tabs: Patron details page
Bug 33528: Use template wrapper for tabs: Patron details page
Screenshot of display
Bug 33528: Use template wrapper for tabs: Patron details page
Bug 33528: Use template wrapper for tabs: Patron details page
Bug 33528: Use template wrapper for tabs: Patron details page
Bug 33528: Use template wrapper for tabs: Patron details page
Bug 33528: (follow-up) Correct selector for event handler
Bug 33528: Fix basic_workflow.t

Description Owen Leonard 2023-04-13 14:56:21 UTC
Continuing the work of Bug 32571, I would like to update the patron details page in the staff client to use the new WRAPPER for tabs
Comment 1 Owen Leonard 2023-05-16 16:05:51 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2023-05-16 16:08:47 UTC
Not expecting this to go into 23.05 but I thought I'd submit it in case someone can start testing it.
Comment 3 Owen Leonard 2023-05-16 16:09:38 UTC
*** Bug 33527 has been marked as a duplicate of this bug. ***
Comment 4 Owen Leonard 2023-05-31 15:47:34 UTC Comment hidden (obsolete)
Comment 5 Sam Lau 2023-05-31 18:27:43 UTC
I ran into some problems testing the club tab in both the check out page and the details page. The club shows up fine, however after clicking the enroll button, the "finish enrollment" button appears on the screen visually plain text rather than a button. And after clicking on the "finish enrollment" text, the club tab essentially disappears but all of the information from the tab is left on the screen. From here if you try to click on another tab, the formatting really gets messed up and the table turns green for some reason. Refreshing the page did put the club tab back in the correct spot and also fixed the formatting, but if you try to cancel your enrollment in the club, a pop up appears that says: "Something went wrong when loading the table." After closing the popup, the formatting of the club tab gets messed up again, with the same problems as described before.
Comment 6 Owen Leonard 2023-06-01 11:44:48 UTC
(In reply to Sam Lau from comment #5)
> I ran into some problems testing the club tab in both the check out page and
> the details page. 

I am not seeing this in my test system. Could you try again? Are there any JavaScript errors in the console?
Comment 7 Lucas Gass 2023-06-01 14:44:57 UTC
(In reply to Owen Leonard from comment #6)
> (In reply to Sam Lau from comment #5)
> > I ran into some problems testing the club tab in both the check out page and
> > the details page. 
> 
> I am not seeing this in my test system. Could you try again? Are there any
> JavaScript errors in the console?

I also recreate this. Steps I used:

1. Apply patch
2. Go to patron checkout or detail tab
3. Click on Clubs tab > Enroll
4. Above the table click 'Finish enrollment' or 'Cancel'
5. Clubs panel now appears in unusual position.
6. Click on another tab (Checkouts, Holds ) and the Clubs panel now has a green background.
Comment 8 Lucas Gass 2023-06-01 14:45:38 UTC
Created attachment 151935 [details]
Screenshot of display
Comment 9 Sam Lau 2023-06-01 14:54:31 UTC
Here is a video of the bug as well: https://www.youtube.com/watch?v=bcNWwtvDuVc
Comment 10 Owen Leonard 2023-06-02 12:56:19 UTC
Created attachment 151980 [details] [review]
Bug 33528: Use template wrapper for tabs: Patron details page

This patch updates the checkout and patron details templates so
that they use the new WRAPPER directive to build tabbed navigation.

The markup for the tab navigation and tab panels is moved into its own
include file since the templates were duplicating essentially the same
code.

To fully test you should have patrons with checkouts, fines, holds,
recalls, and article requests.

Apply the patch and go to the checkout page in the staff interface. With
each tab, test as much functionality as you can within that tab.

- Checkouts
- Holds
- Recalls (with UseRecalls enabled)
- Claims (with a LOST value defined in ClaimReturnedLostValue)
- Restrictions
- Article Requests (with ArticleRequests preference enabled)
- Clubs (with at least one club defined)
- Relatives' checkouts (patron must have a guarantee linked to their
  account).

Perform the same tests on the patron details page. Note that the
"Charges" tab is shown on the patron details page but not the checkout
page. This is not a change made by this patch.
Comment 11 Owen Leonard 2023-06-02 12:57:18 UTC
(In reply to Sam Lau from comment #9)
> Here is a video of the bug as well

Thanks, I'm not sure why I wasn't seeing it before I'd failed to update all the places where the changed DOM structure needed to be updated in the JavaScript
Comment 12 Sam Lau 2023-06-02 14:41:30 UTC
Created attachment 151984 [details] [review]
Bug 33528: Use template wrapper for tabs: Patron details page

This patch updates the checkout and patron details templates so
that they use the new WRAPPER directive to build tabbed navigation.

The markup for the tab navigation and tab panels is moved into its own
include file since the templates were duplicating essentially the same
code.

To fully test you should have patrons with checkouts, fines, holds,
recalls, and article requests.

Apply the patch and go to the checkout page in the staff interface. With
each tab, test as much functionality as you can within that tab.

- Checkouts
- Holds
- Recalls (with UseRecalls enabled)
- Claims (with a LOST value defined in ClaimReturnedLostValue)
- Restrictions
- Article Requests (with ArticleRequests preference enabled)
- Clubs (with at least one club defined)
- Relatives' checkouts (patron must have a guarantee linked to their
  account).

Perform the same tests on the patron details page. Note that the
"Charges" tab is shown on the patron details page but not the checkout
page. This is not a change made by this patch.

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 13 Katrin Fischer 2023-06-08 19:48:04 UTC
First time I noticed this: when doing actions on the tabs, the page reloads and you lose your 'active' tab. Maybe we could take a look at this sometime. But this is the same without the patch set.
Comment 14 Katrin Fischer 2023-06-08 19:48:35 UTC
Created attachment 152216 [details] [review]
Bug 33528: Use template wrapper for tabs: Patron details page

This patch updates the checkout and patron details templates so
that they use the new WRAPPER directive to build tabbed navigation.

The markup for the tab navigation and tab panels is moved into its own
include file since the templates were duplicating essentially the same
code.

To fully test you should have patrons with checkouts, fines, holds,
recalls, and article requests.

Apply the patch and go to the checkout page in the staff interface. With
each tab, test as much functionality as you can within that tab.

- Checkouts
- Holds
- Recalls (with UseRecalls enabled)
- Claims (with a LOST value defined in ClaimReturnedLostValue)
- Restrictions
- Article Requests (with ArticleRequests preference enabled)
- Clubs (with at least one club defined)
- Relatives' checkouts (patron must have a guarantee linked to their
  account).

Perform the same tests on the patron details page. Note that the
"Charges" tab is shown on the patron details page but not the checkout
page. This is not a change made by this patch.

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Owen Leonard 2023-06-13 11:56:23 UTC
Created attachment 152291 [details] [review]
Bug 33528: Use template wrapper for tabs: Patron details page

This patch updates the checkout and patron details templates so
that they use the new WRAPPER directive to build tabbed navigation.

The markup for the tab navigation and tab panels is moved into its own
include file since the templates were duplicating essentially the same
code.

To fully test you should have patrons with checkouts, fines, holds,
recalls, and article requests.

Apply the patch and go to the checkout page in the staff interface. With
each tab, test as much functionality as you can within that tab.

- Checkouts
- Holds
- Recalls (with UseRecalls enabled)
- Claims (with a LOST value defined in ClaimReturnedLostValue)
- Restrictions
- Article Requests (with ArticleRequests preference enabled)
- Clubs (with at least one club defined)
- Relatives' checkouts (patron must have a guarantee linked to their
  account).

Perform the same tests on the patron details page. Note that the
"Charges" tab is shown on the patron details page but not the checkout
page. This is not a change made by this patch.

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Owen Leonard 2023-06-13 11:56:26 UTC
Created attachment 152292 [details] [review]
Bug 33528: (follow-up) Correct selector for event handler

The event handler for restriction deletion needs to be adjusted
according to the new container ID. Without this change the JavaScript
confirmation message didn't appear and restrictions were deleted without
confirmation.
Comment 17 Tomás Cohen Arazi 2023-06-14 12:24:33 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Tomás Cohen Arazi 2023-06-14 15:47:11 UTC
Created attachment 152344 [details] [review]
Bug 33528: Fix basic_workflow.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Martin Renvoize 2023-07-17 11:13:41 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 20 Martin Renvoize 2023-07-17 11:15:45 UTC
This was a fairly involved backport, but I felt it made sense to aid future backports and conflicts there.
Comment 21 Pedro Amorim 2023-07-18 10:52:24 UTC
Does not apply to 22.11.x. Not pushing.