251556 - Allow "Bug ID" fields to have one-way mutual relationships (like blocks/dependson)

Status: RESOLVED FIXED

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

Description

[Joe Llywelyn Griffith Blakesley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

This is an RFE for a function to input and display (on bug pages) the bug
numbers of bugs that the current bug is a regression of (and vice versa; i.e.:
bugs that fixing the current bug causes).  This could be implemented in the same
way as dependency information (possibly also with a regression tree).

Below "Bug x depends on:" and "Bug x blocks:" on show_bug.cgi, there would be
"Bug x is a regression of:" (or "Bug x is caused by fixing:") and "Fixing bug x
causes:".


Reproducible: Always
Steps to Reproduce:

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I've recently seen the need for something like this. Since neither "depends on"
or "blocks" is appropriate for indicating which bug caused the regression,
people tend to pick one or the other and it gets confusing.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Trying to deal with bug 284086 is a perfect example of why this is needed. The
bug introduced several regressions, and some of those filed regressions depend
on patches in other filed regression bugs. Ideally, bug 284086 would have a
field for "bugs the fix for this bug caused", and then the "depends on" and
"blocks" field of the regression bugs could be used for proper tracking of the
dependencies.

Comment 3

[Frédéric Buclin]

Adding a dependency on bug 287334, which would fix this bug. I mark it as blocker even if fixing this one doesn't necessarily require bug 287334. But we have no "related bugs" field yet (bug 12286). ;)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that many people have been using dependencies to track regressions (generally using the convention that depends on N == this bug caused regression N; blocks bug N == this bug is a regression from N, which makes sense if you're considering the dependencies in terms of taking the fixes on a branch).  Here's an example of why that's insufficient:
 * bug 321054 is a regression from bug 241518
 * bug 206520 depends on bug 241518
 * bug 321054 depends on bug 206520
Using dependencies for regression information causes that to be a circular dependency, which bugzilla won't allow.

Comment 5

[Bill Gianopoulos [:WG9s]]

I filed bug 405215 as a temporary workaround for the BMO instance until a real fix is available.

Comment 6

[Max Kanat-Alexander]

I'm morphing this bug into an extension of bug 287334. Once we have "Bug ID" fields, we can give them functionality something like how the "blocks/dependson" fields work now.

Comment 7

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson)

Uses custom table in the same one that the mutli-select fields do.  Shows dependency tree but not graph.  The Bug ID field is linkable and allows edit like depends on\blocks fields.

Comment 8

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v2)

Comment 9

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v3)

Comment 10

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v4)

Corrects \'s in new file name

Comment 11

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v5)

Comment 12

[Frédéric Buclin]

Comment on attachment 335013 [details] [diff] [review]
Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v5)

You must update your installation. A lot of stuff in this patch has already been checked in separately. Don't mix everything.

Comment 13

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v6)

Updated installation to remove previously checked-in code.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 335318 [details] [diff] [review]
Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v6)

>-my @custom_fields = grep { $_->type != FIELD_TYPE_MULTI_SELECT }
>+my @custom_fields = grep { $_->type != FIELD_TYPE_MULTI_SELECT &&
>+                           $_->type != FIELD_TYPE_BUG_ID}

  All that stuff (throughout the patch) shouldn't be necessary--we only have one bug_id.

  I need a new patch that assumes there will only be one bug_id. If we want multiple bug ids, that would be a totally separate bug from this one.

>@@ -1394,9 +1400,8 @@
>         $invocant->_check_strict_isolation_for_user($qa_contact)
>-            if (ref $invocant && $id);
>+            if ref $invocant;

  What is that doing here?

Comment 15

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v7)

Bug ID fields only accepts one ID.  Shows dependency relationship in one direction.  For example,  if bug 1 has a Bug ID field with a value of 3 and bug 3 has a Bug ID field with a value of 5, then the tree for bug 1 will show as:
1
 - 3
    - 5

But the inverse relationship is not being shown, e.g. if bug 7 has a Bug ID field with a value of 1, then the relationship 
1
  - 7 
is not being shown.  Is this also required?

Comment 16

[Max Kanat-Alexander]

Yes, I want to see the inverse relationship also.

Comment 17

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v8)

Comment 18

[Max Kanat-Alexander]

Comment on attachment 335683 [details] [diff] [review]
Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) (v8)

>Index: Bugzilla/Bug.pm
>+     # Check if current ID is put into bug id field.
>+    my $bug_id = ref($invocant) ? $invocant->id : 0;
>+    if ($bug_id == $value) {
>+        ThrowUserError("bugid_loop");
>+	}

  Actually, you'll have to be even more strict than that, because there could be a longer loop. I guess you'll have to look at the code in ValidateDependencies, although maybe you could just do something simpler.

>+sub bug_id_field_relationships {

  Let's call that "related_bugs", and return Bug objects. I'd imagine that using Bugzilla::Bug->match would work, although you'd then also have to check visible_bugs and all that.

>Index: Bugzilla/Field.pm
>+    is_relationship => \&_check_is_relationship,

  That can just be \&Bugzilla::Object::check_boolean.

>+sub _check_reverse_relationship_desc {
>+    my ($invocant, $reverse_desc) = @_;
>+    $reverse_desc = clean_text($reverse_desc);
>+    return $reverse_desc;

  Oh, but it also can't be empty.

>Index: Bugzilla/Install/DB.pm
>+    #2008-08-26 [e-mail] - Bug 251556
>+    $dbh->bz_add_column('fielddefs', 'is_relationship',
>+                        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
>+    $dbh->bz_add_column('fielddefs', 'reverse_relationship_desc',
>+                        {TYPE => 'TINYTEXT'});

  Because this changes the fielddefs table, it has to go into a separate function. Look at where the other fielddefs changes are.

>Index: template/en/default/admin/custom_fields/create.html.tmpl
>+    is_relationship.disabled = this_select.selectedIndex != 0;
>+    reverse_desc.disabled = this_select.selectedIndex != 0;

  Wait, are you always assuming that "Bug ID" will be the first item in the list, or...?

>+      <th align="right"><label for="is_relationship">Allow mutual one-way relationships:</label></th>

  Let's call this "Represents a [% terms.bug %] relationship" instead.

>Index: template/en/default/bug/edit.html.tmpl
>+    [% IF field.is_relationship %]
>+    <tr>
>+      <td class="field_label">

  Oh, ugh, I don't like having to separate out this block from field.html.tmpl, but I understand why we have to.

>Index: template/en/default/global/user-error.html.tmpl
>+  [% ELSIF error == "bugid_loop" %]
>+    [% title = "Bug ID Loop Detected" %]
>+    You can't enter [% terms.abug %] into it's own Bug ID custom field.

  Would be ideal to display the field description instead of "its own Bug ID custom field" if possible.

>--- /dev/null	2006-11-30 16:00:00.000000000 -0800
>+++ show_bug_dependency_tree.cgi	2008-08-27 02:40:02.750000000 -0700

  Hmm, we can't just modify the already-existing dependency_tree.cgi?

  In any case, this name is too long, so let's call this relationship_tree.cgi.

>+++ template/en/default/bug/bugid-dependency-tree.html.tmpl	2008-08-27 02:52:37.156250000 -0700

  It's not a dependency tree, it's a relationship tree, and the word "Dependency" shouldn't exist in this template, but should be replaced by the field description in some intelligent way.

Comment 19

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v9

Comment 20

[Max Kanat-Alexander]

Hmm, can the depthControlToolbar BLOCK be shared between the two templates? And is it possible to make both templates into one?

Comment 21

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v10

Corrected some problems with tree display.  Went back to a separate relationship_tree.cgi file instead of trying to modify showdependencytree.cgi as the trees were not being shown correctly.

Comment 22

[Elliotte Martin]

(In reply to comment #20)
> Hmm, can the depthControlToolbar BLOCK be shared between the two templates? And
> is it possible to make both templates into one?
> 

The depthControlToolbar is shared.  It is just shown at the bottom and top, but either bar controls both trees.  I'm not sure what you mean by making both templates into one.  You mean one tree or combine dependency-tree.html.tmpl with relationship-tree.html.tmpl.  

Comment 23

[Max Kanat-Alexander]

I want it to be shared between dependency-graph and relationship-tree templates, as though we were members of the 1990's and we didn't write code twice.

Comment 24

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v11

For now I have removed the relationship tree link and the new files.  I'll clean up the tree templates and submit later.

Comment 25

[Max Kanat-Alexander]

Comment on attachment 335980 [details] [diff] [review]
  Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v11

This doesn't work on bug creation:

Can't use string ("Bugzilla::Bug") as a HASH ref while "strict refs" in use at Bugzilla/Bug.pm line 2366.

Comment 26

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v12

Corrects error when entering Bug ID field on bug creation.

Comment 27

[Max Kanat-Alexander]

Comment on attachment 335993 [details] [diff] [review]
  Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v12

  Okay, this is fine for the bmo customization. Could you backport this to 3.2 and attach it on bugzilla.everythingsolved.com?

  This would need more cleanup to go upstream, I think.

Comment 28

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v13

Reused and modified existing dependency code to also show relationships.

Comment 29

[Elliotte Martin]

Attachment: Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v14

Comment 30

[Albert Ting]

Could somebody confirm that this RFE supports both blocking and non-blocking relationships?  Meaning will I be able to configure a custom bug-id field such that it can be closed, regardless of the bug status for the other tickets (one-way and/or two-way)?

Comment 31

[Albert Ting]

Hi Max/Elliotte, is this patch ready for checkin?  I have a need to add some more bug id's and so this would be a great feature to have released.

Comment 32

[Elliotte Martin]

(In reply to comment #31)
> Hi Max/Elliotte, is this patch ready for checkin?  I have a need to add some
> more bug id's and so this would be a great feature to have released.

I think so.  Should probably be reviewed though.

Comment 33

[Jesse Clark]

Comment on attachment 336794 [details] [diff] [review]
Add custom "Bug ID" field that supports one-way mutual relationships (like blocks/dependson) - v14

> Index: template/en/default/admin/custom_fields/create.html.tmpl
>     'reverse_relationship_desc'

This needs a comma even though it is at the end of a list.

>     var bug_id_field_selected = this_select.options[this_select.selectedIndex].value == 6;

This should refer to [% constants.FIELD_TYPE_BUG_ID %] for readability.

> Index: Bugzilla/Field.pm
>    $params->{reverse_relationship_desc} = 
>              $class->_check_reverse_relationship_desc(
>              $params->{reverse_relationship_desc}, $params->{is_relationship});

I can't manage to actually create such a field unless I also add this before it:

>    $params->{is_relationship} = Bugzilla::Object::check_boolean($params->{is_relationship});

Comment 34

[Elliotte Martin]

Jesse,

Thank you for your review.  I don't have time to work on this now and won't be able to submit a new patch anytime soon. If you're willing, please submit your changes to close this out.

Comment 35

[Jesse Clark]

Attachment: v15

Updated the patch for the head, and verified that creating and using a relationship field works correctly.

Comment 36

[Jesse Clark]

We may want to add an event to a bug's activity log when another bug creates a relationship to it.

Comment 37

[Max Kanat-Alexander]

Comment on attachment 359020 [details] [diff] [review]
v15

Could you take the dependency tree parts out of this and put them into a separate bug? That'll give two simpler patches to review.

Comment 38

[Jesse Clark]

Should we migrate the duplicates table into a field of this type?

Comment 39

[Max Kanat-Alexander]

(In reply to comment #38)
> Should we migrate the duplicates table into a field of this type?

  Well, that would be tricky if Bug ID fields are ever allowed more than one Bug ID in them. It's something we could consider in another bug, in any case.

Comment 40

[Albert Ting]

(In reply to comment #39)
>   Well, that would be tricky if Bug ID fields are ever allowed more than one
> Bug ID in them. It's something we could consider in another bug, in any case.

Does that mean we currently can't replace blocks/dependson with the Custom Bug ID feature?

Comment 41

[Max Kanat-Alexander]

(In reply to comment #40)
> Does that mean we currently can't replace blocks/dependson with the Custom Bug
> ID feature?

  Yeah, currently, we can't. But we'd like to be able to some day.

Comment 42

[Jesse Clark]

Blocks/dependson is a many-to-many relationship, so it can't be stored in a Bug ID field which is many-to-one, although Bug ID fields are ideal for duplicates and regressions.

At NASA we allow a Bug ID field to appear multiple times per bug and it ends up being more complex than is appropriate for core Bugzilla. Instead we may want to generalize the dependency table into an arbitrary link table.

Comment 43

[Albert Ting]

(In reply to comment #41)
> > Does that mean we currently can't replace blocks/dependson with the Custom Bug
> > ID feature?
> 
>   Yeah, currently, we can't. But we'd like to be able to some day.

Ok, I created bug 479550 as I couldn't find a ticket for this.

Comment 44

[Gervase Markham [:gerv]]

The bug system at my last employer had a good way of doing this. You chose both the relationship, from a list, and the bug number. So you could say:

[Blocks] 23456, 245732
[Regression Of] 127644
[Duplicate Of] 76743

You could also define new relationships, so for example one Bugzilla might choos to have a "Documented by" flag. One can also imagine them being displayed on the other bug with a defined string indicating the inverse relationship, so if bug 12345 was:

[Regression of] 54321

then 54321 would say:

[Caused regression(s)] 12345

Implementing a system like that would allow us to generalise several existing specific bug relationships we have (blocks/dependson, duplicate, related) into one generic system, which I think would be an improvement. 

Gerv

Comment 45

[Jesse Clark]

Let me update this for bitrot and modularity. The field type code should be the same regardless of frontend widgets.

Comment 46

[Jesse Clark]

Attachment: v16

This is a minimal implementation of describable relationships. There is also some future-proofing compared to v15, based on some things we have been using these fields for in the last year, such as links to external bug trackers, multiple links to the same bug, and field data taken from linked bugs.

Comment 47

[Max Kanat-Alexander]

Comment on attachment 421175 [details] [diff] [review]
v16

>Index: Bugzilla/Bug.pm
> sub _check_bugid_field {
>-    my ($invocant, $value, $field) = @_;
>+    my ($self, $value, $field) = @_;

  No reason to change that--we call it $invocant when we don't know whether it's an object or class.

>+    my $ids = {$self->bug_id =>1};

  Nit: Spacing.

>+    relationship_loop_detect($field, $self->bug_id, $ids, $value);

  I'd call it _check_relationship_loop--for functions that throw an error, we use "check".

>+    return $self->check($value, $field)->id;

  I think it's a bit logically odd, to me as a code reader, to have this statement in the function twice. Also, it seems like we should make sure that it's a valid bug ID before we ever call relationship_loop_detect.

>+sub relationship_loop_detect {
>+    # Generates a dependency tree for a given bug.  Calls itself recursively
>+    # to generate sub-trees for the bug's dependencies.
>+    my ($field, $bug_id, $ids, $dep_id) = @_;

  $ids is not a necessary argument; you can generate that within the function.

>+    # Don't do anything if this bug doesn't have any dependencies.
>+    return unless defined($dep_id);

  Nit: Parens aren't necessary.

>+
>+    # Get this dependency's record from the database
>+    if (!$ids->{$dep_id}) {
>+        $ids->{$dep_id} = 1;
>+        $bug_id = $dep_id;

  I think it might be less confusing, for any future modifications of this subroutine, to not overwrite this variable.

>+        my $dbh = Bugzilla->dbh;
>+        detaint_natural($dep_id);

  If we call relationship_loop_detect first, above, we won't have to worry about detaint_natural. Also, we could pass in a bug object instead of a bug ID, and then that would speed up and simplify what will probably be the most common case, which is a one-level-deep check.

>+        my $field = new Bugzilla::Field({'name' => $field});

  Why not just call check() there? Would be simpler, I think.

>+        ThrowUserError("relationship_loop_single", {'bug_id' => $bug_id,
>+                        'dep_id' => $dep_id, 
>+                        'field_desc' => $field->description});

  You shouldn't be passing $field->description to a template, you should just be using field_descs in the template.

>+sub related_bugs {
>+    my ($self, $relationship) = @_;
>+    unless (ref $relationship eq "Bugzilla::Field") {

  Calling "ref $object eq 'Class'" isn't something we should be doing--that busts up inheritance. I think all you need to do here is check "blessed $relationship" and that's it.

>+        # if not a ref assume we are given a field name
>+        $relationship = Bugzilla::Field->new({name => $relationship});

  And that should be check().

>+    return [] if $self->{'error'};

  That should probably just be at the top of the method.

>+        ORDER BY bug_id", undef, $self->bug_id);

  It's more normal to use $self->id now.

>Index: Bugzilla/Field.pm
> [snip]
>+sub _check_reverse_relationship_desc {
> [snip]
>+    my $type;
>+    if (ref $invocant) {
>+        $type = $invocant->type;
>+    }
>+    elsif (defined $params) {
>+        $type = $params->{type};
>+    }
>+    else {
>+        $type = FIELD_TYPE_BUG_ID;
>+    }

  There's no need for that final else there. Either $invocant is blessed or it's not.

>+    unless ($type == FIELD_TYPE_BUG_ID)

  Nit: It would be more normal to do an if !=.

>+sub is_relationship  {     

>+    my ($self) = @_;
>+    my $reverse_relationship_desc = $self->reverse_relationship_desc();

  Nit: No (). Also might want to just call the variable $desc for brevity's sake.

>+Applies only to fields of type FIELD_TYPE_BUG_ID, 
>+    FIELD_TYPE_EXTERNAL_RECORD_ID,
>+    FIELD_TYPE_FIELD_LINK.

  Those other types don't exist for us.

>+Describes the reverse relationship if this field has is_relatiosnhip set to

  Typo in "relationship".

  Also, we don't set is_relationship on fields, in this patch (which I think is sensible).

>@@ -846,6 +914,12 @@
>         $class->_check_value_field_id($params->{value_field_id},
>             ($type == FIELD_TYPE_SINGLE_SELECT 
>              || $type == FIELD_TYPE_MULTI_SELECT) ? 1 : 0);
>+
>+    $params->{reverse_relationship_desc} = $class->_check_reverse_relationship_desc(
>+        $params->{reverse_relationship_desc}, 'reverse_relationship_desc', $params);

  I think it would be better to directly pass in $type, if that's what the method needs, no? (Although maybe we should modify the system to always pass in $params, and that would solve most of our needs for run_create_validators...hmm.... Well, that's to think about in another bug.

>Index: Bugzilla/DB/Schema.pm
>+            reverse_relationship_desc => {TYPE => 'TINYTEXT'},

  Oh, don't use TINYTEXT, use a varchar(255) if that's explicitly what you mean.

>Index: template/en/default/admin/custom_fields/cf-js.js.tmpl
> [snip]
>+   var reverse_desc = document.getElementById('reverse_desc');

  Perhaps we should call the HTML field reverse_relationship_desc? Or call the database field reverse_desc (which would probably be better).

>+   if (type_field.value == [% constants.FIELD_TYPE_BUG_ID %])

  I suspect tests will complain about filtering, there.

>Index: template/en/default/admin/custom_fields/create.html.tmpl
>+      <th align="right" style="vertical-align:top;">
>+        <label for="reverse_desc" id="reverse_desc_label">Reverse<br/>Relationship<br/>Description:</label>

  This is gonna need some sort of in-page help, or it's going to be really confusing to people.

>+        <input type="text" id="reverse_desc" name="reverse_desc" value="" size="40" disabled=true>

  disabled="disabled" I believe, no? (Needs quotes in any case.)

>@@ -146,4 +155,9 @@
>+[%# set initial editability of fields such as Reverse Relationship Description %]
>+<script>

  You need a type="text/javascript" there. Also, seems like it would be better to make than an ONDomReady, no?

>Index: template/en/default/admin/custom_fields/edit.html.tmpl
> [snip]
>+    [% IF field.type == constants.FIELD_TYPE_BUG_ID %]
>+       <tr>
>+        <th align="right" style="vertical-align:top;">
>+            <label for="reverse_desc">
>+            Reverse<br/>Relationship<br/>Description:

  I think it might make more sense to set some CSS widths on the columns than to force line breaks in the HTML.

>Index: template/en/default/global/user-error.html.tmpl

>+    [%+ dep_id FILTER html %][% " row $dep_row" IF dep_row %].

  What's dep_row?

Comment 48

[Jesse Clark]

Attachment: v17

Thanks for the review. I have made the suggested changes, and some readability improvements.
I also included the reverse link display on the edit bug page in this patch, because while testing this I felt it was absurd to have no display whatsoever.

>   I think it would be better to directly pass in $type, if that's what the
> method needs, no? (Although maybe we should modify the system to always pass in
> $params, and that would solve most of our needs for
> run_create_validators...hmm.... Well, that's to think about in another bug.
> 

I have changed it to only take $type, but full access to params would probably be a good policy.
It is currently pretty difficult to write this kind of validator, because it needs to be called in Bugzilla::Object context without damaging its arguments, then be called in Bugzilla::Field context and remove invalid arguments.

Comment 49

[Jesse Clark]

Attachment: v18

used YAHOO.util.Event.onDOMReady instead of broken code on the create custom field page

Comment 50

[Max Kanat-Alexander]

Comment on attachment 421568 [details] [diff] [review]
v18

  Wow, yes, this is definitely an excellent cleanup, and I agree that it should show up on show_bug for this patch--I didn't realize that that had been taken out of the last one.

  Just a few notes:

>Index: Bugzilla/Field.pm
>+sub _check_reverse_relationship_desc {
> [snip]
>+    if (defined($type) && $type != FIELD_TYPE_BUG_ID) {
>+        return undef;
>+    }

  Shouldn't that be a "!defined($type) ||" instead?

>+sub is_relationship  {     
>+    my $desc = $self->reverse_relationship_desc();

  Nit: No ().

>+    if (defined $desc && $desc  ne "") {

  Nit: Extra space before ne. Also, for clarity we should probably put parens like defined($desc), otherwise the precedence isn't clear to me (and might be wrong).

>+=item C<reverse_relationship_desc>
>+
>+Applies only to fields of type FIELD_TYPE_BUG_ID.
>+Describes the reverse relationship if this field.

  Typo: if -> of

>+    $params->{reverse_relationship_desc} = $class->_check_reverse_relationship_desc(
>+        $params->{reverse_relationship_desc}, 'reverse_relationship_desc', $type);

  It could just always expect $type as the second argument.

>Index: template/en/default/admin/custom_fields/cf-js.js.tmpl
>+   var reverse_desc = document.getElementById('reverse_desc');

  Oh, what I meant here was this:

  Let's rename the DB field to reverse_desc and use that everywhere instead of reverse_relationship_desc.

>Index: template/en/default/admin/custom_fields/create.html.tmpl
>+        <input type="text" id="reverse_desc" name="reverse_desc" value="" size="40" disabled="disabled">

  Nit: value="" isn't necessary.

>@@ -146,4 +157,9 @@
>   <a href="editfields.cgi">Back to the list of existing custom fields</a>
> </p>
> 
>+[%# set initial editability of fields such as Reverse Relationship Description %]
>+<script type="text/javascript">
>+YAHOO.util.Event.onDOMReady(function() {onChangeType(document.getElementById('type'))});
>+</script>

  That should probably go at the top of the file, since that's usually where we put whole-page JS.

>Index: template/en/default/admin/custom_fields/edit.html.tmpl
>+        <th align="right" style="vertical-align:top;white-space:normal;">

  Unless we're already using style="" in this file, let's avoid it.

>+          <label for="reverse_desc" id="reverse_desc_label">Reverse Relationship Description:</label>

  Hmm, any reason to give the label an id?

>+        <td>
>+          Use this label for the list of [% terms.bugs %] that link to this [% terms.bug %].

  Hmm, not quite clear enough--I think the description from the POD might be a bit clearer--the one that has an example.

>Index: template/en/default/bug/edit.html.tmpl
>+    [% IF field.is_relationship %]

  Hmm, any reason that this shouldn't be in field.html.tmpl instead? (This is an honest question, I'm not sure.)

>+      <td class="field_label">
>+        <b>[% field.reverse_relationship_desc FILTER html %]</b>:
>+      </td>

  Let's use th instead of a <td> with a <b>. Also, field_label is already bold, I think.

>+        [% FOREACH depbug = bug.related_bugs(field) %]
>+           [% depbug FILTER bug_link(depbug) FILTER none %][% " " %]
>+        [% END %]

  Hmm, is this the same code that the current dep lists use? I think there's more advanced code for those.

Comment 51

[Jesse Clark]

Attachment: v19

>  Let's rename the DB field to reverse_desc and use that everywhere instead of
>reverse_relationship_desc.

Ah ok, I also added some migration code for PRACA, I don't know whether you want it upstream, for anyone else who might have been using preliminary versions.

>     $dbh->bz_add_column('fielddefs', 'reverse_desc', {TYPE => 'varchar(255)'});
>+    if ($dbh->bz_column_info('fielddefs', 'reverse_relationship_desc')) {
>+        print "Copying 'reverse_relationship_desc' to 'reverse_desc'...\n";
>+        $dbh->do('UPDATE fielddefs SET reverse_desc = reverse_relationship_desc');
>+        $dbh->bz_drop_column('fielddefs', 'reverse_relationship_desc');
>+    }

>  It could just always expect $type as the second argument.

For now, the first arguments have to conform to the spec used in Bugzilla::Object->create().
This could be addressed in bug 539865.

>  Unless we're already using style="" in this file, let's avoid it.

It's not already using style="", but it does have some direct markup, like <table border="0" cellspacing="0" cellpadding="5">.
I'd prefer to leave it like this, rather than apply separate CSS to the first and third column in order to keep it readable.

>  Hmm, any reason that this shouldn't be in field.html.tmpl instead? (This is
>an honest question, I'm not sure.)

The reverse display is similar to a second call to field.html.tmpl, but we don't necessarily want it to follow the same visibility behavior as the forward link.
(Maybe a bug is not in a valid status to be marked as a dup, but we still want to see dups of it.)
In PRACA I have an additional argument to field.html.tmpl to show the reverse link instead of the forward link, and it requires a lot of extra logic.
Putting it here gives us the same display as we currently get for blocks/depends, with minimal indirection.
We may want to revisit this for many-to-many relationships.

Comment 52

[Max Kanat-Alexander]

(In reply to comment #51)
> Ah ok, I also added some migration code for PRACA, I don't know whether you
> want it upstream, for anyone else who might have been using preliminary
> versions.

  Nah, no need to migrate in checksetup from some customization that was only extant in a patch here.

> For now, the first arguments have to conform to the spec used in
> Bugzilla::Object->create().

  Well, no, they don't, really. The method doesn't do anything with that second argument, ever.

> >  Unless we're already using style="" in this file, let's avoid it.
> 
> It's not already using style="", but it does have some direct markup, like
> <table border="0" cellspacing="0" cellpadding="5">.

  Those are valid HTML Strict, actually. Though they shouldn't be being used, and should both be 0.

> I'd prefer to leave it like this, rather than apply separate CSS to the first
> and third column in order to keep it readable.

  Mmm, style="" is pretty much a bad thing; it's very difficult to override it even in a JetPack or custom CSS or anything. I'd rather have a class here and then some rule in a CSS file.

> The reverse display is similar to a second call to field.html.tmpl, but we
> don't necessarily want it to follow the same visibility behavior as the forward
> link.

  Okay. However, it does need to respect the visibility_controller of the parent field, so maybe it would be best to just have it in field.html.tmpl.

> In PRACA I have an additional argument to field.html.tmpl to show the reverse
> link instead of the forward link, and it requires a lot of extra logic.

  Oh, I wouldn't call field.html.tmpl twice. I'd just add this to the FIELD_TYPE_BUG_ID section directly.

Comment 53

[Max Kanat-Alexander]

Comment on attachment 421964 [details] [diff] [review]
v19

r- just based on my last comment response.

Comment 54

[Jesse Clark]

Attachment: v20

Moved styles for the edit custom field page to global.css.

Moved the reverse relationship display to bug/field.html.tmpl. I feel bad about closing a <tr> in a different file from the one it was opened in, but this keeps the template code simple compared to any alternative I could think of. I check [% bug.id %] to determine whether to treat the bug as an object, is that idiomatic?

Comment 55

[Max Kanat-Alexander]

Comment on attachment 422456 [details] [diff] [review]
v20

>Index: Bugzilla/Bug.pm
> sub _check_bugid_field {
>     my ($invocant, $value, $field) = @_;
>     return undef if !$value;
>+    
>+    # check for loop (can't have a loop if this is a new bug)
>+    if (ref $invocant) {
>+        detaint_natural($value);
>+        _check_relationship_loop($field, $invocant->bug_id, $value);
>+    }
>+
>+    # check that the value is a valid, visible bug id
>     return $invocant->check($value, $field)->id;

  I'm pretty sure that the check for whether or not this is a valid bug id should happen before we check for a relationship loop.

>+    $ids->{$bug_id} = 1;

  Hmm, so autovivification works even on totally undefined variables? I think I knew that, but maybe not. :-) (I usually avoid it because it's mystifying to inexperienced Perl coders.)

>+sub related_bugs {
> [snip]
>+        $self->{'related_bugs'}->{$field_name} = $dbh->selectcol_arrayref(
>+            "SELECT bug_id FROM bugs WHERE $field_name = ? 
>+            ORDER BY bug_id", undef, $self->id);

  Nit: Usually the spacing on that is that the "Y" in "BY" would align with the "T" in "SELECT".

  Also, why not use $class->match, here? I think it would be more useful to return objects, but maybe not?

>Index: Bugzilla/Field.pm
> [snip]
>+sub _check_reverse_desc {
>+    my ($invocant, $reverse_desc, $fieldname, $type) = @_;
>+    
>+    if (blessed $invocant) {
>+        $type = $invocant->type; # type is not submitted for existing fields
>+    }
>+    

>+    # give the caller the benefit of the doubt if it is unable to send $type
>+    # (such as when called in Bugzilla::Object context on field creation)

  This doesn't look like you're giving them the benefit of the doubt; it looks like this just handles the case where there's no "type" argument specified to "create".

>+=item C<reverse_desc>
>+
>+Applies only to fields of type FIELD_TYPE_BUG_ID.
>+Describes the reverse relationship if this field.

  Typo: s/if/of/

>Index: Bugzilla/DB/Schema.pm
>+            reverse_desc => {TYPE => 'varchar(255)'},

  I'm trying to think of what it gets us to have a reverse_desc that's NULL instead of just an empty string. (That is, whether or not we should just default this field to an empty string.) I agree that conceptually, it makes more sense to have it be NULL for a non-bug_id-type field, but I'm not sure what it's getting us.

  Also, 255 characters is way too long for a field description--is that the limit for fielddefs.description?

>Index: skins/standard/global.css
> [snip]

  I think that I may have recently added an admin.css or something where these rules would be more appropriate. If I haven't, this would be a fine time to add one.

>Index: template/en/default/admin/custom_fields/create.html.tmpl
>-  <table border="0" cellspacing="0" cellpadding="5">
>+  <table border="0" cellspacing="0" cellpadding="5" id="edit_custom_field">

  If you feel like it, this would be an OK time to remove the cellpadding too (by setting it to 0), but it's absolutely not a required part of this patch in any way. :-)

>@@ -205,11 +205,25 @@
>+[%# for reverse relationships, we show this pseudo-field after the main field %]
>+[% IF bug.id && field.is_relationship %]
>+    </tr> [%# this row was started in bug/edit.html.tmpl %]

  Ahh, okay, yes, seeing this, I understand why you put this code into bug/edit.html.tmpl instead. I now agree with you that that is where it should be instead--this seems too hacky to me, trying to figure this stuff out inside of field.html. The only other place that would then matter is show-multiple.html.tmpl, so perhaps this stuff could go into its own template and be used by both edit.html.tmpl and show-multiple.

Comment 56

[Gervase Markham [:gerv]]

Jesse: is there a public installation of Bugzilla somewhere, on landfill or elsewhere, where we can see this in action and try it out?

Gerv

Comment 57

[Jesse Clark]

https://landfill.bugzilla.org/relationships/

I have set up a few bugs as "chicks" and "hens" of each other.

Comment 58

[Jesse Clark]

Attachment: v21

>   Also, why not use $class->match, here? I think it would be more useful to
> return objects, but maybe not?

My intuition from PRACA is that objects have more overhead, especially because match() doesn't use a request cache. This function can be called recursively, so I'd like to optimize for performance and memory use. Also, I am concerned about code parallelism with many-to-many relationships which will not be able to use match().

> 
>   This doesn't look like you're giving them the benefit of the doubt; it looks
> like this just handles the case where there's no "type" argument specified to
> "create".
> 

Yeah, the inheritance is pretty opaque. How would you suggest structuring this function so that it accepts any $reverse_desc in Bugzilla::Object context but verifies by type in Bugzilla::Field context? This currently gives the correct behavior, but the code could be cleaned up quite a bit after Bug 539865 is taken care of.

> 
>   I'm trying to think of what it gets us to have a reverse_desc that's NULL
> instead of just an empty string. (That is, whether or not we should just
> default this field to an empty string.) I agree that conceptually, it makes
> more sense to have it be NULL for a non-bug_id-type field, but I'm not sure
> what it's getting us.
> 
>   Also, 255 characters is way too long for a field description--is that the
> limit for fielddefs.description?
> 

fielddefs.description is TINYTEXT, so I'll set reverse_desc to TINYTEXT also.

>   I think that I may have recently added an admin.css or something where these
> rules would be more appropriate. If I haven't, this would be a fine time to add
> one.
> 
>   If you feel like it, this would be an OK time to remove the cellpadding too
> (by setting it to 0), but it's absolutely not a required part of this patch in
> any way. :-)

Moved styles to admin.css, but I had no luck getting them to look good without the cellpadding.

> 
>   Ahh, okay, yes, seeing this, I understand why you put this code into
> bug/edit.html.tmpl instead. I now agree with you that that is where it should
> be instead--this seems too hacky to me, trying to figure this stuff out inside
> of field.html. The only other place that would then matter is
> show-multiple.html.tmpl, so perhaps this stuff could go into its own template
> and be used by both edit.html.tmpl and show-multiple.

I've tried a compromise between placing it directly in the format files and creating a new file. They style is maybe too PRACA-ish, but it has the right topology in multiple contexts. Let me know what you think.

Comment 59

[Max Kanat-Alexander]

(In reply to comment #58)
> My intuition from PRACA is that objects have more overhead, especially because
> match() doesn't use a request cache. This function can be called recursively,
> so I'd like to optimize for performance and memory use.

  Hmm. I think that's a premature optimization. And after all, bug_link is creating a Bug object for each bug anyhow--it's faster to do it here.

> Also, I am concerned
> about code parallelism with many-to-many relationships which will not be able
> to use match().

  Well, they can use new_from_id, or we can modify Bugzilla::Bug::match to do a join in some cases.

> Yeah, the inheritance is pretty opaque. How would you suggest structuring this
> function so that it accepts any $reverse_desc in Bugzilla::Object context but
> verifies by type in Bugzilla::Field context? This currently gives the correct
> behavior, but the code could be cleaned up quite a bit after Bug 539865 is
> taken care of.

  Why would you want it to accept any $reverse_desc during create()? There shouldn't be any validation difference.

> Moved styles to admin.css, but I had no luck getting them to look good without
> the cellpadding.

  Okay, it's fine to keep it then for now.

> I've tried a compromise between placing it directly in the format files and
> creating a new file. They style is maybe too PRACA-ish, but it has the right
> topology in multiple contexts. Let me know what you think.

  Hmmm. I don't want to depend on checking "bug.id" as a determination as to whether or not the "bug" argument is an object--I don't want to have to check that at all. The structure is actually pretty clever, but I think it's probably fine to just have a separate template.

Comment 60

[Vitaly Fedrushkov]

https://landfill.bugzilla.org/relationships/showdependencytree.cgi?id=2&bugfield=cf_hen
https://landfill.bugzilla.org/relationships/showdependencytree.cgi?id=5&bugfield=cf_hen

This would require more wording (right now heavily affected by 'depends on'/'blocks').

Then there will be l12y adventures, too :-)

Comment 61

[Max Kanat-Alexander]

(In reply to comment #60)
> This would require more wording (right now heavily affected by 'depends
> on'/'blocks').
> 
> Then there will be l12y adventures, too :-)

  That actually isn't part of the patch attached to this bug and hasn't really been reviewed by me (or by jjclark, as far as I know).

Comment 62

[Jesse Clark]

(In reply to comment #61)
>   That actually isn't part of the patch attached to this bug and hasn't really
> been reviewed by me (or by jjclark, as far as I know).

Yes, the wording on the tree page is pretty rough right now, maybe we could discuss it on bug 539131.

Comment 63

[Gervase Markham [:gerv]]

I have UI comments based on 
https://landfill.bugzilla.org/relationships/.

I don't seem to be getting my account creation email (is email disabled?) and it's hard to see quite what the issue is without seeing the admin interface but, in general, the UI of 
https://landfill.bugzilla.org/relationships/show_bug.cgi?id=1
looks overly-complex and not so pretty :-(

If a bug is in no relationships, the only UI should be that to add a relationship.

I suggest that UI should be:

This bug [ chicks   |v] bug(s) [               ]

If a bug is in a number of relationships, there should be one line per relationship, listing all the bugs, with discreet "edit" links which work like depends_on/blocks now, plus the UI for adding another one. If a bug is a member both ways in a directional relationship, perhaps the two could be grouped together. However, we need only one "show tree" link.

We want to use the generality of this feature to reduce, rather than increase, UI complexity and field count for sites which use it.

Gerv

Comment 64

[Jesse Clark]

Gerv:

I fixed the email on that test site and gave you admin access.

I agree that it would be nice to unify all the links and relationships in the frontend, including non-custom fields like blocks and depends, and future many-to-many links or external links. I have time to work on such a widget soon, but first I would like to agree on a backend implementation so that I can keep this in sync with the projects that are funding me to work on it.

Comment 65

[Max Kanat-Alexander]

Also, before we work on such a widget, we'd need some time of users actually using this feature and seeing if they routinely actually have more than one custom bug id relationship field. If they don't, then adding a lot of additional UI for it would probably not be necessary.

Comment 66

[Max Kanat-Alexander]

Comment on attachment 423897 [details] [diff] [review]
v21

  Hmm, actually, reading this again, I've changed my mind about extra_field_item--I like it; it allows us to have extra rows for other fields in the future, also.

  I do want related_bugs to return objects.

>Index: Bugzilla/Field.pm
> [snip]
>+sub _check_reverse_desc {
>+    # Bugzilla::Object->create() does not give us $type,

  That's actually not true--that's what run_create_validators is doing.

>Index: template/en/default/admin/custom_fields/create.html.tmpl
> [snip]
>+      <th class="narrow_label">
>+        <label for="reverse_desc">Reverse Relationship Description:</label>
>+      </th>
>+      <td>
>+        Use this label for the list of [% terms.bugs %] that link to a [% terms.bug %]
>+        with this [% field_types.${constants.FIELD_TYPE_BUG_ID} %] field.
>+        For example, if the description is "Is a duplicate of",
>+        the reverse description would be "Duplicates of this [% terms.bug %]".
>+        Leave blank to disable the list for this field.
>+        <br/>
>+        <input type="text" id="reverse_desc" name="reverse_desc" value="" size="40" disabled="disabled">

  I haven't looked at this in the UI yet, but I'd imagine that the input should be above the descriptive text, so that the input aligns with the label, and that the descriptive text should perhaps have some special styling.

Comment 67

[Max Kanat-Alexander]

  I do have one question--how in the world is extra_field_item actually *working*? We call field.html.tmpl using INCLUDE, not PROCESS.

Comment 68

[Gervase Markham [:gerv]]

(In reply to comment #65)
> Also, before we work on such a widget, we'd need some time of users actually
> using this feature and seeing if they routinely actually have more than one
> custom bug id relationship field. If they don't, then adding a lot of
> additional UI for it would probably not be necessary.

I would anticipate using the widget for the default relationships of "Blocks/Depends On" (an asymmetric relationship) and "See Also" (a symmetric one). So having such UI would actually reduce the UI complexity of the default bug page, not increase it.

I bring this up now because as you know, the way HTML works means that the URL parameters you use, and therefore the server-side interface to the code, depends on the widgets you've chosen. If we change the UI later, we may end up having to support URL parameters encoding two different ways of saying the same thing.

Gerv

Comment 69

[Jesse Clark]

That's a good point, I don't like the idea of adding special logic to process_bug.cgi to handle link input in multiple formats. However, this simple interface uses a POST format that requires no special handling, and a unified widget would probably want to use hidden fields to duplicate the same format if possible, but I'm not entirely sure if that would require javascript above and beyond what Bugzilla is comfortable with.

So we can either add extra processing logic, or fall back to this simple format. I don't think either of those would require a change to this backend code. I'm still looking into ways to make the naive interface look less cluttered.

Comment 70

[Max Kanat-Alexander]

@Jesse Yeah, I'm in support of the naieve interface for now. I've talked about this with Pyrzak before, I think, but one thing about the philosophy we're using with Bugzilla is that we like to stick with the standard metaphor of the web as much as possible, and invent our own widgets as little as possible.

Comment 71

[Jesse Clark]

Attachment: v22

(In reply to comment #59)
>   Why would you want it to accept any $reverse_desc during create()? There
> shouldn't be any validation difference.
> 

I like being able to store NULL for field types that can't have reverse descriptions. But that means I need to know the field type. It's available in Bugzilla::Field::run_create_validators, but not in Bugzilla::Object::run_create_validators, which gets called first. I don't want that first call to delete it from the params, causing it to be blank for the second call to the validator, which would then set the reverse description to blank when creating a new field.

This is exactly the behavior it needs, but I don't know how to make the code read more clearly without adding $params to Bugzilla::Object::run_create_validators and all the other changes that would entail.

> Hmmm. I don't want to depend on checking "bug.id" as a determination as to
> whether or not the "bug" argument is an object--I don't want to have to check
> that at all.

Bug::related_bugs is only meaningful if we have a bug id, but I guess it doesn't matter whether it's a valid object or not. The main concern is to prevent trying to call related_bugs on the create bug page. This could also be addressed with a more fleshed out extra_field_item interface.

(In reply to comment #67)
>   I do have one question--how in the world is extra_field_item actually
> *working*? We call field.html.tmpl using INCLUDE, not PROCESS.

Yeah, that's kind of confusing. It's consistent with my understanding of Template Toolkit's symbol table, which isn't documented very well. Do you think it would be more clear to pass a container along as input?

Comment 72

[Max Kanat-Alexander]

(In reply to comment #71)
> I like being able to store NULL for field types that can't have reverse
> descriptions. But that means I need to know the field type. It's available in
> Bugzilla::Field::run_create_validators, but not in
> Bugzilla::Object::run_create_validators, which gets called first.

  Oh, no, that's what UPDATE_VALIDATORS is for. Just put it in UPDATE_VALIDATORS.

> Bug::related_bugs is only meaningful if we have a bug id, but I guess it
> doesn't matter whether it's a valid object or not. The main concern is to
> prevent trying to call related_bugs on the create bug page. This could also be
> addressed with a more fleshed out extra_field_item interface.

  Yeah, I'm OK with bug.id after thinking about it. Checking whether or not an object has an id seems reasonable.

> Yeah, that's kind of confusing. It's consistent with my understanding of
> Template Toolkit's symbol table, which isn't documented very well. Do you think
> it would be more clear to pass a container along as input?

  It might be, I don't know. If it's working, right now, let's not worry about it.

Comment 73

[Jesse Clark]

Attachment: v23

Ahh, now it makes sense. Moved _check_reverse_desc to UPDATE_VALIDATORS.

Comment 74

[Max Kanat-Alexander]

Comment on attachment 425390 [details] [diff] [review]
v23

Looks great! Two things to fix on checkin:

1. There's a tab in editfields.cgi, it looks like.

2. "FILTER html_light" in edit.html.tmpl is incorrect, because that will double-filter the header and unless HTML::Scrubber is installed, will kill all the links in the data section.

Comment 75

[Max Kanat-Alexander]

On checkin, I also modified editfields.cgi to not check FIELD_TYPE_BUG_ID on doing the set_ calls, because the set call will work just fine no matter what type the field is.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified .bzrignore
modified editfields.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Field.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified skins/standard/admin.css
modified template/en/default/admin/custom_fields/cf-js.js.tmpl
modified template/en/default/admin/custom_fields/create.html.tmpl
modified template/en/default/admin/custom_fields/edit.html.tmpl                
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 6969.

Comment 76

[Max Kanat-Alexander]

There were a few filter test failures that I fixed after checkin:

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified template/en/default/filterexceptions.pl
modified template/en/default/admin/custom_fields/create.html.tmpl
modified template/en/default/admin/custom_fields/edit.html.tmpl
Committed revision 6970.

Comment 77

[Max Kanat-Alexander]

Added to the release notes in bug 604256.

Comment 78

[Frédéric Buclin]

Attachment: Selenium script

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
modified t/test_custom_fields.t
Committed revision 217.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_custom_fields.t
Committed revision 204.

Comment 79

[Frédéric Buclin]

Documentation added to bug 707170.