706629 - Allow the creation of a sighting via the sightings table

Status: ASSIGNED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Robert Webb]

With the addition of the sightings table, we need a way in the UI to add new sightings to a bug. A create new bug button should be added to the sightings table that will create a new sighting of the master bug.

Comment 1

[Robert Webb]

Been busy with other projects at work. I'm moving back onto getting this finished; should be able to post some diffs this week.

Comment 2

[Robert Webb]

Attachment: Create A Sighting From the Sightings Table diff v1.

This adds a button to the bottom of the sightings table that will allow you to create a new sighting on the bug. I've also included a hidden/toggle text area for you to add a different description to the sighting that's created.

Comment 3

[Robert Webb]

A note on this; I wasn't too sure what fields to exclude when duping the values from the master bug. From the design we have "When a user edits a master bug, the values are populated to all of the sightings. We synch the values for almost all fields except for CCs, Time Tracking, Attachments, Flags, Status, Resolution, Dupe of and Comments." But we don't state anything about values to synch when creating the sighting.

Comment 4

[Robert Webb]

Comment on attachment 606725 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v1.

Requesting review from LpSolit

Comment 5

[Frédéric Buclin]

Comment on attachment 606725 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v1.

>=== modified file 'process_bug.cgi'

>+if (defined $cgi->param('add_sighting')) {

What happens if I try to hack the URL and I define add_sighting while editing several bugs at once (mass-change)?


>+    foreach my $field (Bugzilla::Bug->fields) {
>+        $bug_params{$field} = $first_bug->{$field};
>+    }

How does this work when a user creates a sighting when the bug is restricted to some groups which the user is not a member of? Does your code also copy attachments (I hope not)?


>+    $bug_params{'master_bug_id'} = $cgi->param('master_bug');

Does the validator make sure you are allowed to access master_bug?


What bothers me with this code is that I cannot create a new sighting independently of what I'm currently doing in the bug I'm viewing. I think creating a new sighting shouldn't commit what you were doing in the bug, because "Create Sighting" != "Save Changes", and I didn't expect my changes to be committed already; I just wanted to create a new sighting.

Also, if a powerless user (without editbugs privs) tries to create a new sighting, he gets a weird error message about a missing product. If the powerless user shouldn't be allowed to create a new sighting, then the "Create Sighting" button shouldn't be displayed.



>=== modified file 'template/en/default/sighting/list.html.tmpl'

When there is no sighting, this table takes too much space. Most bugs won't have any sighting and we shouldn't waste so much space. To partially fix this problem, the "Create Sighting" problem should at least be on the same row as the "Create sighting with different description" link. Also, its location seems weird to me. I would expect to see it at the very top of the bug report, not above the timetracking fields, which makes the UI really not nice. Also, I think the product, component and version should at least be displayed in this table, else it's pretty useless.

Comment 6

[Robert Webb]

> What happens if I try to hack the URL and I define add_sighting while editing
> several bugs at once (mass-change)?

It'll create a single sighting on the first bug in the list. I can certainly restrict this by only allowing sightings to be added when you're only editing a single bug: if we get more than one display an error.

> How does this work when a user creates a sighting when the bug is restricted to
> some groups which the user is not a member of? Does your code also copy
> attachments (I hope not)?

Attachments are not copied; in the design we have them on a per-bug basis. I had thought I had thrown in a security check to make sure the user can edit the bug (they should only be able to add a sighting to a bug they cannot edit), but I apparently missed that check. Also, if you can think of a better way to clone the bug's values to a new bug, I'd gladly use it. :)

> Does the validator make sure you are allowed to access master_bug?

Yes, it does.

> What bothers me with this code is that I cannot create a new sighting 
> independently of what I'm currently doing in the bug I'm viewing. I think 
> creating a new sighting shouldn't commit what you were doing in the bug, 
> because "Create Sighting" != "Save Changes", and I didn't expect my changes to 
> be committed already; I just wanted to create a new sighting.

Absolutely, I agree. Originally I was thinking of having another form that I'd post to when you press the submit button, but unfortunately due to the restrictions in HTML you can't nest forms that post to different places. What I can do, however, is move this whole sightings ordeal up higher in the process_bug.cgi file to add the sighting and bail out of modifying the original bug. If you have other options I'd be glad to hear them.

> Also, if a powerless user (without editbugs privs) tries to create a new
> sighting, he gets a weird error message about a missing product. If the
> powerless user shouldn't be allowed to create a new sighting, then the "Create
> Sighting" button shouldn't be displayed.

I hadn't considered this. I'll modify the templates to not even render the buttons if you can't modify the bug.

> When there is no sighting, this table takes too much space. Most bugs won't
> have any sighting and we shouldn't waste so much space. To partially fix this
> problem, the "Create Sighting" problem should at least be on the same row as
> the "Create sighting with different description" link.

I can hide a lot of this table when there are no sightings available on the bug (so all you see is just that section with the "Create Sighting" button). I'll also move the description and sighting buttons onto the same line to save vertical space.

> Also, its location seems weird to me. I would expect to see it at the very top
> of the bug report, not above the timetracking fields, which makes the UI
> really not nice. Also, I think the product, component and version should at
> least be displayed in this table, else it's pretty useless.

Where in the bug report would you like to see the table? The original thought was just to keep it down with the rest of the tables (attachments, timetracking, etc).

As for having the product, component and version being displayed, that area off to the right is eventually going to hold all of the fields that the sighting is varied on. Having extra fields displayed there that the sighting is not varied on would be rather confusing. I know it looks weird now, but this is only really half of the stuff we planned on putting into the table. Definitely refer to the design proposal we have for this on bug 55970 as we have a full mock-up for how this will appear in the end. If you have any questions or concerns involving that document, I'd gladly hear them.

Comment 7

[Frédéric Buclin]

(In reply to Robert Webb from comment #6)
> Absolutely, I agree. Originally I was thinking of having another form that
> I'd post to when you press the submit button, but unfortunately due to the
> restrictions in HTML you can't nest forms that post to different places.

Couldn't you use JSON-RPC to create the sighting without leaving the current bug, and update the sighting table accordingly? I know this is much more work, but this would be the less surprising behavior.


> Where in the bug report would you like to see the table? The original
> thought was just to keep it down with the rest of the tables (attachments,
> timetracking, etc).

I was thinking to have it above the bug summary.


I will have to re-read comments in bug 55970. I will do that once I have some time. :)

Comment 8

[Robert Webb]

Certainly, we could use JSON-RPC, though I mainly was going with how the rest of Bugzilla works. Are there any other pages that use this to make changes? How are errors handled? It just strikes me that JSON-RPC would be a much less expected behavior than submitting a form. However, if this is the direction Bugzilla is taking to become more Javascript oriented, I certainly don't mind implementing it in that way.

So for the sightings table to be above the bug summary, do you mean at the top of the page basically?

I wouldn't bother reading through all those comments in bug 55970. The important thing is the Initial Sightings Design Doc. That has almost everything incorporated in it from the comments on the bug, just in a more distilled form.

Comment 9

[Robert Webb]

Byron or David, would either of you be available to follow up on this review? I've still got some outstanding questions that need answering, but I can provide a new patch as soon as they're covered.

Comment 10

[Robert Webb]

Attachment: Create A Sighting From the Sightings Table diff v2.

David, I just assigned the review to you but if you want to pass this onto Byron (or someone else who can do these), feel free. You may want to read up on the original proposal on bug 55970 as well.

I've made the following changes:
 - You cannot create a sighting when editing multiple bugs (hacking your POST params won't force one to be created, we check for it).
 - When a sighting is created it doesn't edit the bug you're currently viewing.
 - When a sighting is created it uses the current state of the bug to be created (no more pulling in data from the POST params).
 - Correctly set visibility on sightings table; if you can't see the bug you won't see it in the sightings table.
 - If there are no sightings in the table you will only see the "Create Sighting" button, unless you're not logged in (then you see nothing).

Comment 11

[:glob ✱]

Comment on attachment 8360742 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v2.

sorry, but i don't have the time to do this review right now.

Comment 12

[Robert Webb]

Chris is taking over these bugs from me. He should be available to make the requested changes and get the reviews updated.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 8360742 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v2.

Hi Robert,

I'm really sorry, but I don't really have time either :-( And I think reviewing this patch would impose an obligation to see the project through and review all the patches in the set, and I definitely don't have time for that. I can imagine this is quite frustrating for you; but Bugzilla is currently not replete with maintainers who have time on their hands :-|

Gerv

Comment 14

[Frédéric Buclin]

Comment on attachment 8360742 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v2.

Should stay in reviewers' radar.

Comment 15

[Frédéric Buclin]

Comment on attachment 8360742 [details] [diff] [review]
Create A Sighting From the Sightings Table diff v2.

I'm a bit surprised that mkanat asked you to split the sightings implementation into so small pieces... Anyway, here is my review.


>=== modified file 'process_bug.cgi'

>+# Add a new sighting to the bug.
>+if (defined $cgi->param('add_sighting')) {

This code should be in Bugzilla::Bug so that sightings can be created using the WebServices. It should be in $bug->create_sighting().


>+    ThrowUserError('create_sighting_on_multiple_not_allowed')
>+        if (scalar(@bug_objects) > 1);

If we are editing several bugs at once, simply ignore sightings. process_bug.cgi already has several |if ($cgi->param('id'))| to check that.


>+    # Create a new bug with all of the same settings as this bug.
>+    my %bug_params;
>+    foreach my $field (Bugzilla::Bug->fields) {
>+        $bug_params{$field} = $first_bug->{$field};
>+    }

Why not using $first_bug->$field instead?


>+    # Convert some IDs to objects.
>+    $bug_params{'assigned_to'} = $first_bug->assigned_to;
>+    $bug_params{'product'} = $first_bug->product;
>+    $bug_params{'component'} = $first_bug->component;

If you use ->$field instead of ->{$field}, this code would be useless.


>+    my @strip_fields = ('actual_time', 'classification', 'classification_id', 'dup_id',
>+                        'resolution', 'see_also', 'bug_id');

You must also exclude the bug alias. Also, do we really want to copy the CC list and the dependency trees? This could spam a lot of people. What about the assignee? I don't have the original proposal in mind, so maybe these questions have already been answered elsewhere.

You should also list bug_status and reporter to make clear that we are going to override them later. I had to read the code carefully to understand what was going on here with these fields.

This makes me think: shouldn't the code be refactored a bit as it seems to share quite a lot of code with post_bug.cgi, especially the list of fields to clone?


>+    foreach my $field (@strip_fields) {
>+        delete $bug_params{$field} if exists $bug_params{$field};
>+    }

No need to check for the existence of the key. Simply delete it.


>+    $bug_params{'master_bug_id'} = $cgi->param('master_bug');

You should get master_bug_id from $first_bug->master_bug_id to avoid user trying to abuse sightings.


>+    my $product = $user->can_enter_product($bug_params{'product'}, THROW_ERROR);

If the user is not allowed to create sightings in the current product, then no button should be displayed to create one.


>+    $vars->{'id'} = $sighting->bug_id;

Nit: note that you can write $vars->{foo} instead of $vars->{'foo'} here and elsewhere.



>=== modified file 'skins/standard/global.css'

>+.bz_sighting_footer td {
>+    text-align: left ! important;
>+}

You shouldn't call !important. Else it's impossible for skins to override this. Also, with the move to HTML5, the master bug is badly aligned in the sighting table. This should also be fixed.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+  [% ELSIF error == "create_sighting_on_multiple_not_allowed" %]
>+    [% title = "Creating a Sighting on Multiple Bugs is not Allowed" %]
>+    You may not create a sighting on multiple [% terms.bugs %] at once.

This error is no longer needed, see above (and "Bugs" was hardcoded in the title).



>=== modified file 'template/en/default/sighting/list.html.tmpl'

>+[% IF sightings.size > 0 OR user.id %]

You should also check user.can_enter_product in case the user is not allowed to create a bug in the current product.


>+<input type="hidden" name="master_bug" value="[% master_bug.id FILTER html %]">

This info is (should be) available from ->master_bug_id.


>+    [% PROCESS render_sighting_row target = master_bug tag = "th" %]

I really dislike passing HTML elements this way. This makes the code hard to read and parse. IMO, HTML elements should be hardcoded. This will let you fix the align problem mentioned above.


>+      </span><br/>
>+      <input id="add_sighting" name="add_sighting" type="submit"
>+             value="Create Sighting"><br/>

Write <br> instead of <br/>. Bugzilla uses the HTML5 doctype, not XHTML.

Comment 16

[Chris Kamradt]

Attachment: Create a sighting from the sightings table

I've made the following changes from the last diff:

1. Added create_sighting to Bugzilla/Bug.pm
2. Hide create sighting button if user can't create one
3. Changed alignment of values within the sightings table
4. ignore creating sightings when editing multiple bugs instead of throwing error

I couldn't find any previous dialog about syncing the CC lists or dependency trees.  The original spec discusses how fields are synced on edit but not on creation.  Within our org we copy the CC field so that users can determine if they want to remain on the CC list or not.  This works better for us than requiring people to add themselves to the CC lists after the fact.

I looked at refactoring code from post_bug.cgi but there seemed to be very little overlap related to the fields.  post_bug builds up the fields required to create a bug while create_sighting has all of these fields ready and removes the ones that no longer apply.

Comment 17

[Frédéric Buclin]

Comment on attachment 8397430 [details] [diff] [review]
Create a sighting from the sightings table

Sorry, but I'm not involved in Bugzilla anymore.

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 8397430 [details] [diff] [review]
Create a sighting from the sightings table

Sorry for squatting on this for so long. I am just too busy with other responsibilities to do a proper review of this. Also the code is undoubtedly bit-rotted and will need to be updated. Plus the sightings table branch was never merged. So we may need to start from square one on this and do it as a single patch.

dkl