Closed Bug 81642 Opened 23 years ago Closed 20 years ago

"Split bug / Clone bug": Enter new bug with prefilled fields

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement, P3)

Product:
Bugzilla
Component:
Creating/Changing Bugs
Version:
2.13
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.20

People

(Reporter: afranke, Assigned: shane.h.w.travis)

References

Details

(Whiteboard: enter_bug)

Attachments

(1 file, 11 obsolete files)

Code patch for tip, take 7
11.21 KB, patch
jouni
: review+
Details | Diff | Splinter Review 
For splitting bugs that contain multiple issues, a "Clone this bug" link in the
"show_bug.cgi" generated bug view would be helpful.

It should have all fields initialized to the original one, except the comments.
If you want to get fancy, you can have an intermediate page where you can choose
to _not_ copy certain fields.

Maybe the description field should be initialized with something like
"*** This bug has been split off bug 12345 ***".
I would like to see the parent bug have the children added as dependant bugs.
ie: It would be required to fix all children before fixing the parent.

Additionally, this mechanism could be used to enter bugs that span multiple
versions. A form that allowed multiple version selections could be added, then
if > 1 version(s) were checked, multiple dependent bugs would be created for
each version, with a meta bug as a parent for all the dependent children.
Priority: -- → P4
Target Milestone: --- → Future
Mattty, can you give a short explanation of the meaning of "Future"? If it means
"post Bugzilla 3.2" which is post-3.0 which in turn may be 2004, then I disagree
with a lot of your target milestone assignments, including this one.
The main task here was to clear off my '---' list to make sure there wasn't
anything really important on it and get the 2.16 list ready.

'Future' in Bugzilla currently basically means "no plans to implement in the
other milestones".  It doesn't mean it will have to wait for 3.0, as it might
happen in 2.20, but it does mean it won't be happening in 2.16/2.18, nor in
3.0/3.2.

That's a little confusing but it's a little hard to target otherwise because we
don't know how many 2.X milestones we're going to have.  If we clear as many
bugs away as I hope we will in 2.16 we'll need to sit down afterwards and figure
out where we're going for 2.18 and beyond and where 3.0 fits into the picture. 
Currently there aren't many bugs targetted for 2.18, 3.0 or 3.2 as most are at
2.16 or Future.

And as always patches are welcome and should hopefully in future be given the
express lane once we clear the patch backlog.
Ok, looking for the express lane then. Please comment whether this approach is
the right or the wrong way to do it.

The patch is mostly working, except for groupsets, dependencies, and keywords.
(dependencies and keywords are not yet supported by enter_bug.cgi).

It would be nice if someone who knows groupsets better could have a look at
enter_bug.cgi and this new code and give me a hint how to debug the bits stuff.
Assignee: tara → afranke
Keywords: patch, review
Priority: P4 → P3
Target Milestone: Future → Bugzilla 2.16
-> Bugzilla product.
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.13
Whiteboard: enter_bug
-> default owner. (Pulling myself out.)
Assignee: afranke → myk
Doesn't "remember values as a bookmarkable template" already accomplish this?
Only if you plan to file two similar bug reports before filing them.  This is
for after the first one already exists, and you decide later it needs to be split.
We definitely want the ability to clone bugs - Asa and Myk and I had a long
discussion about it. However, we want to associate the clones in the DB, and I
think it's a bit bigger of a project than filing a new bug with the same data.

At any rate, this isn't 2.16 material :-)

Gerv
Keywords: patch, review
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
This sounds like bloat to me.  It's already possible to file a new bug and then 
cross-reference the two bugs.  What makes this feature worth the extra 
complexity of the code and of bugzilla's user interface?
Gerv, can you clarify what you meant by "associate the clones in the DB"? I
mean, the new bug won't be an exact clone, will it? In most cases, you will
tweak the summary a bit, and maybe some other things. Maybe you mean
"originated_from" or "part_of" (which could be expressed as a new dependency
like bug 141175 calls for)? Can you explain your intended semantics?
The way I envisage this working is as follows:

There is an operation you (or a sufficiently empowered user) can perform on a
bug, known as "cloning" it. This files a new bug report, with all the fields set
to exactly the same values as the old report, and makes a link between the bugs
using some DB mechanism (I have ideas, but it's not relevant.)

The UI would then be that bug reports which were part of a set of clones would
have tabs at the top, giving the numbers, and perhaps summaries, of the clones.

Once you have cloned a bug, you are responsible for changing the summary or
fields to indicate why it's different, e.g. changing the Version field to
"Mozilla 1.0 Branch" for a bug to track a fix which has been checked in on the
trunk.

So, what do we do about things which are associated with the intial bug:
- attachments
- comments
- votes
- dependencies?

This would require discussion. Off the top of my head:
- attachments. We could either try to clone all non-obsolete ones, or not clone
any, on the basis that the patch for the clone is probably different.
- comments. Don't clone these. There will be loads of dross, and the cloner can
summarise the issues the new bug is about when they fiddle the fields.
- votes. Again, not much point cloning these really. We are past the stage where
votes matter
- dependencies. Hmm. If the bug has been fixed, "depends on" is irrelevant.
"blocks" might be relevant.

Another thought: you may be asked to redefine the initial description of the
clone, based on the description of the original.

Gerv

Ok, I understand your reasoning, but that's different from what I had in mind
when I filed this bug. I was just finding myself quite often in a situation
where I needed to file a new bug, with almost the same settings as an existing
one I was viewing at the same time. I wouldn't want to have them associated in
the database, or displayed as tabs, or something. I just wanted to have the
option to file a "similar" bug where I only needed to adjust a few fields
instead of re-entering *all* the fields' data. 

In some cases, setting the (yet to be implemented) "related bugs" field (see bug
12286) may make sense, but that would fit under a generalization of bug 141175.

Gerv: can you make a decision whether this bug is about mine or your proposal,
and whether we need a third bug for the other one (besides bug 141175 which
calls for a shortcut to create a dependent bug)?
> I just wanted to have the
> option to file a "similar" bug where I only needed to adjust a few fields
> instead of re-entering *all* the fields' data. 

Your idea and mine are almost exactly the same; all that would be required would
be to have a checkbox, checked by default, marked "These bugs should be
associated with each other" on the intermediate confirmation screen for cloning
a bug (where the user defines those bits of the bug which aren't copied from the
clone.)

If we then define a way to create the associations independently of a "clone"
operation, we have the "related bugs" feature. Maybe. But I'm not convinced by
"related bugs" - the examples given in bug 12286 fit into dependency
relationships, as far as I can see.

Gerv
> checkbox [...] on the intermediate confirmation screen for cloning a bug 

nice, except that I've been happily using my "cloning" feature for almost half a
year now, without an extra intermediate screen, and I think it's really not
necessary. But if you agree that this checkbox can be displayed somehow on the
same page with the ordinary enter_bug form, then this could be a solution.
Of course, it would be nice if I could customize my template(s) so that this
checkbox is hidden (and off) when my users hit my customized "clone" link 
(in case I haven't any use for the "associated in the database" feature).
But maybe the database-association will be useful enough in the end that
everybody wants to have this option, I don't know.
> nice, except that I've been happily using my "cloning" feature for almost half a
> year now, without an extra intermediate screen, and I think it's really not
> necessary. 

That must mean that your cloned bugs start off identical to the original, if
there is no opportunity to modify the exact parameters of the cloning. I think
that doing that would make the feature substantially less useful and general.

Gerv
No, it just takes me directly to the enter_bug page where I can modify
everything I want, just like a bookmarked enter_bug template, except that the
prefill values are taken from the bug where I click the link on. See the patches
attached to this bug. What I meant is: if you can agree to put that checkbox on
the enter_bug page directly, then this is probably solved.
> No, it just takes me directly to the enter_bug page where I can modify
> everything I want,

Except you can't, because not all bug fields appear on the enter_bug page. I'm
not quite convinced that going via the enter_bug page is the right approach...
I'd have to think about that.

Gerv
> Except you can't, because not all bug fields appear on the enter_bug page.

which is a bug in the enter_bug page, IMO. Anyway, it worksforme, see attachment
45537 [details] [diff] [review] (clone_bug.cgi). The only shortcomings I am aware of:

#XXX TODO: groupsets    (enter_bug already has support for that),
#          dependencies (no support in enter_bug yet),
#          keywords     (no support in enter_bug yet).
#          settable_resolution
Blocks: bz-zippy
This probably depends on combining process_bug and post_bug (and, ideally,
making them use bug.pm). If it doesn't depend on it, it would probably make it
_much_ easier.
*** Bug 141175 has been marked as a duplicate of this bug. ***
I would be happy with this being a dup of bug 141175 if clone_bug.cgi could also
take a dependecy (dependson and blocked) argument to pre-fill with those fields
on enter_bug with those values (and if not sent, clone the bug).

This would allow me to easily make a link for "Enter blocking/dependant bug" in
the template like 141175 requested.

And as far as the TODOs:
> #XXX TODO: groupsets    (enter_bug already has support for that),
> #          dependencies (no support in enter_bug yet),

Fixed in bug 112373.

> #          keywords     (no support in enter_bug yet).

Fixed in bug 25521.


*** Bug 175250 has been marked as a duplicate of this bug. ***
So comments are not shared between cloned bugs, Gerv?
> So comments are not shared between cloned bugs, Gerv?

In my vision, no. When the screen comes up for filing the clone, the "Original
Description" would be initialised with the original description from the first
bug, but then it could be changed or added to if necessary, to say why this
particular bug was split off (if it wasn't obvious from other fields.)

Fitting with the idea of developing the fix in one place, and then splitting for
the actual application to various places, most of the comments would be
irrelevant anyway. This does raise the question of whether we want to clone
attachments; perhaps we could clone non-obsolete ones. I don't think there's a
problem having two bugs referencing the same attachment.

Gerv
The Product to which the bug should be cloned should be selectable, instead of
fixed.

Let's say you start a new product from older code, but you still need to
maintain the older code, you might want to clone the bug to both products.





clone_bug.cgi is perfect, but it would be more perfect :) if you could:
1. select the product to clone the bug to
2. clone the comments
3. clone the attachments

I'd be willing to make the changes myself, if someone could point me in the 
right direction.

Line 125 of attachment 45537 [details] [diff] [review] is missing an = sign.
    . "&op_sys"        . url_quote($bug{'op_sys'})
should be:
    . "&op_sys="       . url_quote($bug{'op_sys'})
The attachment 45539 [details] [diff] [review] is obsolete. It should replaced by patch for template
bug/navigate.html.tmpl

To fix this there should be added:
"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<a href="clone_bug.cgi?id=[% bug.bug_id %]">Clone
this bug</a>" at the end of this template.

*** Bug 226208 has been marked as a duplicate of this bug. ***
I vote for this bug!  

My company tends to use Collector bugs to track groups of similar bugs.  We
would greatly benefit from this functionality in that we could easily create new
bugs to add to the Collector by editing the Collector, clicking on the new
'Create similar bug' link, and then changing the fields as needed, prior to
submission.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
sort of a different idea in Bug 252807
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Attached patch Patch against 2.18rc2, first try (obsolete) — — Splinter Review 
Alright... I'm new at this, so be gentle. :)

I hesitated to upload my patch because I didn't want to step on anyone's toes,
but I can see that this has been on the books for a long time, and am told that
Andreas is mostly inactive these days. I don't know if this will facilitate
getting the feature into an upcoming version, but I figure it certainly can't
hurt. 

I've come at this from a different direction from Andreas, I think. My version
adds another field on enter_bug.cgi that allows you to name another bug whose
contents you wish to clone. Information is sucked from that bug, and the page
is redrawn with said info pre-filled in to the fields. These fields can then be
edited if the user desires.

Only the first comment from the original bug is copied, and an additional line
of text is included at the top of the comment to note that the information was
cloned from another bug.

It is possible to clone a bug across products; a security check is done to
ensure that the author has permissions to see the information on the bug he is
trying to clone. If he does, it goes ahead; he will have to re-set the product
and component before he can commit.

Local decision was that attachments were not to be copied. I can see some
arguments for why they should, but for the most part I agree that they should
stay with the original. Since there's no way to edit inclusion/exclusion of
attachments in enter_bug, I'm more comfortable leaving it out.

patch includes an allow-bug-cloning parameter so sites can turn this feature
off if they won't use it.

I originally did the fix in 2.16.6, but had to hack some stuff to get the
dependencies to work. The attached patch is was done against 2.18rc2 and is
much more elegant, IMHO. 

I have (literally) never made a patch file before, so I am hoping that it
works. :)
Attachment #162935 - Flags: review?
It would be best to avoid going to the database with specialized SQL to do this.
 Can you do this usign either the fields alredy provided to the template by the
previous show_bug (to prepopulate the new bug template - if a bookmarklet can do
it (bug 241443), template code should be able to) or by prefilling the fields
using the bug object?

The reason it works for bookmarkable templates is because (up to 2.18 anyway,
don't know about beyond) a template URL looks something like this:

http://fwk-svr.sedsystems.ca/bugzilla-test/enter_bug.cgi?product=TestProduct&bug_status=NEW&version=other&component=TestComponent&rep_platform=Macintosh&op_sys=Windows%20Server%202003&priority=P5&bug_severity=blocker&assigned_to=travis&cc=test%20test2%20test3%20&bug_file_loc=http%3A%2F%2Fwww.sedsystems.ca&short_desc=Bug%20Bug%20Numbah%20One%21&comment=%2B%2B%2B%20This%20log%20item%20was%20initially%20created%20as%20a%20clone%20of%20Log%20Item%20%231%20%2B%2B%2B%0D%0AFirst%20bug%21%20Booyah%21&keywords=important%20gui%20&dependson=1&blocked=&maketemplate=Remember%20values%20as%20bookmarkable%20template&form_name=enter_bug


Right there, that's all the information you need to re-create an enter_bug
form. 

For my patch, that information does not exist. There *is* no 'previous
show_bug' or existing bug object. 

As I said in comment #36, this is NOT an update of Andreas' patch; it is an
entirely new way of coming at it. Andreas' implementation (as I understand it)
was to have a 'Clone this bug' button on show_bug; mine is to have a 'clone bug
# ___' on enter_bug. This is not an intentional slight on Andreas' work; I had
already coded up my way of cloning bugs at the request of our local users, and
it was only later, after several people on the developer's list and newsgroup
showed similar interest in having these capabilities, that I learned of the
existence of this bug, at which point I updated my modifications to work with
2.18rc2 and made them publicly available here.

Andreas' method may be great; it may, in fact, be better than mine. I don't
know, because I've never implemented his method. I'm just adding my voice to
the chorus of people who'd like to see this functionality exist globally, and
putting my money where my mouth is by making my patch available to all. If the
decision is that mine is the wrong way of going about it, I won't be crushed
irrevocably; I just won't be able to work to get the feature into Bugzilla any
more, and will have to content myself with our local solution.

Having said that, I found one minor bug, some commented code I left in, and a
couple of tabs-instead-of-spaces, so I've made a second attempt.
Attachment #162935 - Attachment is obsolete: true
Attachment #163092 - Flags: review?(justdave46203)
Attachment #162935 - Flags: review?
Attachment #163092 - Flags: review?(justdave46203) → review?(justdave)
Hmm. I'd say the correct implementation is definitely to have the "clone bug"
link on the original bug. Otherwise, you are looking at the original bug, and
you have to go somewhere else, taking the bug number with you, to clone it...

Gerv
Y'know... I had a big explanation written in response to Comment #39 as to why
doing it this way was better -- mostly because Andreas' method didn't allow for
bugs to be cloned from one project to another, whereas my patch did. (This was
critical for local implementation.) Besides, even the original patch was going
back to the database for all the information DESPITE being positioned already
on the show_bug page.

The more I went looking for specific reasons why I couldn't do it, though, the
more I realized that it might be workable after all... especially when keeping
in mind Joel's comments about bookmarks templates.

So, long story short (I know, too late) I re-wrote the whole darn thing to make
a new template clone.html.tmpl that hangs off show_bug, only appears if the
parameter is true, allows for cloning across products, and doesn't ever have to
go to the database. As the script-kiddies say, this one = teh win. :)
Attachment #163092 - Attachment is obsolete: true
Attachment #163469 - Flags: review?(justdave)
Attachment #163092 - Flags: review?(justdave)
Comment on attachment 163469 [details] [diff] [review]
3rd patch against 2.18rc2 - complete re-write

Learning the process, realizing Dave's a busy man... maybe kiko can take a look
at this instead. :)
Attachment #163469 - Flags: review?(justdave) → review?(kiko)
Attachment #163469 - Flags: review?(jouni)
Assignee: myk → travis
Status: NEW → ASSIGNED
Comment on attachment 163469 [details] [diff] [review]
3rd patch against 2.18rc2 - complete re-write

First off: I like the approach here. However, the architecture fails in one
critical spot: passing the first comment. Because of this, some of the
following code comments may be less useful for the future versions, but I'll
post them nevertheless to help (or frustrate, whatever ;-)) you in your
Bugzilla career.

In the future, please patch against the tip instead of 2.18 branch. Not a
problem this time though; the patch applied just fine. Also, please run the
testing suite against the patched version - at least this one fails the tests.


>+   name => 'allow-bug-cloning',

We don't use hyphens in parameter names (just "allowbugcloning" will do).

>+   desc => 'If this option is on, users may make a \'clone\' of any existing bug they can access. Cloning pre-fills various fields of the copy with information pulled from the original; these fields can be edited before the new bug is committed.',

Wrap this line to the 80 char limit. Also, "the cloning link appears on the
show_bug screen" might be a worthy addition.

>--- bugzilla-2.18rc2/enter_bug.cgi	2004-06-22 01:49:15.000000000 -0600
>+++ bugzilla-2.18rc2.clone/enter_bug.cgi	2004-10-26 15:33:46.000000000 -0600
>+        if ( Param('allow-bug-cloning') ) {
>+          $vars->{'url'} = $::buffer;
>+        }

"url" isn't a particularly enlightening name for a variable. How about
"enter_params_url" or whatever? (at least you might add a comment describing
what's going on here)

>--- bugzilla-2.18rc2/template/en/default/bug/create/clone.html.tmpl	1969-12-31 18:00:00.000000000 -0600
>+++ bugzilla-2.18rc2.clone/template/en/default/bug/create/clone.html.tmpl	2004-10-26 15:25:36.000000000 -0600

Since this template is just the clone link, it's perhaps better off in the bug
directory where the show templates reside? Also, maybe named as "clone-link" or
whatever?

>+[%# Sort of a messy way to go about this, but we need to 'FILTER url_quote'
>+  # some parts of the string, and not others. Besides, this scans easily,
>+  # which is seldom a bad thing.
>+  #%]

I agree about the messy part :-) You don't need to explain it in the source
file, though... That explanation is necessary for the reviewer, not for the
code reader. Anyway, I have a replacement suggestion (which has its problems
too, but it's IMO more readable):

[% clone_url = BLOCK %]bug_status=NEW&version=
  [% bug.version FILTER url_quote %]&component=
  [% bug.component FILTER url_quote %]&rep_platform=
  [% bug.rep_platform FILTER url_quote %]
[% END %]

(several fields omitted, but you get the point)

>+[% clone_comment = '+++ This log item was initially created as a clone of ' _ terms.Bug _ ' #' _ bug.bug_id _  '. +++' FILTER url_quote %]
>+[% clone_comment = clone_comment _ '%0D%0A' %]
>+[% next_param = bug.longdescs.0.body FILTER url_quote %]
>+[% clone_url = clone_url _ '&comment=' _ clone_comment _ next_param %]

This part is the main problem here. First, it causes a "414 Request URI too
long" server error if your comment 0 is long enough. Second, it exposes the
content of the first comment in even if it is marked as private (using the
insider group). 

You could go around this by reformatting the link as a group of hidden fields
and a submit button, but that's likely to be too dominant an UI element. Of
course, you could do the extra db access to handle this, but it seems like
quite some hassle. Hmm... I wonder if the first comment is really worth all
this effort? Usually it isn't the best summarization of a problem, so the
reporter is probably going to end up writing a renewed summary anyway.

The clone comment prefix would better work as something like "This description
was cloned from bug Xxx".

>+[% clone_url = clone_url _ '&blocked=' %]

Why this empty param? (the same for the empty assignee param passed)
Attachment #163469 - Flags: review?(jouni) → review-
Attachment #163469 - Flags: review?(kiko)
(In reply to comment #42)
> >+   name => 'allow-bug-cloning',
> We don't use hyphens in parameter names (just "allowbugcloning" will do).

I beg to differ: cf: all the moved-* parameters. (Many newer parameters also 
have underlines in them.)

I recall seeing a bug where someone requested that all parameters be broken up 
on word boundaries to make it easier for non-english speakers, and I think it's 
a very good idea. I don't mind switching from hyphens to underscores if that's 
the preferred method, but I do think that multi-word paramaters should be 
broken *somehow* on word boundaries.


> >--- bugzilla-2.18rc2/template/en/default/bug/create/clone.html.tmpl

> Since this template is just the clone link, it's perhaps better off in the bug
> directory where the show templates reside?

Well, you do end up creating a new bug, hence my placement in 'create' and 
not 'show'. If you're adamant about moving it, I could do so, but this seemed 
more natural to me.

Rename makes sense, and will be in next version.


> >+[%# Sort of a messy way to go about this, but we need to 'FILTER url_quote'
> >+  # some parts of the string, and not others. Besides, this scans easily,
> >+  # which is seldom a bad thing.
> >+  #%]
> I agree about the messy part :-) You don't need to explain it in the source
> file, though... That explanation is necessary for the reviewer, not for the
> code reader.

Well, my personal opinion is that Bugzilla suffers from being significantly 
*under* commented, so there's seldom any explanation as to what's happening or 
why. 

One quote in our quips file (put there by me, admittedly) is "Always code as if 
the guy who ends up maintaining your code will be a violent psychopath who 
knows where you live. ~Martin Golding"  For me, this means not only well-
written code, but programmer comments where they make sense. Since this looked 
so kludgy, it made sense here. <shrug>



> This part is the main problem here. First, it causes a "414 Request URI too
> long" server error if your comment 0 is long enough. 

Hrm... kay, I'll have to look into that.

> Second, it exposes the content of the first comment in even if it is marked
> as private (using the insider group). 

Could this not be fixed by duplicating the first comment that the user can see? 
(And setting the privacy to  be the same in the new bug as it was in the old 
bug...) After all, if someone can only see the third comment because the first 
two are insider comments, and they go to clone the bug, expected behaviour 
would be to bring over the first comment visible to them, no?


> Hmm... I wonder if the first comment is really worth all this effort?

I think it is, and several people in the thread of this bug have commented that 
they'd like the first comment preserved.

> The clone comment prefix would better work as something like "This description
> was cloned from bug Xxx".

Why 'this description' and not 'this bug'? After all, you're cloning the entire 
bug (and linking them), not just the description...


> >+[% clone_url = clone_url _ '&blocked=' %]
> Why this empty param? (the same for the empty assignee param passed)

Probably unnecessary. I'll check.


Thanks very much for the feedback, Jouni; I look forward to your response, and 
will have an updated patch some time in the near future.

(In reply to comment #43)
> I beg to differ: cf: all the moved-* parameters. (Many newer parameters also 
> have underlines in them.)

Very well.

> I recall seeing a bug where someone requested that all parameters be 
> broken up on word boundaries to make it easier for non-english speakers, 
> and I think it's  a very good idea.

I agree 100%; it's just that I believe in doing it uniformly one way and then
switching in one go. But if we've already begun sliding, I'm not going to oppose
it here.

> Well, you do end up creating a new bug, hence my placement in 'create' and 
> not 'show'. If you're adamant about moving it, I could do so, but this seemed 
> more natural to me.

Yes, but we're only creating a _link_ to bug creation here. As a really bad
analogy, we don't have the footer's New link templatized in the create directory.

> Well, my personal opinion is that Bugzilla suffers from being significantly 
> *under* commented, so there's seldom any explanation as to what's happening or 
> why. 

True, though I feel this isn't the commentary Bugzilla needs most direly. It's
all about signal/noise ratio. But I'm not going to hold review over this
comment, of course :-) If you like my replacement suggestion, this may of course
become irrelevant anyway.

> Could this not be fixed by duplicating the first comment that the user 
> can see? 

Perhaps, but it could practically produce a very strange basis for a bug report
- the first comment the user can see could be anything, including something
totally worthless. I'd almost say we could leave the first comment empty if we
can't get comment 0.

> > The clone comment prefix would better work as something like "This 
> > description was cloned from bug Xxx".
> Why 'this description' and not 'this bug'? After all, you're cloning the 
> entire bug (and linking them), not just the description...

All right, you've got me turned.
Attached patch Code patch for tip, take 2 (obsolete) — — Splinter Review 
* Patched against tip, not 2.18
* Changed parameter name to have underscores instead of hyphens
* Wrapped parameter info properly
* $vars->{url} is now $vars->{clone_link_url}
* Renamed clone.html.tmpl to clone_link.html.tmpl (but left it in the same
    directory; if you're adamant, I guess I can still move it)
* Used Jouni's replacement code suggestion
* Removed blank parameters
* Removed any attempt to get the first comment into the clone

Of these, only the last point really bothers me. I put code in place so that
the first comment was not copied if the user couldn't see it, and that was
simple enough... but when I tried to deal with the too-long-URI issue, it
really smacked me down hard. Empirical testing showed that I was usually safe
with a URL up to at least 4000 characters on several different browsers, but
Jouni you said that the standard indicates that browsers only HAVE to accept
URLS up to 512 characters, which doesn't leave much space for the initial
comment at all. :(  

Even with 4000 characters, I was having problems deciding where to stop
(because [% FILTER url_quote %] was adding a lot of characters to the URL), and
what to do if I had to truncate, but those were mere technical issues that I
could eventually have gotten around.

My honest opinion is that without the ability to copy the first comment, this
becomes a much less useful feature... but maybe that's just me and my needs.
(Although even Gerv seemed to think that the first comment was useful too --
see comment #27). Can anyone help me brainstorm a way to make this happen that
gets around the URI limit? 

Anyway... on the assumption that it cannot be done and/or it's still a useful
feature without cloning the first comment, I'll still finish it up and get it
on the trunk.

One last quesiton: Andreas had his clone link in the navigation bar, I put mine
in the knob. Other than style/preference, any good reasons to go with one over
the other?
Attachment #163469 - Attachment is obsolete: true
Attachment #168881 - Flags: review?(jouni)
Comment on attachment 168881 [details] [diff] [review]
Code patch for tip, take 2

(In reply to comment #45)
> really smacked me down hard. Empirical testing showed that I was usually safe
> with a URL up to at least 4000 characters on several different browsers, but
> Jouni you said that the standard indicates that browsers only HAVE to accept
> URLS up to 512 characters, which doesn't leave much space for the initial
> comment at all. :(  

I seem to have remembered wrong. The HTTP spec doesn't limit nor require
anything on the URI length. 512 chars must've been the limitation from some
package I was using a long time ago. My apologies for the false information.
Some quick googling shows that the most dominant limitation here is 2048 (or
2083, depends on how you count) by IE 5.5 and earlier; see
<http://support.microsoft.com/kb/q208427/>. Apparently my testbed Apache
rejects URIs of 4k+ - but as you noted, whatever the limitations are, we will
have a problem as the uri simply cannot contain all this information, and
that's it. Let's focus on the alternatives.

> My honest opinion is that without the ability to copy the first comment, this
> becomes a much less useful feature... but maybe that's just me and my needs.
> (Although even Gerv seemed to think that the first comment was useful too --
> see comment #27). 

What's this "even Gerv" stuff? ;-)

I'm not saying the first comment wouldn't be useful at times - it might be a
reasonable default. Even though you're probably right at least for some usage
scenarios, the lack of first comment copying doesn't IMO make the feature
useless - cloning a bug is still easier with this stuff than without it.

> Can anyone help me brainstorm a way to make this happen that
> gets around the URI limit? 

As far as I can see, there are two reasonable options. Either you make Clone
link a button and use HTTP post to convey the information OR you pass a bug ID
and retrieve the comment from the DB at enter_bug. I strongly prefer the first
one, since I don't think it's necessarily worthy to bloat the show_bug HTML by
including the basic bug information and comment 0 twice. If you go down the DB
access line, then passing any of the other fields in the URI seems illogical as
well.

> One last quesiton: Andreas had his clone link in the navigation bar, I put mine
> in the knob. Other than style/preference, any good reasons to go with one over
> the other?

Your choice is IMO better; it fits the grouping of links to
list-level/bug-level better than the placement in nav bar.


I also noted that security group and time tracking information doesn't get
cloned. It's an interesting question whether or not they should (then again,
why not?), but as the amount of fields to clone grows, I'm more and more
starting to think about the DB-read approach.


---
>--- tip/template/en/default/bug/create/clone_link.html.tmpl	1969-12-31 18:00:00.000000000 -0600
>+++ tip.clone/template/en/default/bug/create/clone_link.html.tmpl	2004-12-16 11:24:26.000000000 -0600

We use hyphens for template names ("clone-link").

>+[%# Sort of a messy way to go about this, but we need to 'FILTER url_quote'
>+  # some parts of the string, and not others. Besides, this scans easily,
>+  # which is seldom a bad thing.
>+  #%]

Hey, I don't think it's messy anymore :-) IMO you can remove this comment
totally.

>+[% clone_comment = '+++ This log item was initially created as a clone of ' 
>+   _ terms.Bug _ ' #' _ bug.bug_id _  '. +++' FILTER url_quote %]

How about "This [% terms.bug %] was initially created"? "log item" doesn't
sound familiar to me.

>         &nbsp; | &nbsp;
>         <a href="long_list.cgi?buglist=[% bug.bug_id %]">Format For Printing</a>
>+      [% IF Param('allow_bug_cloning') %]
>+        [% PROCESS bug/create/clone_link.html.tmpl %]
>+      [% END %]
>       </b>

Nit: Indent the IF block by two spaces.
Attachment #168881 - Flags: review?(jouni) → review-
(In reply to comment #46)
> What's this "even Gerv" stuff? ;-)

Honestly, just a reference to the earlier discussion. By my reading, Gerv 
didn't think that it was worth copying *all* the comments, but he did think it 
was worth taking the first one. I would tend to agree with his logic, and was 
basically using his support as justification.

> the lack of first comment copying doesn't IMO make the feature
> useless - cloning a bug is still easier with this stuff than without it.

So, by my reading of your comments, this patch is basically ready to go (ready 
to go, barring a couple of nits)... and we're going to get rid of it because we 
can't use it to get the first comment. That right?

(I'm not complaining: I'm just trying to see if I've correctly divined what you 
are saying.)


> Either you make Clone
> link a button and use HTTP post to convey the information OR you pass a bug ID
> and retrieve the comment from the DB at enter_bug. I strongly prefer the first
> one, since I don't think it's necessarily worthy to bloat the show_bug HTML by
> including the basic bug information and comment 0 twice.

Some confusion here: did you mean to say, "I strongly prefer the second one?" I 
don't see how passing the bug_id would cause bloat, but perhaps I'm missing 
something.

> If you go down the DB access line, 
> then passing any of the other fields in the URI seems illogical as well.

I guess so. The whole reason we were trying to pass in all the info in the URI 
was to make use of the bookmark-like features and avoid any database access 
whatsoever. One access is infinitely more than no accesses... and two is only 
twice as much as one. ;-)


> I also noted that security group and time tracking information doesn't get
> cloned. It's an interesting question whether or not they should (then again,
> why not?), but as the amount of fields to clone grows, I'm more and more
> starting to think about the DB-read approach.

The security group is just a non-product group, isn't it? Nothing inherently 
special about it... although it does beg the question as to whether or not a 
cloned bug should start off with all the same bug groups as its parent. What if 
the clone is in a different product altogether? That's allowed... and some of 
the bug groups may no longer apply if that's the case. My thought is just to 
leave them to whatever the defaults are, or whatever can normally be set for a 
new bug of this type on the enter_bug screen.

Time tracking should definitely *not* be copied, for two reasons:
* First, it would mean that you've got hours for one bug recorded in two
   places. Time spent on the original and time spent on the clone are
   tracked in each individual bug.
* Secondly, time tracking is stored in longdescs, and it's not possible
   to copy a longdesc without taking the comment - can't have a blank
   longdesc:thetext entry or many things break. Since we're not taking
   the comments, we *can't* take the time entries. 


> >+++ tip.clone/template/en/default/bug/create/clone_link.html.tmpl	2004-12-
16 11:24:26.000000000 -0600
> We use hyphens for template names ("clone-link").

I tried that; I thought that it choked because it was finding a '-' character 
where it wasn't expecting one, which is why I renamed it. I'll check it again 
both ways and report back.


> Hey, I don't think it's messy anymore :-) IMO you can remove this comment
> totally.

Oops. :) Oversight. It should have been removed.

> How about "This [% terms.bug %] was initially created"? "log item" doesn't
> sound familiar to me.

Absolutely. 'log item' is local usage. (I thought I fixed that...)


So... where do we go from here, boss? Fix up these nits and call it a day? Or 
another complete re-write to eliminate clone-link altogether and instead pass a 
bug_id into enter_bug? IF that's the case, we're back to updating attachment 
#163092 [details] [diff] [review] (and the objections to it in comment #37 and comment #39). That patch 
assumed that you'd be entering the bug_id on the enter_bug page, but most of 
the code would be the same (I think) if the bug_id was instead getting passed 
up the chain. 

Please take a look at that patch in light of these later discussions and tell 
me if you think I'm on the right track.

> So, by my reading of your comments, this patch is basically ready to go 
> (ready to go, barring a couple of nits)... and we're going to get rid of 
> it because we can't use it to get the first comment. That right?

I'm not sure what you refer to with "it" here - get rid of what? The patch? ;-) 

> > Either you make Clone link a button and use HTTP post to convey the 
> > information OR you pass a bug ID and retrieve the comment from the DB at 
> > enter_bug. I strongly prefer the first one, since I don't think it's 
> > necessarily worthy to bloat the show_bug HTML by including the 
> > basic bug information and comment 0 twice.
> Some confusion here: did you mean to say, "I strongly prefer the second 
> one?" 

Yes, of course. Sorry :-( Using an additional HTML form would be bloat.

> > I also noted that security group and time tracking information doesn't get
> > cloned. It's an interesting question whether or not they should (then again,
> > why not?), but as the amount of fields to clone grows, I'm more and more
> > starting to think about the DB-read approach.
> The security group is just a non-product group, isn't it? Nothing inherently 
> special about it... although it does beg the question as to whether or not a 
> cloned bug should start off with all the same bug groups as its parent. 

If (and when) we're approaching the cloning as pre-filling enter_bug, I believe
the correct approach would be to pass all group information. If (some of) the
groups are not allowed in the new product, they are of course then discarded -
but otherwise, their checkboxes should be checked by default in the enter_bug
form. Naturally, all mandatory groups for the new product need to be
automatically added for the clone, but post_bug takes care of this for us. So,
to simplify: Of the checkboxes normally available in enter_bug, we should check
those which are also checked on the bug we're cloning from. The other group
stuff we can ignore.

From a practical standpoint, many organizations have a "security vulnerability"
-style group in addition to normal product-wise grouping. A clone of security
bug should by default be a security bug - I believe this is what the user
expects as well.

> Time tracking should definitely *not* be copied, for two reasons:
> * First, it would mean that you've got hours for one bug recorded in two
>    places. Time spent on the original and time spent on the clone are
>    tracked in each individual bug.

Yeah, copying the whole time tracking information would probably lead to
undesired results. However, copying the "estimated hours" field isn't probably
such a bad idea: we have that field in enter_bug already, and copying it
wouldn't probably hurt. In fact, it is probably even desirable: If I want the
same fix done in three products, I just want to file it in one place and then
use "clone" to create the two copies. Copying the time estimate is probably the
best we can do.

However, _if_ the "clone" is actually used to fork an already worked-on bug into
two different issues, the result would truly be somewhat confusing. I still
think the best way to do this is copying the estimate and leaving it up to the
user to fix it correctly.

> * Secondly, time tracking is stored in longdescs, and it's not possible
>    to copy a longdesc without taking the comment - can't have a blank
>    longdesc:thetext entry or many things break. Since we're not taking
>    the comments, we *can't* take the time entries.

estimated_time is recorded in bug table, so we won't encounter this issue if we
copy only that.

> I tried that; I thought that it choked because it was finding a '-' character 
> where it wasn't expecting one, which is why I renamed it. I'll check it again 
> both ways and report back.

Probably in the PROCESS directive? Quote the template path.

> So... where do we go from here, boss? Fix up these nits and call it a 
> day? Or another complete re-write to eliminate clone-link altogether 
> and instead pass a bug_id into enter_bug? 

What do you think? Either approach is fine with me. 

I believe we should clone the security groups and the estimated_time field, but
other than those and the couple of nits, I think your patch is fine. The bug_id
solution might be more elegant and enable comment passing, but as I already
said, I believe this approach is useful and worth getting checked in. 

FWIW, I'm not at all concerned about the additional DB retrieval that the bug_id
approach involves, but the code changes for that will be much more complicated.
This one is simple enough that I'd even be ready to ask Dave for a 2.20 approval
if you get the patch done and working soonish.

Two thoughts from a quick review of the patch:

- IMO there's no need for a controlling param. Why would anyone ever want to
turn this off?

- IMO we should force the new bug to be in the same product as the old by adding
&product=<foo> to clone_url. Given that cloned bugs are likely to be very
similar to the original, this is the 99.9% case, and it prevents _everyone_
having to go through the Select Product screen. The 0.1% of people can change
the product afterwards in a separate step. 

Both these suggestions have the pleasant side-effect of making the patch quite a
bit simpler.

Other than that, nice one :-)

Gerv

(In reply to comment #49)
> - IMO there's no need for a controlling param. Why would anyone ever want to
> turn this off?

There's always somebody who wants to disable it. But I think you're still right
- it's cleaner to make the few people do this by customizing the templates.
Let's forget about the param.

> - IMO we should force the new bug to be in the same product as the old by 
> adding &product=<foo> to clone_url. Given that cloned bugs are likely to be 
> very similar to the original, this is the 99.9% case, and it prevents 
> _everyone_ having to go through the Select Product screen. 
> The 0.1% of people can change the product afterwards in a separate step. 

This is more controversial. For bmo, your statement might hold. For other
installations, not at all necessarily. The concept of product can f.e. be "a
client site" - a typical bug might then be "Upgrade library xxx to y.yy". In
this case, the typical cloning scenario is making one bug for each of the products.

This is, of course, customizable through the templates. However, I'm inclined
towards leaving it as it is now. It is probably easier for an admin to realize
and use the possibility to _limit_ the functionality. If we limit clones to the
same product by default, it requires quite complex understanding of many things
to realize that the option could actually also be used for cloning between products.
(In reply to comment #49)

As usual, Jouni seems to speak my mind for me. I have no issues with removing 
the parameter. At the time I thought it was a good idea, but I've sort of 
wondered myself if it was *really* needed as I progressed. (Remember, this bug 
represents my very first attempt at a code patch to Bugzilla, and I'm still 
learning.) I'll get rid of it in the next re-write.

I will fight you tooth and nail on the product change, though, as it is an 
integral part of why we need it locally, and I can't believe that we're unique 
in this regard. 

I will concede that cloning across products may not be very useful on bmo... 
but honestly I cant see bmo really using cloning at all. (In fact, now that I'm 
thinking about it, bmo was one of the reasons I put in the parameter in the 
first place - I figured that cloning bugs would be disabled here. Don't we have 
enough duplication already without allowing the user one-click access to more? 
Hrm... maybe the parameter should stay in after all...)

> Given that cloned bugs are likely to be very
> similar to the original, this is the 99.9% case, and it prevents _everyone_
> having to go through the Select Product screen. The 0.1% of people can change
> the product afterwards in a separate step.

If I agreed with your numbers, I might also agree with your conclusion... but 
even then I think I'd argue in favour of flexibility over restriction. 
It is intuitive to have to pick a bug's product at bug creation; you already 
have to do it with every new bug that you make, so this adds nothing to that 
process. The alternative is to be forced into picking the 'wrong' product then 
going back to fix it later; this option would make no sense to a lot of people, 
and creates a lot more bugmail.

The real kicker is that your scheme could make it so that someone *cannot* 
clone a bug.  It's not difficult to imagine this scenario: Product X and 
Product Y have their own, mutually exclusive groups. Fred (who runs product X) 
gets cc:'d on a bug in Product Y because it seems to be related to his area. 
Normally, Fred can't see Product Y, but since he's in the CC field, he can see 
this particular bug. He agrees that it affects Product X, and goes to clone the 
bug.

If the product is set at the time of the cloning, Fred will be told that he 
doesn't have the permissions to clone the bug, since he can't create a bug in 
the new product. Furthermore, while Jim (the head opf Project Y) can clone the 
bug, he can't change the product to Product X because he doesn't have access to 
that product group.

If the product is *not* set at the time of cloning, Fred hits the 'Clone this 
bug' link, and is taken to the product choice screen... where only Product X 
shows up for him. He clones the bug into Product X and continues on his way.

Finally: It's easier for a customizer to make something *more* restrictive than 
it is to make it *less* restrictive. If a given site wants to fix the product, 
they certainly can... and without having to understand too much of the code or 
patch either. 


> Both these suggestions have the pleasant side-effect of making the patch
> quite a bit simpler.

Gotta say, I don't consider the removal of one line from choose-
product.html.tmpl to make things *that* much simpler. ;)

If you'd like, I can even include the following in clone-link.html.tmpl:

[%# Uncomment the following if you do not want to allow 
    cloning into different products %]
[%# product = bug.product FILTER url_quote %]
[%# clone_url = clone_url _ '&product=' _ next_param %]
 
That should make it trivially easy for people to alter it to fit their 
specifications... although I confess I don't recall seeing anything like this 
anywhere else. Would this make it easier to swallow?


> Other than that, nice one :-)

Thanks!

OK, I'm sold. Remove the param, leave the product stuff as-is, and add the
comment as you suggest. :-)

Gerv
Depends on: 276446
Attached patch Code patch for tip, take 3 (obsolete) — — Splinter Review 
We're back to getting the information from the database, but in a much more
elegant way. I've tested this in as many ways as I can think of testing, and
cleaned up the code so it's as pretty as I can make it; hoping that this is an
easy review, Jouni. :)

Obsoleting Andreas' initial patches now, since a) I'm able to, and b) I'm
confident I'll be able to see this through to the end.
Attachment #45537 - Attachment is obsolete: true
Attachment #45539 - Attachment is obsolete: true
Attachment #168881 - Attachment is obsolete: true
Attachment #171770 - Flags: review?(jouni)
I seem to have a problem with posting 3rd revisions... this isn't the first
time I screwed it up. (The deleted one was unedited and contained some
passwords, which is why Dave deleted it for me. Thanks dave!)
Attachment #171770 - Attachment is obsolete: true
Attachment #171774 - Flags: review?(jouni)
Attachment #171770 - Flags: review?(jouni)
Comment on attachment 171774 [details] [diff] [review]
Code patch for tip, take 3 (for real)

Looking pretty good...

>+# If a user is trying to clone a bug
>+#   Check that the user has authorization to view the parent bug
>+#   Create an instance of Bug that holds the info from the parent
>+$cloned_bug_id = $cgi->param('cloned_bug_id') || 0 ;
>+
>+if ($cloned_bug_id > 0) {

You don't need the "|| 0" or "> 0".

>+if ($cloned_bug_id > 0) {
>+
>+    $default{'component_'}    = $cloned_bug->{'component'};
>+    $default{'priority'}      = $cloned_bug->{'priority'};
>+    $default{'bug_severity'}  = $cloned_bug->{'bug_severity'};
>+    $default{'rep_platform'}  = $cloned_bug->{'rep_platform'};
>+    $default{'op_sys'}        = $cloned_bug->{'op_sys'};

All these make me think it might be time to change the interface to the
template. In other words, simply do:

$vars->{'default'} = $cloned_bug;
and then have the template do [% default.component %], [% default.op_sys %]
etc. It seems to me to make semantic sense for the defaults for a bug entry
form to be contained in something that is, or looks like, a Bug object.

>+# We need to ensure that we respect the 'insider' status of
>+#  the first comment, if it has one. Either way, make a note
>+#  that this bug was cloned from another bug.

Nit: leading spaces.

>+
>+    $cloned_bug->longdescs();
>+    my $desc                  = "+++ This bug was initially created " .
>+                                "as a clone of bug #$cloned_bug_id +++\n\n";

This is an l10n problem. I know we do it elsewhere, but we shouldn't compound
the problem. Can you please execute a template and get the returned string?

>+    my $isprivate             = $cloned_bug->{'longdescs'}->[0]->{'isprivate'};
>+
>+    $vars->{'comment'}        = $desc;
>+    $vars->{'commentprivacy'} = 0;
>+
>+    if ( ( $isprivate <= 0 ) ||
>+         ( 
>+          ( Param("insidergroup") ) && 
>+          ( UserInGroup(Param("insidergroup")) ) 
>+         ) 
>+       ) {

I don't know about anyone else, but this brace style has me completely
flummoxed. :-) Can we use something a bit more like existing examples?

Gerv
Attachment #171774 - Flags: review-
Attached patch Code patch for tip, take 4 (obsolete) — — Splinter Review 
(In reply to comment #55)

> You don't need the "|| 0" or "> 0".
Stripped.

> All these make me think it might be time to change the interface to the
> template. 
Perhaps it is; I can even see your logic. I do hope, however, that you're not 
proposing it be done as part of *this* bug, are you? Seems to me like that's
a whole 'nother issue...

> Nit: leading spaces.
Local style - reverse-indent comment paragraphs. I sort of like it, (and did
it without thinking), but it's not a gamebreaker for me. Removed.

> >+
> >+	$cloned_bug->longdescs();
> >+	my $desc		  = "+++ This bug was initially created " .
> >+				    "as a clone of bug #$cloned_bug_id
+++\n\n";
> This is an l10n problem. I know we do it elsewhere, but we shouldn't compound

> the problem. Can you please execute a template and get the returned string?

mutter grumble... fix the world's problems on the back of my bug... ;)

Alright, I moved the string down into create.html.tmpl; it is prepended to the
comment if cloned_bug_id exists. Someone still has to replace the string if
they
want to localize, but now they do it in a template instead of a .cgi file. Does

that satisfy the requirement?


> I don't know about anyone else, but this brace style has me completely
> flummoxed. :-) 
Indented by clause... perfectly readable to me, but then again I wrote it.
Changed somewhat for the hard-of-deciphering. ;-)

I also fixed a couple of other things that I found I hadn't tested - you can't 

take the defauly bug_status from the clone, for example, because then you could

end up with a new bug that's trying to start out with a status of RESOLVED
(which breaks all sorts of things). Fixed now.
Attachment #171774 - Attachment is obsolete: true
Attachment #171822 - Flags: review?(jouni)
Attachment #171774 - Flags: review?(jouni)
Comment on attachment 171822 [details] [diff] [review]
Code patch for tip, take 4

This crashes with "Can't use an undefined value as an ARRAY reference at
/var/www/bugzilla/tip/enter_bug.cgi line 357." if you have no CC list.


The first line of the actual comment is strangely indented. If my first comment
is:

--
Test content, line 1
Test content, line 2
Test content, line 3
--

The cloned result becomes:

--
+++ This bug was initially created as a clone of Bug #22. +++

    Test content, line 1
Test content, line 2
Test content, line 3 
--


>Index: enter_bug.cgi
>===================================================================
>+    $vars->{'dependson'}      = $cloned_bug_id;

WHAT? This causes pretty interesting results.

>+   $cloned_bug->longdescs();
>+    my $isprivate             = $cloned_bug->{'longdescs'}->[0]->{'isprivate'};

Nit: Indent the first line properly.

>+        $vars->{'comment'}        .= $cloned_bug->{'longdescs'}->[0]->{'body'};
>+        $vars->{'commentprivacy'} = $isprivate;

Nit: This would look much more cool if the =s were aligned ;-)

>+    # Use the version specified in the URL, if one is supplied. If not,
>+    # then use the cookie-specified value. (Posting a bug sets a cookie
>+    # for the current version.) If no URL or cookie version, the default
>+    # version is the last one in the list (hopefully the latest one).
>+    # Eventually maybe each product should have a "current version"
>+    # parameter.

I don't think this code works for me. If the version set in the originating
product is not available in the new product, nothing is set and an ugly error
is shown (unless I manually pick something).


Starts looking good though :-)
Attachment #171822 - Flags: review?(jouni) → review-
Attached patch Code patch for tip, take 5 (obsolete) — — Splinter Review 
(In reply to comment #57)
> (From update of attachment 171822 [details] [diff] [review] [edit])
> This crashes with "Can't use an undefined value as an ARRAY reference at
> /var/www/bugzilla/tip/enter_bug.cgi line 357." if you have no CC list.

argh. good catch. Fixed.


> The first line of the actual comment is strangely indented. 
No indication in the .tmpl file as to why it was doing this. I re-wrote
this chunk of text, and now it doesn't add any extra spaces. (Looks a lot
nicer too! :)

> Nit: This would look much more cool if the =s were aligned ;-)
Done deliberately as I did not want people to mistake the ".=" for an "=".

It's unnecessary now that I no longer pre-fill the comment with the 
"+++ This bug..." string (as of previous revision), so I changed it to an '='
and lined it all up real pretty. :)


> >+	$vars->{'dependson'}	  = $cloned_bug_id;
> 
> WHAT? This causes pretty interesting results.

Alright, long conversation in IRC about this between Jouni and I, here are the
results.

As per comment #14, attachments/comments/votes/dependencies are handled
differently than everything else (which, as near as possible, is just copied
directly from the parent into the child).

attachments: aren't taken at all.
votes: aren't taken at all.
comments: first comment only is taken, and prepended with a string to indicate
that it is a copy of a comment in another bug. If the cloning user can't see
the first comment, only the prepended string exists.

Dependencies... aye, there's the rub. Jouni was of the opinion that a cloned
bug should have the same dependencies as its parent; it is, after all, supposed
to be a clone/copy. I do not think that will always (or even usually) be useful
information, and I also think that there should be a direct relationship
between a cloned bug and parent; as such, I have made the cloned bug depend on
the parent itself with no other dependencies or blockages.

Whichever way is chosen it is going to be useful for some situations and not
for others... so Jouni has agreed not to deny review based on this criteria,
and to let Dave decide if he feels strongly about it one way or the other when
giving approval. 


> I don't think this code works for me. If the version set in the originating
> product is not available in the new product, nothing is set and an ugly error

> is shown (unless I manually pick something).

Right. That was the intention -- to ensure that you manually picked something.
I am aware, however, that this is different behaviour from the default entry
form. This section is modified now so that it will always pick a version again
-- either the same version as the parent (if the product is the same) or a
version based on the rules that were here before.

Onward!
Attachment #171822 - Attachment is obsolete: true
Attachment #171917 - Flags: review?(jouni)
Comment on attachment 171917 [details] [diff] [review]
Code patch for tip, take 5

Your patch to bug 266579 made this bitrot (hunk 6 of enter_bug doesn't apply),
and applying this by hand seems non-trivial, so I can't test this reliably
enough to r+ anyway. By inspection it looks fine apart from the single issue
mentioned below. I'll test the next patch, then.

BugMail.pm:
-----------

> # disable email flag for offline debugging work
>-my $enableSendMail = 1;
>+my $enableSendMail = 0;

Let's not check this in, please. :-)
Attachment #171917 - Flags: review?(jouni) → review-
(In reply to comment #56)
> Perhaps it is; I can even see your logic. I do hope, however, that you're not 
> proposing it be done as part of *this* bug, are you? Seems to me like that's
> a whole 'nother issue...

Well, it would mean you made a different set of changes to the template
interface instead of the set you are making at the moment. But I suppose it can
wait.

> mutter grumble... fix the world's problems on the back of my bug... ;)

Not at all - I'm just asking you not to make things any worse :-) I didn't ask
you to fix all the other occurrences of this, after all... ;-)

> Does that satisfy the requirement?

Yes - the text to change has to be in a template. That's all it is.

Gerv
Attached patch Code patch for tip, take 6 (obsolete) — — Splinter Review 
- Updated to HEAD (as of the time of creation)
- Removed the BugMail oopsie from the patch.
Attachment #171917 - Attachment is obsolete: true
Attachment #172236 - Flags: review?(jouni)
Attachment #172236 - Flags: review?(jouni)
As above, plus:
- add a line to filterexceptions so that the test suite runs successfully.
Attachment #172236 - Attachment is obsolete: true
Attachment #172237 - Flags: review?(jouni)
Comment on attachment 172237 [details] [diff] [review]
Code patch for tip, take 7

r=jouni
Attachment #172237 - Flags: review?(jouni) → review+
Dave/Myk;

Before approving, please take into account the 'dependencies' issue raised in 
comment #58. If you want it addressed differently than this patch handles it, 
speak now or forever hold your peace. ;)
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
I like the way you did dependencies...  this is all just preloading a new bug
form for you, correct?  So the user can still change things before the new bug
is actually created?  If that's how it works, consider it a+'d.  We can hash out
the little stuff in individual bugs if people don't like specific things about
the way it works.
Flags: approval? → approval+
Thank you to everyone who helped me shepherd my first significant enhancement 
attempt through the process. When I started, I had never even made a patch file 
before... and now I'm checking in the bug myself. :) Short time, long way, lot 
of learning taken place.

Onward and upward!!

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.101; previous revision: 1.100
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <-
-  filterexceptions.pl
new revision: 1.32; previous revision: 1.31
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--
  knob.html.tmpl
new revision: 1.14; previous revision: 1.13
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.41; previous revision: 1.40
done
Checking in template/en/default/global/choose-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/choose-
product.html.tmpl,v  <--  choose-product.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 252807 has been marked as a duplicate of this bug. ***
I have found what appears to be a (very small) bug in the patch.

there is a trailing space in the following line:
+            [% IF cloned_bug_id %]&amp;cloned_bug_id=[% cloned_bug_id FILTER
url_quote %][% END %] 

This causes breakage if you pass a format into create_bug.cgi.

I'm not re-opening, since I can't verify that the bug made it into CVS, but I'd
recommend that it be checked for.

Besides that little quirk, bug-cloning is working great, thanks.
Dave; thanks for pointing this out, but I believe it has already been 
identified and fixed as bug 283424. I have marked that bug (and one more that 
pointed out an error in implementation) as being blocked by this one so it's 
easier to spot them.
Blocks: 282983, 283424
Dependecies inheritance declared in
comment 58
(cloned bug depends on his parent) is not useful. 
Bug cloning extremely useful when we, working on some bug, create some subtask, 
cloned from general task. In this case, parent bug must depends from subtask 
(see "timetracking summary" reports and so on).

In our company we use  (enter_bug.cgi)
    $vars->{'dependson'}      = "";
    $vars->{'blocked'}        = $cloned_bug_id;

instead of 

#    $vars->{'dependson'}      = $cloned_bug_id;
#    $vars->{'blocked'}        = "";

and we propose this scheme (or may be make it customizeable by  Bugzilla 
parameters).

(In reply to comment #70)
> Dependecies inheritance declared in comment 58 ... is not useful. 

As was also declared in comment #58:
> Whichever way is chosen it is going to be useful for some situations and not
> for others... 

Dave decided he liked it this way, so this is how it is, and I doubt you'll get 
it changed now.

If you'd like to open up an enhancement request for customizing it via 
parameter, by all means feel free! Personally, I'm not sure there's a need... 
and it looks like you already know how to fix the issue for your site, though, 
(your changes look to me like they should do the trick).

If you do open one, though, please cc: me on it.
Blocks: 355302
Making the dependency behaviour of cloned bugs configurable is bug 428102.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: