726353 - Add a default requestee to flags

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Josh Matthews [:jdm]]

Limi says that ui-review requests should be directed at [e-mail]. Could we have that as the default value if it's going to be the common case?

Comment 1

[David Lawrence [:dkl]]

Bugzilla does not currently support setting the requestee field of a flag with a default value.
We could look into implementing something to add this support either as part of an extension or as a core feature pushed upstream. Not sure when we could start on this as we have some other projects to complete first and this is not a trivial change. Until then people can still manually add the value as needed.

dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v1)

New extension that allows a default requestee to be stored for a flag type and auto-fill the value when the flag is requested (set to ?).

dkl

Comment 3

[:glob ✱]

Comment on attachment 630064 [details] [diff] [review]
Patch creating extension to set default requestee for flags (v1)

Review of attachment 630064 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/FlagDefaultRequestee/Extension.pm
@@ +127,5 @@
> +
> +sub _default_requestee {
> +    my ($self) = @_;
> +    my $dbh = Bugzilla->dbh;
> +    return $self->{default_requestee} if $self->{default_requestee};

this should be |if exists $self->{default_requestee}|
currently it will hit the database each time if there isn't a default_requestee, as you're storing undef in $self->{default_requestee} which is false.

::: extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.html.tmpl
@@ +17,5 @@
> +
> +  function fdr_on_change(ev) {
> +    var id = ev.target.id.split('-')[1];
> +    var state = ev.target.value;
> +    var requestee_field = YAHOO.util.Dom.get('requestee_type-' + id);

this code doesn't work for attachment flags, where the flag-id is used, not the flag_type-id.

@@ +27,5 @@
> +        requestee_field.value = '[% flag_default_requestees.$type_id FILTER js %]';
> +        requestee_field.focus();
> +        requestee_field.select();
> +      }
> +    [% END %]

this produces inefficient javascript.
you only need to check state and requestee_field.value once, rather than for every type_id.
you could use a switch to determine the right value.

::: extensions/FlagDefaultRequestee/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +<tr>
> +  <th>Flag Default Requestee:</th>
> +  <td>

please add a blurb about what the field does.

Comment 4

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #3)
> this should be |if exists $self->{default_requestee}|
> currently it will hit the database each time if there isn't a
> default_requestee, as you're storing undef in $self->{default_requestee}
> which is false.

Oops. Fixed.

> extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.
> html.tmpl
> @@ +17,5 @@
> > +
> > +  function fdr_on_change(ev) {
> > +    var id = ev.target.id.split('-')[1];
> > +    var state = ev.target.value;
> > +    var requestee_field = YAHOO.util.Dom.get('requestee_type-' + id);
> 
> this code doesn't work for attachment flags, where the flag-id is used, not
> the flag_type-id.

My original intent was to only operate on new flags (flag_type-XX) and not do anything with 
current set flags (flag-XX). If a flag is already set and there is no requestee, it was intentional by the user. But looking now, there does seem to be a flaw with my implementation in that if a set flag has the same id as a flag_type there could be some confusion but it would only manifest if the user switched from ? to + and back to ? anyway.
I will fix.

I tested my code with adding/editing attachments. Can you give me a specific test case where it did not work for you?
 
> @@ +27,5 @@
> > +        requestee_field.value = '[% flag_default_requestees.$type_id FILTER js %]';
> > +        requestee_field.focus();
> > +        requestee_field.select();
> > +      }
> > +    [% END %]
> 
> this produces inefficient javascript.
> you only need to check state and requestee_field.value once, rather than for
> every type_id.
> you could use a switch to determine the right value.

Good idea.

> > +<tr>
> > +  <th>Flag Default Requestee:</th>
> > +  <td>
> 
> please add a blurb about what the field does.

Fixed.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v2)

New patch with suggested changes.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v3)

This patch adds the ability to set default requestee for currently set flags as well unless they are already set to ? then it leaves the current requestee value alone.

dkl

Comment 7

[:glob ✱]

Comment on attachment 631189 [details] [diff] [review]
Patch creating extension to set default requestee for flags (v3)

>+  var currently_requested = new Array();
>+  [% FOREACH id = flag_currently_requested.keys %]
>+    currently_requested['[% id FILTER js %]'] = 1;
>+  [% END %]

because the id is numeric, this creates a massive array; on my dev system i ended up with a ~500,000 element array.

>+    if (type.search(/_type/) == -1) {
>+      if (currently_requested[id]) {
>+        return;
>+      }

i'm still not sure this is the right thing to do.

here's what i'm expecting to be able to do:

if i have a flag which is currently set to ? with no requestee, i want to be able to easily retarget this to the default requestee.  i expect this would be common immediately after this extension is deployed.

with the current implementation i have to change the flag select to empty, submit changes to the bug, go back into attachment details and change the flag to ? (which prefills the requestee with the default) and finally submit.

i'm expecting to be able to change the flag select to empty (which hides the requestee field immediately), and then change the flag select back to ?, which shows the requestee field with the default requesteed prefilled, then submit the change.

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v4)

New patch that addresses the issues mentioned in the last review.

dkl

Comment 9

[:glob ✱]

Comment on attachment 632526 [details] [diff] [review]
Patch creating extension to set default requestee for flags (v4)

Review of attachment 632526 [details] [diff] [review]:
-----------------------------------------------------------------

the "use default" link needs to be displayed as (use default) to look like our other action links -- like (edit), and also use the correct cursor.

it should only be visible if the _current value_ of the select is ? .. changing the flag to + still keeps the "use default" link next to it, which doesn't actually do anything in this context.

the "use default" link is only visible when the requestee is empty; what are your thoughts on making it always visible, or visible when the current value in the text field != the default requestee?

if you're going to make the "use default" link hidden when clicked, it probably should be restored if the value is changed from anything but default.

::: extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.html.tmpl
@@ +67,5 @@
> +        if (default_requestees['id_' + type_id]) {
> +          var default_link = document.createElement('a');
> +          default_link.appendChild(document.createTextNode('use default'));
> +          YAHOO.util.Dom.setAttribute(default_link, 'title', 
> +                                      'Click to set requestee to default');

if we make the text "use default requestee" instead of just "use default", the title text is probably redundant and can go.

if it's to be kept in, please remove "Click to".

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v5)

New patch addressing suggested fixes.

One thing I could not figure out yet is what is making the requestee field grow larger when ajax_user_autocompletion is enabled and I add the default link span right after. If I do not add the link or if autocompletion is turned off it looks fine. Maybe you will have an idea. YUI.AutoComplete adds a div wrapper around the requestee text field that I may need to alter something with the CSS to make it stay the same size. Not sure yet.

dkl

Comment 11

[:glob ✱]

Comment on attachment 649282 [details] [diff] [review]
Patch creating extension to set default requestee for flags (v5)

this appears to be identical to v4

Comment 12

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (v6)

Ugh, sorry about that. Here is the new one.

dkl

Comment 13

[:glob ✱]

Comment on attachment 650063 [details] [diff] [review]
Patch creating extension to set default requestee for flags (v6)

Review of attachment 650063 [details] [diff] [review]:
-----------------------------------------------------------------

i can see why the field is oddly wide, but it looks painful to fix.  let's not hold this back any longer for that.

r=glob  with the following changes addressed

::: extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.html.tmpl
@@ +82,5 @@
> +  function fdrShowDefaultLink (requestee_field, type_id, flag_id) {
> +    var default_requestee = default_requestees['id_' + type_id];
> +
> +    var default_link = document.createElement('a');
> +    default_link.appendChild(document.createTextNode('default requestee'));

the default_link div doesn't have the correct cursor.  you can fix this with:
  YAHOO.util.Dom.setAttribute(default_link, 'href', 'javascript:void(0)');

Comment 14

[David Lawrence [:dkl]]

Attachment: Patch creating extension to set default requestee for flags (SCHEMA ONLY) (v7)

Schema change only patch for first stage push.

dkl

Comment 15

[:glob ✱]

Comment on attachment 656479 [details] [diff] [review]
Patch creating extension to set default requestee for flags (SCHEMA ONLY) (v7)

r=glob

Comment 16

[David Lawrence [:dkl]]

Schema change code committed. Will commit the remainder for next weeks push. Leaving open.

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added extensions/FlagDefaultRequestee
added extensions/FlagDefaultRequestee/Config.pm
added extensions/FlagDefaultRequestee/Extension.pm
Committed revision 8298.

Full commit for bmo/4.2:
Committing to: bzr+ssh://[e-mail]/bmo/4.2
added extensions/FlagDefaultRequestee
added extensions/FlagDefaultRequestee/Config.pm
added extensions/FlagDefaultRequestee/Extension.pm
added extensions/FlagDefaultRequestee/lib
added extensions/FlagDefaultRequestee/template
added extensions/FlagDefaultRequestee/lib/Constants.pm
added extensions/FlagDefaultRequestee/template/en
added extensions/FlagDefaultRequestee/template/en/default
added extensions/FlagDefaultRequestee/template/en/default/flag
added extensions/FlagDefaultRequestee/template/en/default/hook
added extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/admin
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment
added extensions/FlagDefaultRequestee/template/en/default/hook/bug
added extensions/FlagDefaultRequestee/template/en/default/hook/admin/flag-type
added extensions/FlagDefaultRequestee/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment/create-end.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment/edit-end.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/create
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/create/create-form.html.tmpl
Committed revision 8322.

Comment 17

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified extensions/FlagDefaultRequestee/Extension.pm
added extensions/FlagDefaultRequestee/lib
added extensions/FlagDefaultRequestee/template
added extensions/FlagDefaultRequestee/lib/Constants.pm
added extensions/FlagDefaultRequestee/template/en
added extensions/FlagDefaultRequestee/template/en/default
added extensions/FlagDefaultRequestee/template/en/default/flag
added extensions/FlagDefaultRequestee/template/en/default/hook
added extensions/FlagDefaultRequestee/template/en/default/flag/default_requestees.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/admin
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment
added extensions/FlagDefaultRequestee/template/en/default/hook/bug
added extensions/FlagDefaultRequestee/template/en/default/hook/admin/flag-type
added extensions/FlagDefaultRequestee/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment/create-end.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/attachment/edit-end.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/create
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
added extensions/FlagDefaultRequestee/template/en/default/hook/bug/create/create-form.html.tmpl
Committed revision 8313.