482584 - Add a parameter to hide the "See Also" field

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P1)

Description

[Frédéric Buclin]

I guess some installations won't need this field as they don't depend on an external Bugzilla installation, making this field useless.

Comment 1

[Max Kanat-Alexander]

I think we should add this to 3.4, actually. It should be really easy. It should only hide the UI, which exists only on show_bug.cgi.

Eventually we can replace this parameter with just setting the field obsolete, when we can control normal fields in editfields.cgi.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> It should be really easy. It
> should only hide the UI, which exists only on show_bug.cgi.

It should also make the validator reject to change this field if turned off.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

For consistency with other use* fields, use_see_also is off by default.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 367819 [details] [diff] [review]
patch, v1

>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v
>retrieving revision 1.276
>diff -3 -p -u -r1.276 Bug.pm
>--- Bugzilla/Bug.pm	26 Jan 2009 21:59:55 -0000	1.276
>+++ Bugzilla/Bug.pm	17 Mar 2009 18:23:28 -0000
>@@ -2352,6 +2352,7 @@ sub remove_group {
> sub add_see_also {
>     my ($self, $input) = @_;
>     $input = trim($input);
>+    return unless Bugzilla->params->{'use_see_also'};

  No, I want this parameter to only affect the UI. If people use the webservices to add a see_also URL, it should still be accepted.

> sub remove_see_also {
>     my ($self, $url) = @_;
>+    return unless Bugzilla->params->{'use_see_also'};

  Same comment here.

> sub see_also {
>     my ($self) = @_;
>-    return [] if $self->{'error'};
>+    return [] if ($self->{'error'} || !Bugzilla->params->{'use_see_also'});

  And here.

>+   name => 'use_see_also',
>+   type => 'b',
>+   default => 0

  I want it to default to on for this field. If in the future we find that a majority of installations don't use it, we can switch the default to off.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
>   No, I want this parameter to only affect the UI. If people use the
> webservices to add a see_also URL, it should still be accepted.

I don't see why. Using the web UI, you cannot edit unused fields. There is no reason to make an exception when using webservices. Also, I disagree to make it a UI only parameter. Other fields which are controlled by a use* parameter also ignore changes made when the parameter is off (e.g. the QA contact).

> >-    return [] if $self->{'error'};
> >+    return [] if ($self->{'error'} || !Bugzilla->params->{'use_see_also'});
> 
>   And here.

No, other fields also return nothing when the field is not in use. There is no valid reason to make an exception here.


>   I want it to default to on for this field.

Same reason here: there is no valid reason to make an exception.

Comment 6

[Max Kanat-Alexander]

Please make it a UI-only parameter. The reason for this is two-fold:

1) Launchpad will expect all Bugzillas to correctly respond to Bug.update_see_also.
2) Future versions of Bugzilla will expect all Bugzillas to correctly respond to Bug.update_see_also

In order for inter-Bugzilla interaction to work, the field itself must *work* in all Bugzillas. It doesn't have to clutter up the UI if people don't want to see it.

Also, in general, ALL field-on-or-off parameters should affect only the UI, and I have been working toward that, here and there, for quite some time. The only cases in which there should be backend code to deal with such parameters is when Bugzilla will actually behave inappropriately when the parameter is off.

This is also a fundamental software design issue. If we have parameters or preferences, they should affect the smallest area possible. You don't want the "model" to have special cases--such things should be happening only in the "controller" or the "view". (I assume that you're familiar with MVC, even though we've never talked about it.) $bug->some_field should always return me what's in the database unless there's some really good design-related reason for it to not to. Additionally, if I actually call $bug->set_some_field, it should change what's in the database unless there's some really good design-related reason for it to not. The place where we make the decision to call $bug->set_some_field is in the controller (in this case, process_bug.cgi). But if, say, a customization or a WebServices caller attempts to set the field, it should still get set. Bug.create already works this way (and if it doesn't, it should) and Bug.update will also work this way.

I want the feature to default to on for discoverability, for now. People can turn it off if they don't want to see the field in their UI. Perhaps eventually it will become "general knowledge" that Bugzilla has this field (thanks to it defaulting to on) and then we can look at whether or not most installations use it, and decide to default it to off then.

Comment 7

[Frédéric Buclin]

What you are asking is to make the parameter behave totally differently from other parameters. *All* other parameters also control what you can get by $bug->foo and edit by $bug->set_foo. I'm not convinced by what you say here.

Comment 8

[Max Kanat-Alexander]

Okay. Do you have responses to my statements that would give logical reasons (based in good software design or utility to our end users) to make it behave the way you want?

Comment 9

[Frédéric Buclin]

use_foo is not only to control the UI. It also controls if you want to use the field at all. Being able to alter delta_ts of bugs, generate bugmail for unused (invisible) fields and generate midair collisions with something which isn't in use is bad design IMO. Webservices must understand that. I don't know how other softwares work; I'm only talking about how Bugzilla works.

Comment 10

[Max Kanat-Alexander]

Okay, I can understand that generating midair collisions and bugmail for a field that is invisible in the UI might be confusing.

But what would you have, say, Launchpad do when it tries to link a bug, if use_see_also is off (as it will be by default)? What would you have future versions of Bugzilla do (which will auto-update the other Bugzilla) when one Bugzilla needs to refer to a bug in another Bugzilla, but use_see_also is off in that Bugzilla? (Particularly consider the case where use_see_also is fist off and then later on in the remote Bugzilla.) Because this is essentially an interoperability feature, I'd really like it to behave consistently in terms of API, even if it is invisible in the UI.

I think that the number of midair collisions generated by the field when it is invisible will be pretty minor.

The emails might be confusing, yes...

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> The emails might be confusing, yes...

  ...but that might be something that I'm willing to accept in exchange for consistency across Bugzillas in terms of what the API does.

Comment 12

[Frédéric Buclin]

(In reply to comment #10)
> But what would you have, say, Launchpad do when it tries to link a bug, if
> use_see_also is off (as it will be by default)?

That's not a problem. Launchpad would still point to and interact with the target installation, but the "dependency" would not be reflected on the target installation. Else if you have something like 20 other Bugzilla installations (not including test installations) which point to some b.m.o bug, you would have a list of 20 URLs in some b.m.o bug pointing to installations you never heard about and about which you absolutely don't care. I don't see why the "dependency" should be bi-directional. With the param turned off, this means "you are free to point to me and do any read-only action on me, but I'm not going to add your changes into my DB".

With what I said above, I wouldn't want to see an "unknown" external installation being able to add itself to our local See Also field without any control. For the interaction to be bi-directional, the other installation should be on a whitelist, maintained locally. When the use_see_also parameter is turned on, all installations being in the whitelist would be requested to report the list of bugs pointing to local bugs so that local dependencies can be synchronized. Synchronization is probably a feature which should be implemented anyway as an external installation may try to add a dependency to your local installation and your local installation is down for e.g. maintenance or the network is busy or down.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> That's not a problem. Launchpad would still point to and interact with the
> target installation, but the "dependency" would not be reflected on the target
> installation.

  Hmm. I think this would get confusing, as Launchpad also synchronizes comments between itself and Bugzilla bugs. Bugzilla users wouldn't be able to see where the bug where the comments were actually coming from

> With what I said above, I wouldn't want to see an "unknown" external
> installation being able to add itself to our local See Also field without any
> control. [snip]

  There are detailed plans written already in a bug as to how we will deal with that, don't worry.

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
>   Hmm. I think this would get confusing, as Launchpad also synchronizes
> comments between itself and Bugzilla bugs. Bugzilla users wouldn't be able to
> see where the bug where the comments were actually coming from

Launchpad writes its own comments into Bugzilla? Or the other way? Or both? If Launchpad writes into Bugzilla, then you want use_see_also to be turned on.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> Launchpad writes its own comments into Bugzilla? Or the other way? Or both? If
> Launchpad writes into Bugzilla, then you want use_see_also to be turned on.

  It does both. Other systems may do something similar in the future, too.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
>   It does both. Other systems may do something similar in the future, too.

My implementation doesn't prevent comments to be in sync, and in this case the param should be turned on for clarity (so that people understand where comments are coming from). I still think my implementation is the right way to go, for all the reasons above.

Comment 17

[Frédéric Buclin]

First some background discussion, for the record:

(23:55:09) wicked: iow, what happens when a webservice client wants to update a disabled QA contact or some custom field that doesn't exist?
(23:55:18) wicked: all field updated need to be consistent, IMO
(23:55:20) mkanat: wicked: If they set a field that doesn't exist, that's an error.
(23:55:39) mkanat: wicked: If they set a field that exists but is disabled by a parameter, who cares? Let them set it.
(23:56:02) LpSolit: bugmail cares, midair collisions care
(23:56:15) LpSolit: delta_ts cares
(23:56:26) wicked: heh, one would get a bugmail that says there was change but doesn't list anything as changed :D
(23:56:31) wicked: so confusing..
(23:56:34) LpSolit: exactly
(23:57:16) LpSolit: and changes wouldn't be recorded correctly in the bugs_activity table
(23:57:31) mkanat: LpSolit: No, they would be recorded correctly.
(23:58:16) mkanat: wicked: One thing I'm trying to keep consistent is what happens in an installation that toggles the parameter over time.


Now mkanat's suggestion, with which I'm only half satisfied:

(23:58:46) mkanat: LpSolit: There is another option.
(23:58:54) mkanat: LpSolit: We could *always* show the field on the bug if it has a value.
(23:59:02) mkanat: LpSolit: And just hide the add box.
(23:59:32) LpSolit: mkanat: specific to see_also? or for QA and milestones too????
(23:59:46) mkanat: LpSolit: Well, See Also is unique in that it is much more likely to not have a value.
(00:00:13) mkanat: LpSolit: Milestones are incapable of being blank, so it wouldn't make any sense for them.
(00:00:16) LpSolit: this behavior is inconsistent with other fields
(00:00:36) mkanat: LpSolit: So?
(00:02:23) LpSolit: mkanat: this would be an intermediate solution between our both positions
(00:02:30) mkanat: LpSolit: Yeah.
(00:02:46) mkanat: LpSolit: And it would hide the "ugly UI box" for people who don't want to see it.
(00:04:42) LpSolit: mkanat: the problem with this solution is: what happens if you hack the URL? would the see also field be updated?
(00:04:52) LpSolit: because technically, the param wouldn't prevent this
(00:04:52) mkanat: LpSolit: Yeah, it would be.
(00:05:00) mkanat: LpSolit: I don't think too many people hack the URL.
(00:05:14) mkanat: LpSolit: And the UI to remove the added URL will still appear.

Comment 18

[Max Kanat-Alexander]

Attachment: v2

Okay, here we go. I'll take review from whoever gets to this first.

Comment 19

[David Lawrence [:dkl]]

I can review this tomorrow (Friday).

Dave

Comment 20

[Frédéric Buclin]

Comment on attachment 373228 [details] [diff] [review]
v2

>Index: template/en/default/bug/edit.html.tmpl

>+      [% INCLUDE bug/field.html.tmpl 

>+           only_if_set = !Param('use_see_also')



>Index: template/en/default/bug/field.html.tmpl

>+  #   only_if_set: boolean; If true, the field will only be shown if it
>+  #                has a value.

>+       [% IF editable && !only_if_set %]


You don't need only_if_set at all. Simply write:

 [% IF editable && Param("use_see_also") %]


r=LpSolit with this change.

Comment 21

[Frédéric Buclin]

a=LpSolit assuming you remove only_if_set on checkin.

Comment 22

[David Lawrence [:dkl]]

Comment on attachment 373228 [details] [diff] [review]
v2

I agree with LpSolit about the only_if_set not being necessary. Also one nit-pick is why the param is not called 'useseealso' similar to all of the other use* params. But not a show-stopper for me. Otherwise tested and works as advertised.

r=dk

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> nit-pick is why the param is not called 'useseealso' similar to all of the

That's intentional. this_is_much_more_readable thanappendingeverything. All new parameter names should use underscores to separate words.

Comment 24

[Max Kanat-Alexander]

The reason I used only_if_set is because field.html.tmpl doesn't know that it's displaying the see_also field. I actually implemented it in such a way that it could be a generic type of custom field at some point. That is, I'd like customizers and possibly future Bugzilla developers to be able to use that field type in a more generic way.

Comment 25

[Frédéric Buclin]

(In reply to comment #24)
> The reason I used only_if_set is because field.html.tmpl doesn't know that it's
> displaying the see_also field.

But for now, that's the single one we have of that type, and the label explicitly says "Add bug URLs", which is a see_also field only to me.

Comment 26

[Max Kanat-Alexander]

Okay, I removed only_if_set. BTW, it's a BUG_URL field, so "Add Bug URLs" would always make sense.

tip:

Checking in Bugzilla/Config/BugFields.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/BugFields.pm,v  <--  BugFields.pm
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/admin/params/bugfields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/bugfields.html.tmpl,v  <--  bugfields.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.157; previous revision: 1.156
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/pages/fields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/fields.html.tmpl,v  <--  fields.html.tmpl
new revision: 1.16; previous revision: 1.15
done


3.4:

Checking in Bugzilla/Config/BugFields.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/BugFields.pm,v  <--  BugFields.pm
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in template/en/default/admin/params/bugfields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/bugfields.html.tmpl,v  <--  bugfields.html.tmpl
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.156.2.1; previous revision: 1.156
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.24.2.1; previous revision: 1.24
done
Checking in template/en/default/pages/fields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/fields.html.tmpl,v  <--  fields.html.tmpl
new revision: 1.15.2.1; previous revision: 1.15
done

Comment 27

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.