Last Comment Bug 365890 - Searches shared by users with bless rights are in the footer by default, with no warning
: Searches shared by users with bless rights are in the footer by default, with...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 2.23.3
: All All
: -- normal (vote)
: Bugzilla 3.0
Assigned To: Teemu Mannermaa (:wicked)
: default-qa
:
Mentors:
Depends on: 352403
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-03 23:21 PST by Dave Miller [:justdave] (justdave@bugzilla.org)
Modified: 2008-01-04 06:17 PST (History)
6 users (show)
LpSolit: approval+
LpSolit: approval3.0+
mkanat: blocking3.0.1+
mkanat: blocking3.0-
wurblzap: documentation+
wurblzap: documentation3.0+
LpSolit: testcase?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
partially tested patch, v1 (4.00 KB, patch)
2007-03-29 13:02 PDT, Frédéric Buclin
wicked: review-
Details | Diff | Splinter Review
Tested patch, V2 (4.22 KB, patch)
2007-07-19 04:24 PDT, Teemu Mannermaa (:wicked)
LpSolit: review-
Details | Diff | Splinter Review
Tested patch with arrays, V2.1 (5.10 KB, patch)
2007-07-19 08:09 PDT, Teemu Mannermaa (:wicked)
LpSolit: review+
Details | Diff | Splinter Review
Tested patch with arrays, V2.2 (5.60 KB, patch)
2007-07-20 23:46 PDT, Teemu Mannermaa (:wicked)
wicked: review+
Details | Diff | Splinter Review
Tested patch with arrays, v2.2 for 3.0 branch (4.79 KB, patch)
2007-07-21 00:09 PDT, Teemu Mannermaa (:wicked)
LpSolit: review+
Details | Diff | Splinter Review
Docs patch (863 bytes, patch)
2007-07-21 04:08 PDT, Marc Schumann [:Wurblzap]
LpSolit: review+
Details | Diff | Splinter Review

Description Dave Miller [:justdave] (justdave@bugzilla.org) 2007-01-03 23:21:02 PST
A great number of people (including myself) were quite surprised the other day to find two new queries in their footer that I had shared, and none of them had subscribed to.  They just automatically showed up for everyone in the group I shared with.

According to a comment at line 452 in userprefs.cgi:

            # If we're sharing our query with a group we can bless, we 
            # subscribe direct group members to our search automatically.
            # Otherwise, the group members need to opt in. This behaviour 
            # is deemed most likely to fit users' needs.

First off, there is no warning in the UI that this is going to happen.  I had no clue until people started asking me "why did I get these new searches without asking for them?"  Second, while it's nice for someone with bless capability to be able to force searches into peoples' footers, it should be an option, and not an automatic.  I shouldn't be forced to force all my searches on everyone just because I'm an admin.
Comment 1 Frédéric Buclin 2007-01-04 04:59:57 PST
mkanat is the one who wanted this behavior, see bug 352403. But I agree with justdave, there should be a checkbox for blessers to decide what to do with their shared queries.
Comment 2 Max Kanat-Alexander 2007-01-04 12:59:57 PST
Yes, agreed, there should be a checkbox. I was thinking exactly this, the other day.
Comment 3 Reed Loden [:reed] (use needinfo?) 2007-03-29 10:49:16 PDT
Josh wanted to share his "Cocoa Blockers" search with people, but since he has bless rights for "editbugs", it showed up in everybody's footer. This is very annoying. :(
Comment 4 Frédéric Buclin 2007-03-29 13:02:14 PDT
Created attachment 260043 [details] [diff] [review]
partially tested patch, v1
Comment 5 Max Kanat-Alexander 2007-03-29 15:31:22 PDT
I understand that this is irritating, but at this point in the release cycle I don't want to block 3.0 on it. I think it would be OK for 3.0.1, even though it adds a feature (the ability for people to choose whether or not they want their shared stuff to show up by default).
Comment 6 Frédéric Buclin 2007-03-30 07:08:08 PDT
I will approve it for 3.0 if this patch is reviewed on time.
Comment 7 Teemu Mannermaa (:wicked) 2007-04-06 21:17:17 PDT
Comment on attachment 260043 [details] [diff] [review]
partially tested patch, v1

Checkbox logic has following oddities:
 1) Checkbox remains selected when selecting non-blessable group.
 2) There's no way to force again because all controls are disabled after page load. (Workaround is to select non-blessable group and then reselect blessable group.)
 3) Would it be better to hide and not only disable the whole checkbox when non-blessable group is selected?

>Index: userprefs.cgi
>===================================================================
>+    $vars->{'bless_group_ids'} = [map {$_->{'id'}} @{$user->bless_groups}];

Add this variable to template interface docs too unless you remove it completely (see below).

>Index: template/en/default/account/prefs/saved-searches.html.tmpl
>===================================================================
>+  <script type="text/javascript">

Would be nice to have this new JS block in an external file as we are trying separate JS from templates.

>+    function update_checkbox(group) {
>+      var bless_groups = [[% bless_group_ids.join(",") FILTER js %]];
>+      // IE6 doesn't support indexOf, so we use this hack.

Bad IE, but why not use search or match for this string too? Better yet call user.can_bless for the group ID so that 1) the check logically matches what you do later and 2) have no need to use template code or extra parameter. Would make the code simpler too.

>+    // Disable all checkboxes by default.
>+    function disable_all() {
..
>+    window.onload = disable_all;

Is this really necessary? Can't you just set this property on the control itself?

>+            <select name="share_[% q.id FILTER html %]"
>+            [% IF user.can_bless %]onchange="update_checkbox(this);"[% END %]>

Nit: There's no space before onchange in the generated HTML.

>+              <input type="checkbox" id="force_[% q.id FILTER html %]"
>+                     name="force_[% q.id FILTER html %]" value="1">
>+              <label for="force_[% q.id FILTER html %]">force</label>

Label for this checkbox is confusing. Are we forcing search to be shared to the group or what?

Maybe something like "Subscribe members" or "Add to footer of group members" would be better. First suggestion uses term subscribe that is not what's used elsewhere in UI and second one is a bit long so you might need to come up with something better.
Comment 8 Teemu Mannermaa (:wicked) 2007-07-19 04:24:07 PDT
Created attachment 272951 [details] [diff] [review]
Tested patch, V2

Here's an updated patch that addresses most of my review comments. There is no way to hide the control so I still just disable them.

In the end there was so little JS code there that having them in a separate file seems like a waste. Maybe someday we should create some misc JS file that can house all of these little scriptlets we have all over the pages.

There is no way to call Perl code like user.can_bless from JS. So I use indexOf() function on the template generated string of blessable group ids and it seems to work in FF2 and IE7. According to http://www.quirksmode.org/js/strings.html#indexof it's one of the oldest and widely used methods so I would be surprised if it didn't work in IE6 too. It seems LpSolit was talking about using indexOf() with arrays and that's not supported in IE. For that we have bz_isValueInArray() function in js/util.js anyway.
Comment 9 Frédéric Buclin 2007-07-19 05:57:35 PDT
Comment on attachment 272951 [details] [diff] [review]
Tested patch, V2

>Index: template/en/default/account/prefs/saved-searches.html.tmpl

>+                  var groups = "[% bless_group_ids.join(",") FILTER js %]";
>+                  if (!this.value || groups.indexOf(this.value) == -1) {

If you can bless groups 10 and 11 and the current selected value is 1, indexOf() won't return -1. That's the reason I used arrays.

Everything else looks good.
Comment 10 Teemu Mannermaa (:wicked) 2007-07-19 08:09:35 PDT
Created attachment 272974 [details] [diff] [review]
Tested patch with arrays, V2.1

Argh, I knew I was forgetting something. My test groups were 6 and 18 so no wonder it was working nicely in my tests. Here's a version with arrays and previously mentioned utility function.
Comment 11 Frédéric Buclin 2007-07-19 17:15:54 PDT
Comment on attachment 272974 [details] [diff] [review]
Tested patch with arrays, V2.1

>Index: template/en/default/account/prefs/saved-searches.html.tmpl

>+              [% IF user.can_bless %] onchange='
>+                  var checkbox = document.getElementById(
>+                                 this.name.replace(/share_(\d+)/, "force_$1"));
>+                  var groups = [[% bless_group_ids.join(",") FILTER js %]];
>+                  if (bz_isValueInArray(groups, this.value)) {
>+                      checkbox.disabled = false;
>+                  } else {
>+                      checkbox.disabled = true;
>+                      checkbox.checked = false;
>+                  }'[% END %]>

I think you should move this JS code outside this <select>. First because it's more readable than this "JS injection", and secondly because this JS code appears once per saved search, making the page bigger (look at the source code of the generated HTML page).


>+            [% IF user.can_bless %]
>+              <input type="checkbox" id="force_[% q.id FILTER html %]"
>+                     name="force_[% q.id FILTER html %]" value="1"
>+                     [% " disabled"
>+                        IF !bless_group_ids.grep("^$q.shared_with_group.id\$").0
>+                     %]>
>+              <label for="force_[% q.id FILTER html %]">Add to footer</label>
>+            [% END %]

We should make clear 1) that we are talking about the footer of other members of the selected group, and 2) this will force the saved search to appear in the footer *everytime* the checkbox is marked. A comment below the table explaining this really makes sense IMO.

Your patch works fine on tip (I think it doesn't apply cleany on 3.0), but I would still like to see these 2 comments addressed anyway. Could you please attach an updated patch and carry forward my r+? r=LpSolit
Comment 12 Teemu Mannermaa (:wicked) 2007-07-20 23:46:46 PDT
Created attachment 273217 [details] [diff] [review]
Tested patch with arrays, V2.2

Done, carrying forward r=LpSolit as instructed.
Comment 13 Teemu Mannermaa (:wicked) 2007-07-21 00:09:12 PDT
Created attachment 273220 [details] [diff] [review]
Tested patch with arrays, v2.2 for 3.0 branch
Comment 14 Marc Schumann [:Wurblzap] 2007-07-21 00:36:04 PDT
The documentation describes the behaviour before this patch and needs to be updated.
Comment 15 Frédéric Buclin 2007-07-21 03:54:21 PDT
Comment on attachment 273220 [details] [diff] [review]
Tested patch with arrays, v2.2 for 3.0 branch

Works like a charm. r=LpSolit
Comment 16 Frédéric Buclin 2007-07-21 04:01:14 PDT
tip:

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.115; previous revision: 1.114
done
Checking in template/en/default/account/prefs/prefs.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl,v  <--  prefs.html.tmpl
new revision: 1.28; previous revision: 1.27
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.16; previous revision: 1.15
done


3.0:

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.112.2.3; previous revision: 1.112.2.2
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.11.2.3; previous revision: 1.11.2.2
done
Comment 17 Marc Schumann [:Wurblzap] 2007-07-21 04:08:59 PDT
Created attachment 273230 [details] [diff] [review]
Docs patch
Comment 18 Marc Schumann [:Wurblzap] 2007-07-21 04:20:17 PDT
Docs patch checked in.

Tip:
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.69; previous revision: 1.68
done

Branch:
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.64.2.5; previous revision: 1.64.2.4
done
Comment 19 Frédéric Buclin 2008-01-04 06:17:30 PST
Has been relnoted in 3.0.1.

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