Last Comment Bug 174039 - Set Flags on Bug Entry
: Set Flags on Bug Entry
Status: RESOLVED FIXED
: selenium
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: unspecified
: All All
-- enhancement with 1 vote (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
: 341932 (view as bug list)
Depends on:
Blocks: 315860 343810 345929
  Show dependency treegraph
 
Reported: 2002-10-11 15:49 PDT by Jeff Hedlund
Modified: 2007-08-10 14:50 PDT (History)
7 users (show)
myk: approval+
LpSolit: documentation+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (27.91 KB, patch)
2006-07-05 14:21 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.1 (28.51 KB, patch)
2006-07-05 14:31 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.1.1 (28.51 KB, patch)
2006-07-05 14:38 PDT, Frédéric Buclin
myk: review+
wurblzap: review-
Details | Diff | Splinter Review
patch, v2 (31.31 KB, patch)
2006-07-17 07:35 PDT, Frédéric Buclin
wurblzap: review+
Details | Diff | Splinter Review
documentation patch, v1 (5.15 KB, patch)
2006-11-22 07:16 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
documentation patch, v2 (5.29 KB, patch)
2006-11-22 07:41 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
documentation patch, v2.1 (5.29 KB, patch)
2006-11-22 07:52 PST, Frédéric Buclin
timeless: review+
Details | Diff | Splinter Review

Description User image Jeff Hedlund 2002-10-11 15:49:41 PDT
Simiilar to bug 98103, but for bugs: I'd like to be able to set flags when
entering a bug instead of having to submit a bug, then immediately change it.
Comment 1 User image Frédéric Buclin 2006-06-18 09:58:31 PDT
*** Bug 341932 has been marked as a duplicate of this bug. ***
Comment 2 User image Frédéric Buclin 2006-07-05 14:21:23 PDT
Created attachment 228185 [details] [diff] [review]
patch, v1

Works with both bug flags and attachment flags. We should definitely put JS scripts out of the templates, but that's for another bug.
Comment 3 User image Frédéric Buclin 2006-07-05 14:31:03 PDT
Created attachment 228187 [details] [diff] [review]
patch, v1.1

I forgot to include filterexceptions.pl in the patch.
Comment 4 User image Frédéric Buclin 2006-07-05 14:38:11 PDT
Created attachment 228190 [details] [diff] [review]
patch, v1.1.1

bah... ./runtests.pl 11 was complaining because I forgot to add a newline before =back in POD.
Comment 5 User image Myk Melez [:myk] [@mykmelez] 2006-07-13 18:41:40 PDT
Comment on attachment 228190 [details] [diff] [review]
patch, v1.1.1

>Index: Bugzilla/Product.pm
  ...
>+sub flag_types {
>+    my $self = shift;
>+
>+    if (!defined $self->{'flag_types'}) {
>+        $self->{'flag_types'} = {};
>+        foreach my $type ('bug', 'attachment') {
>+            my %flagtypes;
>+            foreach my $component (@{$self->components}) {
>+                foreach my $flagtype (@{$component->flag_types->{$type}}) {
>+                    $flagtypes{$flagtype->{'id'}} ||= $flagtype;
>+                }
>+            }
>+            $self->{'flag_types'}->{$type} = [sort { $a->{'sortkey'} <=> $b->{'sortkey'}
>+                                                    || $a->{'name'} cmp $b->{'name'} } values %flagtypes];
>+        }
>+    }
>+    return $self->{'flag_types'};
>+}

This makes product flag types be the union of component flag types, but flag types can apply to an entire product, in which case they don't belong to any specific component.  This code would need to take that into account.


>+        // We first disabled all flags. Then we will re-enable
>+        // those which are available for the selected component.

The comment describing what the set_assign_to function does should mention that it now also selectively enables flags.

Otherwise, the code looks good.  I haven't reviewed the UI, but if it's like the UI for flags that appears on the show bug page, it should be fine.
Comment 6 User image Frédéric Buclin 2006-07-14 04:14:39 PDT
(In reply to comment #5)

> This makes product flag types be the union of component flag types, but flag
> types can apply to an entire product, in which case they don't belong to any
> specific component.  This code would need to take that into account.

Myk, I think it does, see Bugzilla::FlagType::sqlify_criteria():

  push(@criteria, "(i.component_id = $component_id OR i.component_id IS NULL)");
Comment 7 User image Myk Melez [:myk] [@mykmelez] 2006-07-14 20:19:56 PDT
Comment on attachment 228190 [details] [diff] [review]
patch, v1.1.1

> Myk, I think it does, see Bugzilla::FlagType::sqlify_criteria():

Ah, indeed.  Ok, r=myk
Comment 8 User image Marc Schumann [:Wurblzap] 2006-07-15 01:04:40 PDT
Comment on attachment 228190 [details] [diff] [review]
patch, v1.1.1

Nice feature, and well-working patch!

The UI offers me to set/request flags I'm not permitted to set/request by flag type grant group or request group (case "setting the approval flag without being an approver"). If I try to do this, the next page has a title of "Flag Creation Failure", then there is a Bugzilla message box containing screen-filling HTML, starting with "An error occured while validating flags: <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 [...]". There is the "Only a sufficiently empowered user can make this change" message buried in there, so I assume I'm seeing the code of a ThrowUserError page here.
Below the fold (for my resolution/textsize/...), I find "Bug [...] has been added to the database". It correctly doesn't have the flag set.
I think either the UI should not offer to set/request flags I'm not allowed to set/request, or the error should be human-readable.

Nit: if I have JavaScript switched off, the UI offers me to set/request a flag I'm not permitted to set/request by flag type inclusion or exclusion settings. If I try to do this, the bug is correctly created without the flag. There is no message about this, though. I see three options here: 1) Show a message along the lines of "setting/requesting flag xy has been turned down because you're not allowed to set/request this flag", 2) turn off flag addition on bug creation if JavaScript is switched off, 3) leave it as it is and assume non-JavaScript users to be clever enough to understand what's going on.

Index: post_bug.cgi
@@ -538,11 +547,30 @@
+# This requires to be in batch mode.

Uh... This is no reason for r-, but it's unfortunate that we need to resort to stuff like this. So as food for thought: we might want to start thinking of a more general way to handle stuff like this. Attachment addition on bug creation comes to mind as a candidate to use this, and there are probably other places, too. Can we do Java-like exception throwing in perl, so that our ThrowXXXError functions don't take side-exits?

>Index: template/en/default/bug/create/create.html.tmpl
>@@ -87,6 +99,31 @@
>                   last_initialqacontact = contact;
>             }
>         [% END %]
>+
>+        // We first disabled all flags. Then we will re-enable

Nit: "First, we disable all flags. Then we re-enable..." seems to work better for me.

+      <td colspan="2">&nbsp;</td>

Why are you adding &nbsp; all over the place? If it's not necessary, I think you shouldn't do this.
Comment 9 User image Frédéric Buclin 2006-07-15 04:33:35 PDT
(In reply to comment #8)
> Why are you adding &nbsp; all over the place? If it's not necessary, I think
> you shouldn't do this.

Because I seem to recall that some web browsers display columns incorrectly (their width not being the one expected) when the content of some cells is completely empty.
Comment 10 User image Frédéric Buclin 2006-07-17 06:15:24 PDT
(In reply to comment #8)
> The UI offers me to set/request flags I'm not permitted to set/request by flag
> type grant group or request group (case "setting the approval flag without
> being an approver").

You have the same "problem" when viewing an existing bug. That's not something I plan to fix now, at least not in this bug (minor issue).
Comment 11 User image Frédéric Buclin 2006-07-17 07:35:12 PDT
Created attachment 229469 [details] [diff] [review]
patch, v2

In batch mode, Error.pm die(), but the error message which is displayed is HTML-formatted, which explains the mess Marc got when reviewing my patch. To fix that, we have to return from {code|user}-error.html.tmpl before displaying the HTML header (and footer).

I also added a "\n" at the end of the die() message in Error.pm to avoid the text "in Bugzilla/Error.pm at line 81" from being appended to the error message.

I don't plan to do anything about browsers not using JS. FlagType::validate() will silently ignore invalid flags, per design.
Comment 12 User image Frédéric Buclin 2006-07-17 07:36:51 PDT
Comment on attachment 229469 [details] [diff] [review]
patch, v2

myk, do you want to look at my patch again? See my previous comment for changes I made in this patch. Else Marc's review is enough.
Comment 13 User image Marc Schumann [:Wurblzap] 2006-07-24 12:44:35 PDT
Comment on attachment 229469 [details] [diff] [review]
patch, v2

Great.
And a good way to fix it, too, I think.
Comment 14 User image Frédéric Buclin 2006-07-24 13:01:02 PDT
Comment on attachment 229469 [details] [diff] [review]
patch, v2

myk already granted review for my previous patch.
Comment 15 User image Frédéric Buclin 2006-07-24 16:26:03 PDT
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.117; previous revision: 1.116
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.158; previous revision: 1.157
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.74; previous revision: 1.73
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.63; previous revision: 1.62
done
Checking in template/en/default/flag/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.21; previous revision: 1.20
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.72; previous revision: 1.71
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.36; previous revision: 1.35
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.178; previous revision: 1.177
done
Comment 16 User image Frédéric Buclin 2006-11-22 07:16:20 PST
Created attachment 246282 [details] [diff] [review]
documentation patch, v1

I had to rewrite a part of the documentation about bug creation as a lot of stuff was missing (and pretty unhelpful).
Comment 17 User image Frédéric Buclin 2006-11-22 07:41:59 PST
Created attachment 246286 [details] [diff] [review]
documentation patch, v2

Updated based on timeless "Live" review.
Comment 18 User image Frédéric Buclin 2006-11-22 07:52:01 PST
Created attachment 246288 [details] [diff] [review]
documentation patch, v2.1

A few more things timeless found.
Comment 19 User image timeless 2006-11-22 07:53:04 PST
Comment on attachment 246288 [details] [diff] [review]
documentation patch, v2.1

good enough.

although "queries on" => "queries for" :)
Comment 20 User image Frédéric Buclin 2006-11-22 07:59:53 PST
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.51; previous revision: 1.50
done
Comment 21 User image Max Kanat-Alexander 2007-02-12 18:08:42 PST
Added to the release notes as part of bug 349423.

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