migrate js/TUI.js and js/util.js to jquery

ASSIGNED
Assigned to

Status

()

Bugzilla
Bugzilla-General
--
enhancement
ASSIGNED
4 years ago
a year ago

People

(Reporter: glob, Assigned: dylanAtHome)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

4 years ago
migrate js/TUI.js and js/util.js to jquery.
hi glob
I am migrating TUI.js to jquery. I am stuck on cookie implementation part. YUI is using subcookies but carhart jquery plugin has no concept of subcookies. how should i implement subcookies in jquery? there is a pluging called ezcookie for subcookie implementation. https://code.google.com/p/ezcookie/ . Should i use that ?
(Reporter)

Comment 2

4 years ago
as TUI is the only place where subcookies are used, i'm not sure it makes sense to change cookie libraries bugzilla-wide for this, especially with ezcookie appearing to be a dead project.

yui's subcookie implementation is just a query string, so you can use jquery's param method to generate the full cookie string, and something like the following to parse it:

    jQuery.deparam = function(params) {
        var o = {};
        if (!params) return o;
        var a = params.split('&');
        for (var i = 0; i < a.length; i++) {
            var pair = a[i].split('=');
            o[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1]);
        }
        return o;
    }
Created attachment 8564096 [details]
tui.js

TUI.js file has been migrated to jquery. json and carhart jquery plugin have been used to implement subccokies.
Created attachment 8564097 [details]
util.js

YUI code in util.js has been converted to jquery code.

Comment 5

4 years ago
Please attach patches, i.e. generate a diff of the old vs new code.
Severity: normal → enhancement
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 8564513 [details] [diff] [review]
patch.diff

TUI.js file has been migrated to jquery. json and carhart jquery plugin have been used to implement subccokies.
Attachment #8564096 - Attachment is obsolete: true
Created attachment 8564514 [details] [diff] [review]
patch1.diff

YUI code in util.js has been converted to jquery code.
Attachment #8564097 - Attachment is obsolete: true

Comment 8

4 years ago
Comment on attachment 8564513 [details] [diff] [review]
patch.diff

Do not forget to request review, else your patch won't appear in reviewers' radar.
Attachment #8564513 - Flags: review?(glob)

Updated

4 years ago
Attachment #8564514 - Flags: review?(glob)

Comment 9

4 years ago
Comment on attachment 8564514 [details] [diff] [review]
patch1.diff

> function bz_toggleClass(anElement, aClass) {
>+    if (element.hasClass(aclass)) {
>+        element.removeClass(aclass);

You pass 'anElement' to the function, but then use 'element'?
Created attachment 8564828 [details] [diff] [review]
patch1_2.diff

YUI code in util.js has been converted to jquery code. Sorry for the silly mistake.
Attachment #8564828 - Flags: review?(glob)

Updated

4 years ago
Attachment #8564514 - Attachment is obsolete: true
Attachment #8564514 - Flags: review?(glob)
(Reporter)

Updated

4 years ago
Attachment #8564513 - Attachment is obsolete: true
Attachment #8564513 - Flags: review?(glob)
(Reporter)

Comment 11

4 years ago
Comment on attachment 8564513 [details] [diff] [review]
patch.diff

oops; you've split the patch into two files.  resurrecting this attachment.
Attachment #8564513 - Attachment is obsolete: false
Attachment #8564513 - Flags: review?(glob)
(Reporter)

Comment 12

4 years ago
Comment on attachment 8564828 [details] [diff] [review]
patch1_2.diff

Review of attachment 8564828 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for this patch, it looks like a nice start.

there are a lot of formatting changes here which are not related to this change; please only change the required parts of the file.
also, for future revisions, please attach a single patch which contains updates to both files.

::: js/util.js
@@ +315,1 @@
>      }

this assumes the element passed in will be a jquery object, which isn't the case.  it's called with the element id, so you need to look that up here.
Attachment #8564828 - Flags: review?(glob) → review-
Created attachment 8565316 [details] [diff] [review]
patch2.diff

YUI.js and TUI.js are now contained inside a single patch file.
Attachment #8564513 - Attachment is obsolete: true
Attachment #8564828 - Attachment is obsolete: true
Attachment #8564513 - Flags: review?(glob)
Attachment #8565316 - Flags: review?(glob)
(In reply to Byron Jones ‹:glob› from comment #12)
> Comment on attachment 8564828 [details] [diff] [review]
> patch1_2.diff
> 
> Review of attachment 8564828 [details] [diff] [review]:
> -----------------------------------------------------------------
 
> 
> ::: js/util.js
> @@ +315,1 @@
> >      }
> 
> this assumes the element passed in will be a jquery object, which isn't the
> case.  it's called with the element id, so you need to look that up here.

I think I am passing a jquery element in bz_toggleClass function as a parameter. I am calling bz_toggleClass in TUI_toggle_class function of TUI.js and passing a jquery element only.
         $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS));
(Reporter)

Comment 15

4 years ago
(In reply to Kapil Bakshi[geekspark] from comment #14)
> I think I am passing a jquery element in bz_toggleClass function as a
> parameter. I am calling bz_toggleClass in TUI_toggle_class function of
> TUI.js and passing a jquery element only.
>          $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS));

that's correct, however bz_toggleClass is used in other places in bugzilla, as well as potentially in extensions.  you cannot change the type of parameter these functions expect.
(In reply to Byron Jones ‹:glob› from comment #15)
> (In reply to Kapil Bakshi[geekspark] from comment #14)
> > I think I am passing a jquery element in bz_toggleClass function as a
> > parameter. I am calling bz_toggleClass in TUI_toggle_class function of
> > TUI.js and passing a jquery element only.
> >          $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS));
> 
> that's correct, however bz_toggleClass is used in other places in bugzilla,
> as well as potentially in extensions.  you cannot change the type of
> parameter these functions expect.

so I shouldn't pass a jquery element here. and call this function as it was being called earlier ?
(Reporter)

Comment 17

4 years ago
Comment on attachment 8565316 [details] [diff] [review]
patch2.diff

Review of attachment 8565316 [details] [diff] [review]:
-----------------------------------------------------------------

> so I shouldn't pass a jquery element here. and call this function as it was being called earlier ?

correct.  this work should be limited to just replacing yui and jquery.
changing function signatures will break code, and other refactoring makes reviewing and testing difficult.

there's a few syntax errors here which prevent your code from running. please ensure you've tested your code prior to submitting a patch.

::: js/TUI.js
@@ +22,4 @@
>  
>  var TUI_alternates = new Array();
>  
> +var json_subcookies = {};

these cookies aren't json, it's a normal js object.
let's call this variable BUGZILLA.subcookies

@@ +32,4 @@
>   * @param className   The name of the CSS class to hide.
>   */
>  function TUI_toggle_class(className) {
> +    $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS));

add spaces around the +
  "." + className

this issue appears multiple times in this patch.

@@ +50,5 @@
> +        if(json_subcookies !=null)
> +        {
> +            if(json_subcookies[className] != 0){
> +                TUI_toggle_class(className);
> +            }

the formatting of this block needs some minor attention.
- please remove the trailing whitespace
- add a space between if and (
- ensure there's a space after =
- ensure there's a space between ) and {
- bring the { up to the same line as the if

@@ +89,5 @@
> +    var cookie = $.deparam($.cookie(TUI_COOKIE_NAME, { raw: true }));
> +    for (cookie_item in cookie) {
> +        if (cookie[cookie_item] == 0) {
> +            var elements = $("."+cookie_item);
> +            elements.each(function{

this has a syntax errox.

@@ +106,5 @@
> +        var pair = a[i].split('=');
> +        o[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1]);
> +    }
> +    return o;
> +}

missing ; after the }

::: js/util.js
@@ +313,3 @@
>   * @param aClass     The name of the CSS class to toggle.
>   */
> +function bz_toggleClass(element, aClass) {

as per my previous review, you cannot change a function's signature.

@@ +321,1 @@
>      }

"aclass" is not defined. it's "aClass"
Attachment #8565316 - Flags: review?(glob) → review-
Created attachment 8567073 [details] [diff] [review]
patch3.diff

All the issues mentioned in comment 17 have been resolved .
Attachment #8565316 - Attachment is obsolete: true
Attachment #8567073 - Flags: review?(glob)
Created attachment 8567089 [details] [diff] [review]
patch3_2.diff

All the issues mentioned in comment 17 have been resolved .

bz_toggleClass function in util.js now has a javascript DOM element whose class is to be toogled  as one of its parameter instead of a jquery one .
Attachment #8567073 - Attachment is obsolete: true
Attachment #8567073 - Flags: review?(glob)
Attachment #8567089 - Flags: review?(glob)
Created attachment 8567101 [details] [diff] [review]
patch3_3.diff

All the issues mentioned in comment 17 have been resolved .

bz_toggleClass function in util.js now has a javascript DOM element (whose class is to be toogled) as one of its parameter instead of a jquery one .
Attachment #8567089 - Attachment is obsolete: true
Attachment #8567089 - Flags: review?(glob)
Attachment #8567101 - Flags: review?(glob)
(Reporter)

Comment 21

3 years ago
Comment on attachment 8567101 [details] [diff] [review]
patch3_3.diff

Review of attachment 8567101 [details] [diff] [review]:
-----------------------------------------------------------------

oops; this patch appears to have been generated using the previous patch as the parent.
please provide a patch using trunk as a base.

::: js/util.js
@@ +311,5 @@
>   * @param anElement  The element to toggle the class on
>   * @param aClass     The name of the CSS class to toggle.
>   */
> +function bz_toggleClass(anElement, aClass) {
> +    anElement.classList.toggle(aClass);

sorry, this still won't work.

look at other places that call this code:

extensions/Voting/template/en/default/hook/admin/products/edit-common-rows.html.tmpl
43:            function() { bz_toggleClass('votes_to_confirm_container',

template/en/default/admin/params/common.html.tmpl
87:            bz_toggleClass("input_[% param.name FILTER html %]", "bz_default_hidden");
88:            bz_toggleClass("table_[% param.name FILTER html %]", "bz_default_hidden");

template/en/default/attachment/list.html.tmpl
23:        bz_toggleClass(rows[i], 'bz_default_hidden');

all of these need to continue to work, which means bz_toggleClass needs to continue to support accepting either an id string *or* a vanilla DOM element as its parameter.

note: just changing the places quoted above isn't sufficient, as this may break third party extensions unnecessarily.

you want something like (untested):
  var el = typeof anElement === 'object' ? $(anElement) : $('#' + anElement);
  el.toggle(aClass);


also note DOM's toggle() method isn't supported on older IE versions (requires v10), so you can't use it.  use jquery's toggle instead.
Attachment #8567101 - Flags: review?(glob) → review-
Created attachment 8571103 [details] [diff] [review]
patch4.diff

Sorry was busy with exams. bz_toggleClass method in util.js is now handling parameters being passed properly.
Attachment #8567101 - Attachment is obsolete: true
Attachment #8571103 - Flags: review?(glob)
Attachment #8571103 - Flags: feedback?(glob)
(Reporter)

Updated

3 years ago
Attachment #8571103 - Flags: feedback?(glob)
Glob Could you please let me know whether my submitted patch is correct or not ?
Flags: needinfo?(glob)
(Reporter)

Comment 24

3 years ago
(In reply to Kapil Bakshi[geekspark] from comment #23)
> Glob Could you please let me know whether my submitted patch is correct or
> not ?

sorry; i have some other higher priority issues that i'm working on right now.
a cursory read of the patch looks good, but you'll have to wait a little longer for a full review i'm sorry.
Flags: needinfo?(glob)
(In reply to Byron Jones ‹:glob› from comment #24)
> sorry; i have some other higher priority issues that i'm working on right
> now.
> a cursory read of the patch looks good, but you'll have to wait a little
> longer for a full review i'm sorry.

Would you be able to review the patch now ?
Flags: needinfo?(glob)
(Reporter)

Comment 26

3 years ago
Comment on attachment 8571103 [details] [diff] [review]
patch4.diff

Review of attachment 8571103 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/TUI.js
@@ +18,4 @@
>   */
>  
>  var TUI_HIDDEN_CLASS = 'bz_tui_hidden';
> +var TUI_COOKIE_NAME = 'TUI';

this change is not required - the equal signs look nice in when in alignment.

@@ +22,5 @@
>  
>  var TUI_alternates = new Array();
>  
> +
> +

this change is not required - only need a single blank line here, not three.

@@ +36,3 @@
>      for (var i = 0; i < elements.length; i++) {
>          bz_toggleClass(elements[i], TUI_HIDDEN_CLASS);
>      }

unfortunately getElementsByClassName isn't supported by IE8.

try something like (completely untested; you'll need to check this works):
$('.' + className).each(function() {
  bz_toggleClass(self, TUI_HIDDEN_CLASS);
}

@@ +62,1 @@
>      if (!link) return;

this will always be true, because $(..) returns an array.
test link.length

(testing should have picked this up)

@@ +77,5 @@
>  }
>  
>  function _TUI_store(aClass, state) {
> +
> +    BUGZILLA.subcookies[aclass] = state;

ReferenceError: aclass is not defined
and then when that's fixed:
TypeError: BUGZILLA.subcookies is undefined

please test your patches.

@@ +87,5 @@
>  }
>  
>  function _TUI_restore() {
> +    var cookie = $.deparam($.cookie(TUI_COOKIE_NAME, { raw: true }));
> +    for (cookie_item in cookie) {

this creates a global 'for' variable.
it should be for (var cookie_item in ..)
Attachment #8571103 - Flags: review?(glob) → review-
(Reporter)

Updated

3 years ago
Flags: needinfo?(glob)
geekspark: are you still working on this? :-)

Gerv
geekspark: I would understand if you were fed up with this, but please do let us know: are you able to carry on working on these patches? We'd like to get migrated to YUI before we release the next version of Bugzilla, and it seems like this bug is necessary for that.

Thanks :-)

Gerv
Flags: needinfo?(technozoom88)
Since there has been no activity for the last two months, I'll assume the assignee has no time for this.

I want us (the project) to learn one thing from this (and similar bugs, with half-completed patches that the patch author didn't test):
Sure, the patch author *should* test, but we have a responsibility as a project to make it easy test patches if we expect patches from volunteers. 

So, thanks :geekspark for the attempt! if you're in our neck of the woods again and would like help getting a working bugzilla dev instance set up -- if you have any issues with that -- let us know!
Assignee: technozoom88 → dylan
Flags: needinfo?(technozoom88)
Attachment #8571103 - Attachment is obsolete: true
Created attachment 8790126 [details] [review]
[bugzilla] dylanwh:bug-1128408 > bugzilla:master
Comment on attachment 8790126 [details] [review]
[bugzilla] dylanwh:bug-1128408 > bugzilla:master

This changes to jquery and moves the storage location of TUI visibility data from a cookie to localStorage. This is a feature -- getting rid of extraneous cookies is a good thing -- at the small inconvenience of forgetting visibility settings.
Attachment #8790126 - Flags: review?(gerv)
gerv: as you have maybe never done a pull request before, you can copy the pull request url and add '.patch' to the end to download a fairly traditional patch file to apply to a recent-ish bugzilla checkout. Splinter was working for pull requests but something is broken and dkl hasn't figured it out yet.
Comment on attachment 8790126 [details] [review]
[bugzilla] dylanwh:bug-1128408 > bugzilla:master

I realize this patch is entirely js, so based on past discussions it's acceptable for kohei to take a stab at reviewing if he has time and interest. So setting r?
Attachment #8790126 - Flags: review?(kohei.yoshino)
(In reply to Dylan Hardison [:dylan] from comment #32)
> gerv: as you have maybe never done a pull request before, you can copy the
> pull request url and add '.patch' to the end to download a fairly
> traditional patch file to apply to a recent-ish bugzilla checkout. Splinter
> was working for pull requests but something is broken and dkl hasn't figured
> it out yet.

Thanks for the tip; that's very useful for my patch-downloading-and-applying script. r=gerv on Github, although it seems that doesn't propagate here?

Gerv
Comment on attachment 8790126 [details] [review]
[bugzilla] dylanwh:bug-1128408 > bugzilla:master

r=gerv if comments on Github are addressed.

Gerv
Attachment #8790126 - Flags: review?(gerv) → review+
Comment on attachment 8790126 [details] [review]
[bugzilla] dylanwh:bug-1128408 > bugzilla:master

This actually won't work because the json structure gets cached per tab. We need to deserialize on every update to keep it in sync.
Attachment #8790126 - Flags: review?(kohei.yoshino)
Attachment #8790126 - Flags: review+
Assignee: dylan → dylan
You need to log in before you can comment on or make changes to this bug.