99567 - Allow Milestone to be set on creation of bug as an option

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Keith Kemp]

We use bugzilla to develop our apps where there is mainly just admin staff 
submitting bugs not so much the users as with bugzilla.mozilla.org.  So the 
person that is creating the bug needs to be able to set the Milestone as 
well.  For the time being we are going back to the bug and setting it but it 
would be nice if we could enable it as an option.  Something I'm sure you guys 
would gladly do since this application goals seams to be to be as flexible as 
possible which it does very well and this is the first time I have had an idea 
how something could be done better.  

Thanks

Comment 1

[Matthew Tuck [:CodeMachine]]

There seems to be a feeling among many of us that we should split enter_bug.cgi
into basic and advanced sections and put everything on advanced.  That is my
opinion.

Comment 2

[Keith Kemp]

Matthew:  I think that would be an excellent idea.  What would the eta on 
something like that be?

Thanks

Comment 3

[Matthew Tuck [:CodeMachine]]

Sometime during the 2.15 cycle I would expect.  Bug #25521 is currently the main
house for this.

Comment 4

[Keith Kemp]

Thanks

*** This bug has been marked as a duplicate of 25521 ***

Comment 5

[Todd Stansell]

This was marked as a duplicate of bug 25521, but this wasn't addressed in that 
bug.  This bug is about being able to set milestones during bug creation, 
where bug 25521 is about setting keywords.  This bug really needs to be 
reopened and addressed.  This should also be marked as blocking bug 165025.

Todd

Comment 6

[Dave Miller [:justdave]]

Agreed.

Comment 7

[Dave Miller [:justdave]]

resetting to component owner

Comment 8

[Todd Stansell]

Attachment: fix against tip

Here's the fix that we've implemented (for 2.18rc2).  Seems to still be
compatible with the tip of CVS.  Someone will need to review and check in, as I
can't.

Todd

Comment 9

[Todd Stansell]

Attachment: v2

Apparently had a poor cut/paste for the previous patch.  This one omits the
extra <tr> tag that shouldn't have been there.

Todd

Comment 10

[Todd Stansell]

Attachment: v3

Hadn't run runtests.sh on it, so I didn't notice it complained about using
target_milestone only once.  So, this patch adds %target_milestone to the use
vars section at the top of enter_bug.cgi. (thanks for noticing kiko!)

Todd

Comment 11

[Christian Reis]

Comment on attachment 161997 [details] [diff] [review]
v3

Looks good. Easily disabled if someone doesn't want it.

I'm curious as to why post_bug contained code for handling this, though.

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #11)
> I'm curious as to why post_bug contained code for handling this, though.

Because it was decided a while back that automatic bug creation scripts might
have a need for it, and it would also allow people to add the field to the
enter_bug form and have it just work if they wanted it.  We purposely didn't add
it to enter_bug at the time because it was Yet Another Field to add to the clutter.

If we allow this to go in, it should perhaps be combined with the "allow
reporter to set priority" param?

The first half of this patch should go in, regardless, so that any site wanting
this only has to edit their template to get it.  The latter half I think is
still up for debate.

Comment 13

[Frédéric Buclin]

*** Bug 268466 has been marked as a duplicate of this bug. ***

Comment 14

[Jim Salter]

While this patch satisfies the requirement of adding targetmilestone to the 
create page, it doesn't satisify the requirement that this not be available 
all the time.

This option should be group- or configuration-parameter enabled.  See bug 
268466 for more details.

Comment 15

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 16

[Dave Miller [:justdave]]

I have a gut feeling I'm going to regret letting this in without a controlling
param for it, but on the other hand, it is easy enough to edit the template
locally if you really don't want it.

Comment 17

[Todd Stansell]

Attachment: v4 - adds letsubmitterchoosemilestone param

Sorry I didn't get to this sooner.  I had been meaning to whip up a new patch
that included a param to turn it off.  So, here's that patch.  I'll leave it up
to Dave to decide if this one should go in now instead of v3.

Todd

Comment 18

[Shane H. W. Travis]

Kiko: v3 has approval, v4 has not been reviewed. I spoke to justdave about 
whether or not his approval would carry over, and he indicated that while we 
did have a little bit of parameter bloat, he didn't really mind and would take 
either version... according to the reviewer's choice.

As such, this patch is not going to be checked in (not by me anyway) until you
-- the reviewer of record -- either weigh in on the subject with an r+/r- of 
the latest patch, or pass it along to someone else to make the decision.

Comment 19

[Vlad Dascalu]

Comment on attachment 161997 [details] [diff] [review]
v3

This will allow anybody to send POST data to b.m.o. and corrupt the database by
setting the target milestone to invalid values.

post_bug.cgi has:

CheckFormField(\%::FORM, 'target_milestone', $::target_milestone{$product});


to prevent this.

Comment 20

[Vlad Dascalu]

Comment on attachment 172966 [details] [diff] [review]
v4 - adds letsubmitterchoosemilestone param

Same here.

Comment 21

[Todd Stansell]

Vladd,

Enter_bug.cgi simply creates the form to post to post_bug.cgi.  Not to mention
the fact that there's no other CheckFormField's in enter_bug.cgi.  Enter_bug.cgi
is not the one doing the database manipulation...post_bug.cgi is.  So, I don't
see how you're comment applies.

Todd

Comment 22

[Christian Reis]

Comment on attachment 172966 [details] [diff] [review]
v4 - adds letsubmitterchoosemilestone param

This is fine, as I had said. post_bug already contains the code to handle this,
really.

Comment 23

[Christian Reis]

I slightly prefer v3 over v4 (param hell avoidance) but our bug posting page is
already enough of a disaster that this won't be serious difference :-)

Comment 24

[Frédéric Buclin]

Comment on attachment 172966 [details] [diff] [review]
v4 - adds letsubmitterchoosemilestone param

>+if ( Param('usetargetmilestone') ) {
>+    $vars->{'target_milestone'} = $::target_milestone{$product};
>+    if (formvalue('target_milestone')) {
>+       $default{'target_milestone'} = formvalue('target_milestone');
>+    } else {
>+       SendSQL("SELECT defaultmilestone FROM products WHERE " .
>+               "name = " . SqlQuote($product));
>+       $default{'target_milestone'} = FetchOneColumn();
>+    }
>+}

IMO, if ( Param('usetargetmilestone') ) should be if
(Param('usetargetmilestone') && Param('letsubmitterchoosemilestone')). It does
not make sense to me to look at $::target_milestone{$product} if
letsubmitterchoosemilestone = 0.

And Vlad is correct, the default target milestone could be incorrect as we only
check that a default target milestone is given, not that it actually exists!

Comment 25

[Dave Miller [:justdave]]

Comment on attachment 172966 [details] [diff] [review]
v4 - adds letsubmitterchoosemilestone param

Fred has a point I guess... not much sense loading the default with a value
that we already know isn't going to be used.  But I think that's only
nitworthy, and not worth preventing a checkin.

Vlad's comment is correct actually, his explanation of it is just confused
because it doesn't actually affect database integrity, just the UI.  It won't
be corrupting anything in the database, but it does indeed fail to verify that
any passed in url param for milestone is valid, and passes it directly to the
template as-is.  Since the field is a list box, and the default value is not
any of the values in the list box, the first item in the box will be chosen by
default, when we should be setting it to the "default milestone" if they pass
in an illegal prefill value.

Comment 26

[Dave Miller [:justdave]]

Comment on attachment 172966 [details] [diff] [review]
v4 - adds letsubmitterchoosemilestone param

On the other hand, it's been pointed out to me that we have other list boxes on
the enter_bug form that have default values that we aren't checking for, so
this is no change from the existing behavior for other fields.	Lets file
another bug to clean all of those up.

Comment 27

[Shane H. W. Travis]

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.146; previous revision: 1.145
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.103; previous revision: 1.102
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tm
pl,v  <--  create.html.tmpl
new revision: 1.43; previous revision: 1.42
done