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 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 Frédéric Buclin 2006-06-18 09:58:31 PDT
*** Bug 341932 has been marked as a duplicate of this bug. ***
Comment 2 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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.