Bug 10533 - basketgroup js functions should in a separated js file
Summary: basketgroup js functions should in a separated js file
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 10522 10716
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-03 08:58 UTC by Jonathan Druart
Modified: 2015-06-04 23:30 UTC (History)
5 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:


Attachments
Bug 10533: basketgroup js functions should in a separated js file (30.99 KB, patch)
2013-07-03 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10533: basketgroup js functions should in a separated js file (30.66 KB, patch)
2013-07-23 15:33 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10533: basketgroup js functions should in a separated js file (30.70 KB, patch)
2013-08-09 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10533: basketgroup js functions should in a separated js file (30.50 KB, patch)
2013-08-13 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10533: move JavaScript functions for basket groups to a separate file (30.73 KB, patch)
2014-04-11 17:06 UTC, Galen Charlton
Details | Diff | Splinter Review
[PASSED QA] Bug 10533: move JavaScript functions for basket groups to a separate file (30.84 KB, patch)
2014-04-18 18:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-07-03 08:58:45 UTC
acqui/basketgroup.tt loads acq.js and a lot of acqui/*.tt loads js functions only used for basketgroup management.
These functions should be in a separated js file.
Comment 1 Jonathan Druart 2013-07-03 09:05:35 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-07-23 15:33:40 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-08-09 15:13:51 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-08-09 15:58:38 UTC
Comparing the acq.js with the new basketgroup.js reveals a bunch of diffs like this:

-    var answer = confirm(MSG_CONFIRM_CLOSE_BASKETGROUP);
+    var answer=confirm(_("Are you sure you want to close this basketgroup?"));

-        alert(MSG_CLOSE_EMPTY_BASKET);
+        alert(_("Why close an empty basket?"));

-    cantprint.innerHTML = MSG_SAVE_BEFORE_PRINTING;
+    cantprint.innerHTML=_("You need to save the page before printing");

In other words, this patch effectively causes a partial regression on bug 7598.

Please try again; it may be better to start from scratch rather than to edit the patch.
Comment 5 Jonathan Druart 2013-08-13 10:19:26 UTC
(In reply to Galen Charlton from comment #4)
> Comparing the acq.js with the new basketgroup.js reveals a bunch of diffs
> like this:
> 
> -    var answer = confirm(MSG_CONFIRM_CLOSE_BASKETGROUP);
> +    var answer=confirm(_("Are you sure you want to close this
> basketgroup?"));
> 
> -        alert(MSG_CLOSE_EMPTY_BASKET);
> +        alert(_("Why close an empty basket?"));
> 
> -    cantprint.innerHTML = MSG_SAVE_BEFORE_PRINTING;
> +    cantprint.innerHTML=_("You need to save the page before printing");
> 
> In other words, this patch effectively causes a partial regression on bug
> 7598.
> 
> Please try again; it may be better to start from scratch rather than to edit
> the patch.

It seems to be a merge error on the signed off patch. I will resubmit a new patch.
Comment 6 Jonathan Druart 2013-08-13 10:19:46 UTC Comment hidden (obsolete)
Comment 7 I'm just a bot 2013-09-29 05:37:18 UTC
Patch applied cleanly, go forth and signoff
Comment 8 Mathieu Saby 2013-10-10 12:51:24 UTC
"- Try all actions for basketgroup ( drag/drop, add, delete, close, print,
reopen, edit, export as csv)."
=> OK for me

M. Saby
Comment 9 Mathieu Saby 2013-10-10 12:53:12 UTC
Is there really a risk of regression on the other pages? Is there a specific point to test?

M. Saby
Comment 10 Jonathan Druart 2013-10-10 13:00:35 UTC
(In reply to mathieu saby from comment #9)
> Is there really a risk of regression on the other pages? Is there a specific
> point to test?

No, it is a c/p code.
Comment 11 I'm just a bot 2013-10-30 07:06:33 UTC
Patch applied cleanly, go forth and signoff
Comment 12 Marcel de Rooy 2014-04-11 15:30:52 UTC
I looked at the move of this code from one file to the other. And made a quick compare between what you deleted and what you created in a git format file.
Unfortunately this compare was not very helpful. Too much differences (including spacing etc.) Makes it harder to verify what happens here..
Comment 13 Galen Charlton 2014-04-11 17:06:13 UTC Comment hidden (obsolete)
Comment 14 Galen Charlton 2014-04-11 17:08:30 UTC
(In reply to M. de Rooy from comment #12)
> I looked at the move of this code from one file to the other. And made a
> quick compare between what you deleted and what you created in a git format
> file.
> Unfortunately this compare was not very helpful. Too much differences
> (including spacing etc.) Makes it harder to verify what happens here..

Indeed.  However, with some use of diff -bw to compare the old acq.js, the new acq.js, and basketgroup.js (and with testing of the acq pages, of course), I was able to convince myself that the patch implements a clean move of those functions.

Plus, it has the benefit of sticking the last active use of YUI by the staff interface in one file.

Signed off.
Comment 15 Katrin Fischer 2014-04-18 18:36:25 UTC
Created attachment 27293 [details] [review]
[PASSED QA] Bug 10533: move JavaScript functions for basket groups to a separate file

This patch moves JavaScript functions used for managing basket groups
to a file.  This has the effect of putting the last (active) use of
the YUI JavaScript library by the staff interface in one file:

  koha-tmpl/intranet-tmpl/prog/en/js/basketgroup.js

Test plan:
- Try all actions for basketgroup ( drag/drop, add, delete, close, print,
reopen, edit, export as csv).
- Check that there is no regression on others acquisition pages:
  * acqui/neworderempty.tt
  * acqui/uncertainprice.tt
  * acqui/addorderiso2709.tt
  * acqui/basketheader.tt
  * admin/aqbudgets.tt
  * admin/aqcontract.tt
  * admin/aqbudgetperiods.tt
  * admin/aqplan.tt
  * suggestion/suggestion.tt

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, passes all tests and QA script.
Comment 16 Galen Charlton 2014-04-18 19:25:56 UTC
Pushed to master.  Thanks, Jonathan!
Comment 17 Fridolin Somers 2014-08-06 10:02:21 UTC
Pushed to 3.14.x, will be in 3.14.10