Closed Bug 174039 Opened 22 years ago Closed 18 years ago

Set Flags on Bug Entry

Categories

(Bugzilla :: Attachments & Requests, enhancement)

Product:
Bugzilla
Component:
Attachments & Requests
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.0

People

(Reporter: jeff.hedlund, Assigned: LpSolit)

References

Details

(Keywords: selenium)

Attachments

(2 files, 5 obsolete files)

patch, v2
31.31 KB, patch
Wurblzap
: review+
Details | Diff | Splinter Review
documentation patch, v2.1
5.29 KB, patch
timeless
: review+
Details | Diff | Splinter Review 
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.
Blocks: 315860
Severity: normal → enhancement
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 341932 has been marked as a duplicate of this bug. ***
QA Contact: mattyt-bugzilla → default-qa
Assignee: myk → attach-and-request
Target Milestone: --- → Bugzilla 2.24
Attached patch patch, v1 (obsolete) — — Splinter Review 
Works with both bug flags and attachment flags. We should definitely put JS scripts out of the templates, but that's for another bug.
Assignee: attach-and-request → LpSolit
Status: NEW → ASSIGNED
Attachment #228185 - Flags: review?(myk)
Attachment #228185 - Flags: review?(wurblzap)
Attached patch patch, v1.1 (obsolete) — — Splinter Review 
I forgot to include filterexceptions.pl in the patch.
Attachment #228185 - Attachment is obsolete: true
Attachment #228187 - Flags: review?(myk)
Attachment #228185 - Flags: review?(wurblzap)
Attachment #228185 - Flags: review?(myk)
Attached patch patch, v1.1.1 (obsolete) — — Splinter Review 
bah... ./runtests.pl 11 was complaining because I forgot to add a newline before =back in POD.
Attachment #228187 - Attachment is obsolete: true
Attachment #228190 - Flags: review?(myk)
Attachment #228187 - Flags: review?(myk)
Attachment #228190 - Flags: review?(wurblzap)
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.
Attachment #228190 - Flags: review?(myk) → review-
(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 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
Attachment #228190 - Flags: review- → review+
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.
Attachment #228190 - Flags: review?(wurblzap) → review-
(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.
(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).
Attached patch patch, v2 — — Splinter Review 
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.
Attachment #228190 - Attachment is obsolete: true
Attachment #229469 - Flags: review?(wurblzap)
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.
Attachment #229469 - Flags: review?(myk)
Blocks: 343810
Comment on attachment 229469 [details] [diff] [review]
patch, v2

Great.
And a good way to fix it, too, I think.
Attachment #229469 - Flags: review?(wurblzap) → review+
Comment on attachment 229469 [details] [diff] [review]
patch, v2

myk already granted review for my previous patch.
Attachment #229469 - Flags: review?(myk)
Flags: approval?
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: relnote
Resolution: --- → FIXED
Blocks: 345929
Flags: documentation?
Flags: testcase?
Attached patch documentation patch, v1 (obsolete) — — Splinter Review 
I had to rewrite a part of the documentation about bug creation as a lot of stuff was missing (and pretty unhelpful).
Attachment #246282 - Flags: review?(documentation)
Attached patch documentation patch, v2 (obsolete) — — Splinter Review 
Updated based on timeless "Live" review.
Attachment #246282 - Attachment is obsolete: true
Attachment #246286 - Flags: review?(documentation)
Attachment #246282 - Flags: review?(documentation)
A few more things timeless found.
Attachment #246286 - Attachment is obsolete: true
Attachment #246288 - Flags: review?(documentation)
Attachment #246286 - Flags: review?(documentation)
Comment on attachment 246288 [details] [diff] [review]
documentation patch, v2.1

good enough.

although "queries on" => "queries for" :)
Attachment #246288 - Flags: review?(documentation) → review+
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
Flags: documentation? → documentation+
Added to the release notes as part of bug 349423.
Keywords: relnote
Flags: testcase? → testcase+
Keywords: selenium
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: