Closed Bug 365890 Opened 17 years ago Closed 17 years ago

Searches shared by users with bless rights are in the footer by default, with no warning

Categories

(Bugzilla :: User Accounts, defect)

2.23.3
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: justdave, Assigned: wicked)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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.
Depends on: 352403
Yes, agreed, there should be a checkbox. I was thinking exactly this, the other day.
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. :(
Flags: blocking3.0?
Attached patch partially tested patch, v1 (obsolete) — Splinter Review
Assignee: user-accounts → LpSolit
Status: NEW → ASSIGNED
Attachment #260043 - Flags: review?(wicked+bz)
Attachment #260043 - Flags: review?(justdave)
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).
Flags: blocking3.0? → blocking3.0-
I will approve it for 3.0 if this patch is reviewed on time.
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.
Attachment #260043 - Flags: review?(wicked+bz)
Attachment #260043 - Flags: review?(justdave)
Attachment #260043 - Flags: review-
Assignee: LpSolit → wicked+bz
Status: ASSIGNED → NEW
Flags: blocking3.0.1+
Attached patch Tested patch, V2 (obsolete) — Splinter Review
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.
Attachment #260043 - Attachment is obsolete: true
Attachment #272951 - Flags: review?(LpSolit)
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.
Attachment #272951 - Flags: review?(LpSolit) → review-
Attached patch Tested patch with arrays, V2.1 (obsolete) — Splinter Review
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.
Attachment #272951 - Attachment is obsolete: true
Attachment #272974 - Flags: review?(LpSolit)
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
Attachment #272974 - Flags: review?(LpSolit) → review+
Done, carrying forward r=LpSolit as instructed.
Attachment #272974 - Attachment is obsolete: true
Attachment #273217 - Flags: review+
The documentation describes the behaviour before this patch and needs to be updated.
Flags: documentation?
Flags: documentation3.0?
Comment on attachment 273220 [details] [diff] [review]
Tested patch with arrays, v2.2 for 3.0 branch

Works like a charm. r=LpSolit
Attachment #273220 - Flags: review?(LpSolit) → review+
Status: NEW → ASSIGNED
Flags: approval3.0+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attachment #273230 - Flags: review+
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
Flags: documentation?
Flags: documentation3.0?
Flags: documentation3.0+
Flags: documentation+
Keywords: relnote
Flags: testcase?
Has been relnoted in 3.0.1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.