+++ 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.
LOC Estimation:
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
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 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.
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 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.
(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
> 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 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
(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
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.
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.
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 %]
(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
(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
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
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 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?
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
(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
(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