Bug 9130 - Remove old javascript from Parcel
Summary: Remove old javascript from Parcel
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: David Cook
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-21 21:05 UTC by David Cook
Modified: 2013-05-23 06:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 9130 - Remove old javascript from Parcel (4.24 KB, patch)
2012-11-21 21:20 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9130 - Remove old javascript from Parcel (4.25 KB, patch)
2012-11-22 16:32 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu (2.00 KB, patch)
2012-12-04 01:00 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu (2.00 KB, patch)
2012-12-04 13:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 9130 - Remove old javascript from Parcel (4.24 KB, patch)
2012-12-04 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu (2.05 KB, patch)
2012-12-04 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9130 - Remove old javascript from Parcel (4.30 KB, patch)
2012-12-04 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu (2.05 KB, patch)
2012-12-04 14:00 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 David Cook 2012-11-21 21:05:57 UTC
When receiving orders, the old javascript would show the following above the tables for pending orders and received orders:

"Only the first 5 items are displayed. Click here to show all 'X' items."

With the implementation of the Jquery Datatables plugin, this text and the 4 JS functions associated with it are now pointless (since users should be using the datatable toolbar) and problematic.

The problem is that the Jquery toolbar says "Showing 1 to 10 of X", while in fact it is actually only showing 5 items, because that other JS takes place after the Jquery. 

While not a crippling bug, it's certainly irritating and misleading.
Comment 1 David Cook 2012-11-21 21:20:22 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-11-22 16:32:04 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2012-11-22 16:34:28 UTC
(In reply to comment #2)
> While not a crippling bug, it's certainly irritating and misleading.

Yes it is! Thank you for the patch ;)
Comment 4 Jonathan Druart 2012-11-23 11:17:25 UTC
I add Kyle to the CC list of this bug.

Do you agree with the current behaviour ?
Comment 5 Kyle M Hall (khall) 2012-12-03 17:16:38 UTC
(In reply to comment #4)
> I add Kyle to the CC list of this bug.
> 
> Do you agree with the current behaviour ?

I would tend to agree with you here. I do have one question. Does the table have a pulldown to choose the number of results displayed? If so, can we make that 'sticky' via a cookie instead. That way, the functionality isn't totally lost. I don't have a Koha installation set up for me to look at right now, or I could probably answer the question myself.
Comment 6 David Cook 2012-12-04 01:00:25 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall (khall) 2012-12-04 13:03:06 UTC
Fantastic! Good work!

Kyle

(In reply to comment #6)
> Created attachment 13866 [details] [review]
> Bug 9130 follow up: add statesave cookie and "all" option for parcel
> drop-down menu
> 
> This patch sets the bStateSave parameter for the "Pending orders"  and
> "Already received" Datatables in Acquisitions, which uses a cookie to save
> the "pagination information, display length, filtering and sorting".
> 
> It also sets the iCookieDuration parameter to 1000 days (the default value
> is 2 hours otherwise).
> 
> It also adds an "all" option to the dro-down menu using the aLengthMenu
> parameter.
> 
> --
> 
> Kyle, this should preserve that "sticky" functionality of which you spoke :)
Comment 8 Kyle M Hall (khall) 2012-12-04 13:03:52 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-12-04 13:56:34 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-12-04 13:57:55 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-12-04 13:59:00 UTC
Thanks David for your followup.

1 little remark: when you add a value (60*60*24*1000), could you add directly the result (86400000)? Because like this the calculation is not performed each time.
It is JS code so it is not crucial and not blocker.

Marked as Passed QA.
Comment 12 Jonathan Druart 2012-12-04 14:00:13 UTC
Created attachment 13875 [details] [review]
Bug 9130 - Remove old javascript from Parcel

When receiving orders, the old javascript would show the following above
the tables for pending orders and received orders:

"Only the first 5 items are displayed. Click here to show all 'X'
items."

With the implementation of the Jquery Datatables plugin, this text and
the 4 JS functions associated with it are now pointless (since users
should be using the datatable toolbar) and problematic.

The problem is that the Jquery toolbar says "Showing 1 to 10 of X",
while in fact it is actually only showing 5 items, because that other JS
takes place after the Jquery.

While not a crippling bug, it's certainly irritating and misleading.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Jonathan Druart 2012-12-04 14:00:19 UTC
Created attachment 13876 [details] [review]
Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu

This patch sets the bStateSave parameter for the "Pending orders"  and "Already received" Datatables in Acquisitions, which uses a cookie to save the "pagination information, display length, filtering and sorting".

It also sets the iCookieDuration parameter to 1000 days (the default value is 2 hours otherwise).

It also adds an "all" option to the dro-down menu using the aLengthMenu parameter.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Jared Camins-Esakov 2012-12-05 13:54:05 UTC
This patch has been pushed to master.
Comment 15 David Cook 2012-12-05 19:05:59 UTC
(In reply to comment #11)
> Thanks David for your followup.
> 
> 1 little remark: when you add a value (60*60*24*1000), could you add
> directly the result (86400000)? Because like this the calculation is not
> performed each time.
> It is JS code so it is not crucial and not blocker.
> 
> Marked as Passed QA.

Good point, Jonathan. In retrospect, I could've just added the result. When looking through other examples (like in jquery.cookie.min.js), I noticed them writing out the whole calculation, so I just copied that at the time. I don't know if it's worth a follow-up, but certainly something to keep in mind for future Datatable mods.