Bug 426386
| Summary: | 3.8.5: Check for javascript ability of client for all pages supporting ajax optimizations | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Community] Bugzilla | Reporter: | David Lawrence <dkl> | ||||||||||
| Component: | User Interface | Assignee: | David Lawrence <dkl> | ||||||||||
| Status: | CLOSED NEXTRELEASE | QA Contact: | |||||||||||
| Severity: | medium | Docs Contact: | |||||||||||
| Priority: | high | ||||||||||||
| Version: | 3.2 | ||||||||||||
| Target Milestone: | --- | ||||||||||||
| Target Release: | --- | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | 2 hours | ||||||||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||||||||
| Doc Text: | Story Points: | --- | |||||||||||
| Clone Of: | Environment: | ||||||||||||
| Last Closed: | 2008-05-28 03:48:52 UTC | Type: | --- | ||||||||||
| Regression: | --- | Mount Type: | --- | ||||||||||
| Documentation: | --- | CRM: | |||||||||||
| Verified Versions: | Category: | --- | |||||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||||
| Embargoed: | |||||||||||||
| Bug Depends On: | |||||||||||||
| Bug Blocks: | 406071, 406181 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
David Lawrence
2007-12-20 17:47:18 UTC
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 |