TPAC: Ability to add titles to permanent book lists

Bug #1003409 reported by Jason Stephenson on 2012-05-23
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

My proposed implementation basically covered the following:

Add an ou setting to enable the warning when adding to a temporary list: opac.temporary_list.warn. This will function so that if it is not set or set to off, then the software behaves as it currently does for adding to a temporary list.

Add an actor usr setting for holding the name of a default book list: opac.my_list.default.

Add a checkbox on the my list interface to indicate that a certain list should be the default list for adding titles.

Modify opac/record and opac/results so that if a patron is logged in their my lists will populate a drop down with the default list selected. Clicking on add to my list will add to the list currently selected in the drop down.

If a logged in patron does not have a list, then the behavior will be the same as for a patron who is not logged in.

If a patron is not logged in, the add to my list will appear the same as it does currently.

Clicking on add to my list will check the ou setting mentioned in above in 1. If that setting is on it will display a warning page to the patron that the item has been added to a temporary list.

There will be a checkbox on this warning page so that if it is checked when the patron dismisses the warning page, a session cookie will be set to indicate that the patron does not want any more warnings about the temporary list.

Logged in users will be able to skip all such warnings via a user setting: opac.temporary_list.warn.

The above was amended at an impromptu meeting held during the 2012 International Conference in Indianapolis:

We selected the attached mock-up as the model.

We would like some type of border around the dropdown so that it doesn’t blend into the background (a css class for this element will allow us to customize this).

The behavior for non-logged-in users will essentially be the same as described in [the] proposal.

For logged-in users, the temporary list will appear first followed by the patron’s existing lists. The default list will appear at the top.

There will also be an option to "add to new list" at the bottom of the dropdown.

There will be a cap on the number of lists that display here. If a user owns more lists, there will be a "See All" option that will lead them to a new page where they can select the right list.

There will be a separate css class for the "Temporary List" element and the "Add to New List" element.

WIP: http://git.mvlcstaff.org/?p=jason/ILS.git;a=shortlog;h=refs/heads/add-to-permanent-bookbag

When done, I'll rebase and push to the working repository on git.evergreen-ils.org.

Jason Stephenson (jstephenson) wrote :
Jason Stephenson (jstephenson) wrote :

The branch is ready to go and signed off in user/dyrcona/add-to-permanent-bookbag on the work repository.

This code was funded by MassLNC http://masslnc.cwmars.org/

Changed in evergreen:
status: In Progress → New
tags: added: pullrequest
Changed in evergreen:
milestone: none → 2.3.0-alpha2
Jason Stephenson (jstephenson) wrote :

I have added the fix from 10021287 and rebased down to 1 commit.

The single commit branch is at working/user/dyrcona/lp1003409

IMO, this is the branch that should be merged. Anyone who wants to see the historical development of the code can look at the other branch.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Dan Scott (denials) wrote :

The code that has been added to record/summary.tt2 and results/table.tt2 is large and almost identical, thus a great candidate for factoring out into one common template that would get included into each of the respective files.

There would be a little bit of upfront work to grab the right context variables (ctx.bre_id vs. rec_id) and resolve a few CSS classes, but I think that factoring it out into a common template would be a big win & would avoid having to duplicate changes for fixes & enhancements down the road. I'm willing to work on this if you don't have the time or interest.

Dan Scott (denials) wrote :

Okay, please check out working/user/dbs/permanent_bookbag - I think I've nailed down the code deduping for the bookbag actions template.

One thing that didn't work during my quick testing was, after I cleared the user setting for "do not warn me when I add to a temporary list" and saved my user preference, I didn't get warnings when I did that. Maybe a cached setting or something? Or maybe a peculiarity of my test instance.

Otherwise, the code worked as promised and is a great addition; I'll be happy to push it to master if you can sign off on the deduping commit. Thanks Jason!

Changed in evergreen:
milestone: 2.3.0-alpha2 → 2.3.0-beta1
Ben Shum (bshum) wrote :

Tested and seems to work well.

Added my signoff at working/user/bshum/permanent_bookbag

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/bshum/permanent_bookbag

Dan Scott (denials) wrote :

Thanks Ben! Pushed to master (and... gah, I duplicated my own sign-off... heh).

Changed in evergreen:
status: New → Fix Committed
Ben Shum (bshum) wrote :

I missed something in my testing, it looks like paging for the "my lists" (having more than 10 lists) is broken with the changes. The issue seems to stem from a check in lists.tt2 to look and see if limit < ctx.bookbag_count. That IF check wraps around the spans containing navigation for previous/next in lists. The check isn't working, so it never shows the navigation for list paging.

Dan Scott (denials) wrote :

Great catch, Ben. I can also confirm that ctx.bookbag_count is not defined in https://localhost/eg/opac/myopac/lists - it's like _load_lists_and_settings isn't getting invoked (which it doesn't seem to, in the context of Account.pm; only via Record.pm and Search.pm).

Dan Scott (denials) wrote :

So, I went ahead and added the bookbag_count to Account.pm::load_myopac_bookbags(); it's almost duplicated code, but solves the immediate problem.

Also, I addressed some previously existing bookbag paging issues in the template while I was at it.

See user/dbs/lp1003409_paging_fixes in working

Ben Shum (bshum) wrote :

Works to bring back paging on our test systems.

Thanks Dan!

Sign-off: user/bshum/lp1003409_paging_fixes

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/bshum/lp1003409_paging_fixes

Dan Scott (denials) wrote :

Thanks Ben! Pushed (and this time without signing off on it again) :)

Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers