Bug 426386 - 3.8.5: Check for javascript ability of client for all pages supporting ajax optimizations
3.8.5: Check for javascript ability of client for all pages supporting ajax o...
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: User Interface (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: David Lawrence
2 hours
:
Depends On:
Blocks: RHBZ30UpgradeTracker 406181
  Show dependency treegraph
 
Reported: 2007-12-20 12:47 EST by David Lawrence
Modified: 2013-06-24 00:14 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-27 23:48:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch to add javascript enabled check to Bugzilla code (v1) (6.99 KB, patch)
2008-04-01 17:05 EDT, David Lawrence
no flags Details | Diff
Patch to add javascript enabled check to Bugzilla code (v2) (9.17 KB, patch)
2008-04-02 17:36 EDT, David Lawrence
nelhawar: review+
kbaker: review+
Details | Diff
Patch to add javascript enabled check to Bugzilla code - logincookies (v1) (69.06 KB, patch)
2008-04-16 16:40 EDT, David Lawrence
no flags Details | Diff
Patch to add javascript enabled check to Bugzilla code - logincookies (v1) (11.12 KB, patch)
2008-04-16 17:21 EDT, David Lawrence
dkl: review? (kbaker)
dkl: review? (nelhawar)
Details | Diff

  None (edit)
Description David Lawrence 2007-12-20 12:47:18 EST
+++ This bug was initially created as a clone of Bug #406181 +++

Description:
On certain pages Ajax optimizations are used to speed up page loading time. Red
Hat has a high number of components for certain products. The component lists
cause a large amount of HTML having to be downloaded by the client browser. The
Ajax functions allow the UI to only show the components needed depending on
which products are selected.

Function Requirements:
Javascript code has been added to specific cgi's to detect whether javascript is
enabled for the clients browser. We then use this information to decide whether
to display the ajax optimizations or to display the legacy page.
Comment 1 David Lawrence 2007-12-20 12:57:55 EST
LOC Estimation:
Comment 2 David Lawrence 2007-12-20 13:02:58 EST
LOC Estimation:

js/cookies.js: 47
process_bug.cgi: 6
show_bug.cgi: 6
query.cgi: 6
templates: 2
selenium test to determine proper setting of cookie for each page load: 2 hours

LOC Total: 67
Comment 3 David Lawrence 2008-04-01 17:05:01 EDT
Created attachment 299968 [details]
Patch to add javascript enabled check to Bugzilla code (v1)

I have created a patch to determine whether a user's browser supports
javascript. It adds a new column to the profiles table called javascript that
is either 1 or 0  based on whether the Bugzilla_javascript cookie is found. The
cookie check happens in Bugzilla/Auth/Verify/Cookie.pm where it will update the
javascript value in profiles for the user who is verified. The
Bugzilla_javascript cookie is then deleted for verification again next time. In
template/en/default/global/header.html.tmpl setcookie('Bugzilla_javascript', 1)
is called in the <body onload> tag. This part is similar to how we do it in
2.18. The difference is we are now storing it in the profiles table so that it
becomes part of the normal User.pm object and can be used anywhere that
Bugzilla->user is available. If the browser does not support javascript,
setcookie() fails to work and profiles.javascript is always 0.

Please review

Dave
Comment 4 David Lawrence 2008-04-02 16:08:51 EDT
Comment on attachment 299968 [details]
Patch to add javascript enabled check to Bugzilla code (v1)

Scratch this one, only works for logged in users which is not fully what we
need. This needs to also work with users who are not logged in so that they can
still take advantage of the query.cgi enhancements.

Working on new patch.
Comment 5 David Lawrence 2008-04-02 17:36:08 EDT
Created attachment 300139 [details]
Patch to add javascript enabled check to Bugzilla code (v2)

New patch. Set up User.pm now to properly have setters and accessors instead of
hitting the DB directly in Bugzilla::Auth::Login::Cookie::get_login_info().
Also the javascript flag should now be set properly even for users who are not
logged in. Also fixed a bug with setcookie() where I wasn't specifying exact
domain and path info so the cookie couldn't be found properly when it came time
to clear it. 

Please review.
Dave
Comment 6 Kevin Baker 2008-04-05 06:17:40 EDT
Comment on attachment 300139 [details]
Patch to add javascript enabled check to Bugzilla code (v2)

This is one bug I would really like to see done in the upstream. There are 100
different ways it could be implemented and it would benefit from the attention
of many eyeballs. 

Regarding the patch, it looks sane, the only issue I have with it is that we
are customising again. But you've all heard that before :D Still, we  have
deadlines so I guess the best we can do is create a bug in the upstream and
hope that it gets pulled into the head for ver4.
Comment 7 David Lawrence 2008-04-05 16:45:16 EDT
(In reply to comment #6)
> (From update of attachment 300139 [details] [edit])
> This is one bug I would really like to see done in the upstream. There are 100
> different ways it could be implemented and it would benefit from the attention
> of many eyeballs. 
> 

I agree. I can see the comments already. "Why do you have so many components
anyway?", "You should code the page in such a way that it doesn't need the
detection", "Why is Red Hat doing so much crack? ;)", etc.

> Regarding the patch, it looks sane, the only issue I have with it is that we
> are customising again. But you've all heard that before :D Still, we  have
> deadlines so I guess the best we can do is create a bug in the upstream and
> hope that it gets pulled into the head for ver4.
 
Will do and see what happens. One of the reasons I decided to store the value in
the database which is different that what we do in 2.18 is due to "RT #18340:
Bugzilla - record which users have javascript cookie set". We can then look back
at the profiles table and get an idea on how many users visit our site with 
javascript enabled. Yet to be seen how useful the stats are, I can already
assume that most people are connecting with either Firefox/Safari/IE and the
majority of them will have javascript enabled. It might be useful to also store
the UserAgent as well in the profiles table.

As for the patch, I wonder if this would be best stored in the logincookies
table instead of profiles. Then we could track javascript usage per session.
With it in the profiles table it just gets overwritten each time.

What do you think

Dave 

Comment 8 Kevin Baker 2008-04-07 10:39:29 EDT
> majority of them will have javascript enabled. It might be useful to also 
store
> the UserAgent as well in the profiles table.

That info should already be in the apache logs. So do we really want to store 
it in the profiles table? 
 
> As for the patch, I wonder if this would be best stored in the logincookies
> table instead of profiles. Then we could track javascript usage per session.
> With it in the profiles table it just gets overwritten each time.
> 
> What do you think

Depends how you want to model it. En/Disabling javascript per session points 
to the logincookies table. The profiles.javascript field could be indicate 
users general preference towards javascript. 

Could be too complicated and most people's heads will explode (WTF? I thought 
I enabled javascript)?  

Comment 9 Noura El hawary 2008-04-10 10:24:59 EDT
Comment on attachment 300139 [details]
Patch to add javascript enabled check to Bugzilla code (v2)

Hi Dave, patch looks good to me too. on 2 notes below.

>Index: Bugzilla/Auth.pm
>===================================================================
>+        # REDHAT EXTENSION START 426386
>+        # Set javascript flag to enable UI enhancements
>+        $user->set_javascript($result->{javascript}) if exists $result->{javascript};
>     }
>

shall we here update the database by $user->update so it will be:

	if (exists $result->{javascript}){
	    $user->set_javascript($result->{javascript});
	    $user->update;
	}



>Index: Bugzilla/User.pm
>===================================================================
>
>+# REDHAT EXTENSION START 426386
>+sub set_javascript { $_[0]->set('javascript', $_[1]) }

just missing a semicolon at the end of the statement
Comment 10 David Lawrence 2008-04-10 10:39:09 EDT
(In reply to comment #9)
> (From update of attachment 300139 [details] [edit])
> Hi Dave, patch looks good to me too. on 2 notes below.
> 
> >Index: Bugzilla/Auth.pm
> >===================================================================
> >+        # REDHAT EXTENSION START 426386
> >+        # Set javascript flag to enable UI enhancements
> >+        $user->set_javascript($result->{javascript}) if exists
$result->{javascript};
> >     }
> >
> 
> shall we here update the database by $user->update so it will be:
> 
> 	if (exists $result->{javascript}){
> 	    $user->set_javascript($result->{javascript});
> 	    $user->update;
> 	}

I dont use $user->update here since this section is only for users who are not
logged in and get the default user object which has $user->id == 0. This $user
object only lives for the live of the current page. $user->update is called
however after I do $user->set_javascript($result->{javascript}) in
Bugzilla/Auth/Verify.pm line 129  which is for when the user is logged in.  

> 
> >Index: Bugzilla/User.pm
> >===================================================================
> >
> >+# REDHAT EXTENSION START 426386
> >+sub set_javascript { $_[0]->set('javascript', $_[1]) }
> 
> just missing a semicolon at the end of the statement


Thanks
Comment 11 Kevin Baker 2008-04-16 12:46:36 EDT
Dave,

what about your idea of tracking the javascript in the logincookies table?

profiles.javascript     - indicates a general user preference
logincookies.javascript - indicates the preference for the session.

Time constraints may make this decision for you. I figure storing it in the 
profiles table will work for 99% of the use cases. The logincookies would 
satisfy that rare animal that wants to run two or more sessions 
simultaneously, some with js off, some with js on. Surely a rare usecase? And 
probably not worth the effort of a re-write now. 

I think I just convinced myself that the wise course is to go with this patch 
for now. Unless anyone complains majorly we put off work on the new improved 
version until it can be done with the upstream.

Comment 12 Kevin Baker 2008-04-16 12:50:21 EDT
Another thought.

Is it even worth updating the profiles table with the javascript value? On the 
plus side we record a history of who is using javascript, valuable info. On 
the minus we have to update the profiles table on each click for logged in 
users. Another pointer that it belongs on the logincookies table as we update 
that anyhow, right?

Just thinking out aloud.
Comment 13 Kevin Baker 2008-04-16 13:13:40 EDT
Oh, and don't forget to remove from 
template/en/default/global/header.html.tmpl

+[%# REDHAT EXTENSION 426382 Debugging purposes only, will remove when checked 
in %]
+[% IF user.javascript %]
+<div id="message">JAVASCRIPT ENABLED</div>
+[% END %]
Comment 14 David Lawrence 2008-04-16 15:08:33 EDT
(In reply to comment #11)
> Dave,
> 
> what about your idea of tracking the javascript in the logincookies table?
> 
> profiles.javascript     - indicates a general user preference
> logincookies.javascript - indicates the preference for the session.
> 
> Time constraints may make this decision for you. I figure storing it in the 
> profiles table will work for 99% of the use cases. The logincookies would 
> satisfy that rare animal that wants to run two or more sessions 
> simultaneously, some with js off, some with js on. Surely a rare usecase? And 
> probably not worth the effort of a re-write now. 

XMLRPC clients.

> 
> I think I just convinced myself that the wise course is to go with this patch 
> for now. Unless anyone complains majorly we put off work on the new improved 
> version until it can be done with the upstream.
> 

Let me quickly look and see what type of work it would take to do this. It
may be better to put in the logincookies table. We have to update the
logincookies.lastused date column anyway.

Dave
Comment 15 David Lawrence 2008-04-16 15:10:27 EDT
(In reply to comment #13)
> Oh, and don't forget to remove from 
> template/en/default/global/header.html.tmpl
> 
> +[%# REDHAT EXTENSION 426382 Debugging purposes only, will remove when checked 
> in %]
> +[% IF user.javascript %]
> +<div id="message">JAVASCRIPT ENABLED</div>
> +[% END %]

Yes of course. Mainly there for debugging purposes so I could see which screens
we knew about javascript.

Dave
Comment 16 David Lawrence 2008-04-16 16:40:47 EDT
Created attachment 302670 [details]
Patch to add javascript enabled check to Bugzilla code - logincookies (v1)

Attaching a patch that uses logincookies.javascript instead of the profiles
table. It still updates the User.pm object with the value for user.javascript
but no longer stores it in the profiles table.

Please review

Dave
Comment 17 David Lawrence 2008-04-16 17:21:57 EDT
Created attachment 302676 [details]
Patch to add javascript enabled check to Bugzilla code - logincookies (v1)

Oops. wrong patch. Let's try this again.

Dave
Comment 18 Kevin Baker 2008-04-16 18:12:11 EDT
Comment on attachment 302676 [details]
Patch to add javascript enabled check to Bugzilla code - logincookies (v1)

So the difference is that instead of adding a db field to profiles we add it to
logincookies. BUT the javascript boolean is still available by querying the
javascript method on the User object. Correct? If so, should set_javascript
still be calling set?
Comment 19 Noura El hawary 2008-04-17 08:07:02 EDT
Dave I tried testing this on bugdev. Although i logged in and i have javascript
enabled ,, I checked the bz-db1-test bugs database logincookies table and
couldn't see that javascript field was set to 1 for my sessions, it was set to
0. am i misunderstanding anything? 

Noura
Comment 20 David Lawrence 2008-04-17 10:45:55 EDT
(In reply to comment #18)
> (From update of attachment 302676 [details] [edit])
> So the difference is that instead of adding a db field to profiles we add it to
> logincookies. BUT the javascript boolean is still available by querying the
> javascript method on the User object. Correct? If so, should set_javascript
> still be calling set? 
> 

I removed the profiles.javascript column from the UPDATE_COLUMNS sub so even
though I am called set_javascript() which calls set() it will not try to update
the value in the profiles table. It will however update the value in the current
User.pm instance so that for all of the code it looks like another attribute of
the user.

Dave
Comment 21 David Lawrence 2008-04-17 10:47:08 EDT
(In reply to comment #19)
> Dave I tried testing this on bugdev. Although i logged in and i have javascript
> enabled ,, I checked the bz-db1-test bugs database logincookies table and
> couldn't see that javascript field was set to 1 for my sessions, it was set to
> 0. am i misunderstanding anything? 
> 
> Noura

My fault. I did not have the patch installed on bugdev so that will explain why
it was not working. i am going to go ahead and check this into cvs since some of
the other patches depend on it and this is lower danger than the other patches.

Dave

Note You need to log in before you can comment on or make changes to this bug.