414236 - show_bug.cgi: Remove the knob in favor of normal <select> boxes

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Max Kanat-Alexander]

Instead of having the radio-button options for changing the status and resolution, we should just have normal <select> boxes for it, and eliminate the "knob" entirely.

Comment 1

[Guy Pyrzak]

Attachment: Patch v1

Behold! Fixes the knob for both single and multiple edit. Had to edit some of the dust skin so that it didn't look silly.

Comment 2

[Frédéric Buclin]

Comment on attachment 300273 [details] [diff] [review]
Patch v1

A few comments so far:

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

>+      Change to [% get_status(bug_status.name) FILTER html %]

Why does this field begin with "Change to" as we don't do it anywhere else? e.g. for the priority field we don't write "change to P2". Note that I'm fine this keeping "Change to", but we should have a good reason to do so.


>+    <span id="duplicate_settings"> of [% terms.bug %] #<input name="dup_id" size="6"></span>

I think "of bug" should be part of the <option> to avoid an extra string on the right of the <select> field.


>+    [% IF show_resolution %]
>+      <noscript><br></noscript>
>+      <span id="resolution_settings">resolved&nbsp;as&nbsp;[% PROCESS select_resolution %]</span>

Please remove "resolved as". This doesn't fit well when changing the bug resolution, and "Change to RESOLVED" [ FIXED v] is clear enough: we mean RESOLVED FIXED, so I don't think the extra string brings any useful information.


>+      [% get_status(bug.bug_status) FILTER html %]&nbsp;
>+                  [% get_resolution(bug.resolution) FILTER html %]

Are we sure that &nbsp; followed by a newline in the template won't be wrapped?


>+  <option value="duplicate">
>+    Mark as Duplicate
>+  </option>

As said above, I think it should be "Mark as Duplicate of [% terms.bug %] ..." to avoid having "of bug" outside the box and waste extra space. "..." at the end would make clear that something else will need to be selected. Same for "Change resolution to" which should probably be "Change resolution to ...".


This looks like a great UI cleanup!

Comment 3

[Frédéric Buclin]

It would be a shame to not have it for 3.2. I think that's the last major UI cleanup in show_bug.cgi.

Comment 4

[Guy Pyrzak]

(In reply to comment #2)
> (From update of attachment 300273 [details] [diff] [review])
> A few comments so far:
> 
> >Index: template/en/default/bug/knob.html.tmpl
> 
> >+      Change to [% get_status(bug_status.name) FILTER html %]
> 
> Why does this field begin with "Change to" as we don't do it anywhere else?
> e.g. for the priority field we don't write "change to P2". Note that I'm fine
> this keeping "Change to", but we should have a good reason to do so.
Mostly  because I didn't know if it was ok to delete it totally. But I'm happy to get rid of it.

> 
> >+    <span id="duplicate_settings"> of [% terms.bug %] #<input name="dup_id" size="6"></span>
> 
> I think "of bug" should be part of the <option> to avoid an extra string on the
> right of the <select> field.
> 
Sure thing
> 
> >+    [% IF show_resolution %]
> >+      <noscript><br></noscript>
> >+      <span id="resolution_settings">resolved&nbsp;as&nbsp;[% PROCESS select_resolution %]</span>
> 
> Please remove "resolved as". This doesn't fit well when changing the bug
> resolution, and "Change to RESOLVED" [ FIXED v] is clear enough: we mean
> RESOLVED FIXED, so I don't think the extra string brings any useful
> information.
Ok


> 
> 
> >+      [% get_status(bug.bug_status) FILTER html %]&nbsp;
> >+                  [% get_resolution(bug.resolution) FILTER html %]
> 
> Are we sure that &nbsp; followed by a newline in the template won't be wrapped?
>
I can double check, but the template block should chomp the whitespace between the two.
 
> 
> >+  <option value="duplicate">
> >+    Mark as Duplicate
> >+  </option>
> 
> As said above, I think it should be "Mark as Duplicate of [% terms.bug %] ..."
> to avoid having "of bug" outside the box and waste extra space. "..." at the
> end would make clear that something else will need to be selected. Same for
> "Change resolution to" which should probably be "Change resolution to ...".
> 
> 
> This looks like a great UI cleanup!
> 
thanks! The suggested changes were things i was unsure about. So thanks for the suggestions.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #2)
> I think "of bug" should be part of the <option> to avoid an extra string on the
> right of the <select> field.

  No, I'd like to keep it separate. As long as it only shows up when you select "DUPLICATE" for the Resolution, that's fine. (Maybe my statement doesn't make sense, though, since I haven't actually looked at the patch yet.)

> Are we sure that &nbsp; followed by a newline in the template won't be wrapped?

  Yes, TT will chomp the whitespace.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> > I think "of bug" should be part of the <option> to avoid an extra string on the
> > right of the <select> field.
> 
>   No, I'd like to keep it separate.

<option>Mark it as duplicate</option> of bug <input> is neither nice nor fit well on small screens (everything is moved to the right to insert this string).

Comment 7

[Guy Pyrzak]

Attachment: Knob Patch v1.5

the patch with Lp's suggestions.

Comment 8

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 9

[Max Kanat-Alexander]

Yes, this is both close and important, and so should go into 3.2.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 300472 [details] [diff] [review]
Knob Patch v1.5

When I go from RESOLVED DUPLICATE to VERIFIED, the resolution box defaults to FIXED. I'm pretty sure that when I go from one closed state to another, the resolution box shouldn't even appear.

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

>+          [% IF bug.resolution == "DUPLICATE" %]
>+            of [% terms.bug %] [%+ "${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]

  Since we're here, I'd really like the word "bug" to be a part of this link.

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

>+        [% select_resolution_for_status = BLOCK %][% select_resolution_for_status %]"[% bug_status.name FILTER js %]", [% END %]

  Hrm. Would you explain this whole crazy recursive block thing here? (That is, what it's used for and why you did it this way.)

>+    [% IF bug.dup_id %]
>+      <span id="duplicate_display">of [% terms.bug %] [%+ "${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]</span>

  Wait, this code is in two places? What's up with that?

>+<script type="text/javascript">
>+  var close_status_array = new Array([% select_resolution_for_status | replace(', $', '') %]);

  Do FILTER instead of | there. Also, why that replace?

>Index: template/en/default/list/edit-multiple.html.tmpl

>+[% BLOCK status_section %]

  You could possibly do this easier by getting the intersection of the statuses every bug can change to.

Comment 11

[Max Kanat-Alexander]

Attachment: Perl Changes for "No Knob"

These are the changes that would have to be made to process_bug.cgi and Bugzilla::Bug if there wasn't a "knob" select, just bug_status, resolution, and dup_id fields.

Comment 12

[Guy Pyrzak]

Attachment: Patch v2

Comment 13

[Frédéric Buclin]

Comment on attachment 304126 [details] [diff] [review]
Patch v2

Doesn't work at all. The UI is completely broken (screenshot will come in a few minutes) and trying to mark a bug as a dupe throws the error:

There is no status named 'duplicate'.

Comment 14

[Frédéric Buclin]

Attachment: screenshot of the broken UI

Comment 15

[Guy Pyrzak]

that's what happens when you do a cvs merge and don't pay attention. ** smacks self**

Comment 16

[Guy Pyrzak]

Attachment: Patch v2.1

I didn't check that the merge that CVS did was right, turns out i missed a paren. Other than that it's exactly the same.

Comment 17

[Frédéric Buclin]

Comment on attachment 304310 [details] [diff] [review]
Patch v2.1

>Index: template/en/default/global/header.html.tmpl

>+    [% Hook.process("additional_header") %]

Unrelated to this bug and has already been committed as part of bug 251740.

Comment 18

[Frédéric Buclin]

Comment on attachment 304310 [details] [diff] [review]
Patch v2.1

With this patch applied, the status and resolution fields are no longer populated, at least when I'm viewing a bug as a powerless user. I didn't check as a power user.

Comment 19

[Guy Pyrzak]

Attachment: Patch V3

Comment 20

[Guy Pyrzak]

Comment on attachment 305321 [details] [diff] [review]
Patch V3

LpSolit, please go over this patch. I know it is broken for moving, but moving is broken in the current build. If it works for everything but moving I'll be glad to fix it once the blocker is taken care of.

Comment 21

[Frédéric Buclin]

pyrzak, could you make sure your patch still works with my patch from bug 419243?

Comment 22

[Guy Pyrzak]

Attachment: ss of duplicates

Comment 23

[Guy Pyrzak]

Attachment: Patch v4

Comment 24

[Guy Pyrzak]

Attachment: Patch v5

Comment 25

[Guy Pyrzak]

Attachment: Patch v6

wasn't handling moved bugs correctly

Comment 26

[Frédéric Buclin]

Tip of the day:

Guy, do you know you can append several reviewers for the same flag? Bugzilla will automatically create one flag for each. So review?mkanat,lpsolit will create review?mkanat, review?lpsolit.

Comment 27

[Frédéric Buclin]

One problem found so far: view an open bug and click the "mark as duplicate" link. The resolution and bug id fields appear as expected. Then realize it's not a dupe and you click back to the original open status. Only the resolution field disappears, but the bug id field is still displayed and the "mark as duplicate" link is not restored.

Comment 28

[Frédéric Buclin]

Comment on attachment 305933 [details] [diff] [review]
Patch v6

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

>+      [% IF (bug.isopened || bug.dup_id ) && !bug_status.is_open  %]
>+        [% show_resolution = 1 %]
>+        [% select_resolution_for_status = BLOCK %]
>+          [% select_resolution_for_status FILTER none %]"
>+          [% bug_status.name FILTER js %]", [% END %]
>+      [% END %]

The indentation of the first [% END %] is incorrect. This makes it hard to read (I had to read it twice to understand it). Moreover, I don't understand what a BLOCK is doing here. BLOCKs should be at the end of the template, to not mess the code.

Comment 29

[Frédéric Buclin]

Comment on attachment 305933 [details] [diff] [review]
Patch v6

Please fix my previous comments. I will have a deeper look when the UI is fixed.

Comment 30

[Guy Pyrzak]

Attachment: Patch v7

fixes made. Let me know what you think.

Comment 31

[Guy Pyrzak]

Attachment: Patch v8

That last one was totally buggy. I missed a few things. This one should be better.

Comment 32

[Frédéric Buclin]

One irritating bug I found in the patch v8 is that when you first scroll down the page a bit, then click the "Mark as duplicate" link, it again jumps to the top of the page. If you just edited the page once, the "Email sent to" and "Excluding" list are displayed first, and if they are big enough, you have to scroll down again because the status/resolution fields are out of view. You should use the same trick we used when clicking the "Show/Hide Obsolete" link in the attachment table.

Comment 33

[Frédéric Buclin]

Comment on attachment 306667 [details] [diff] [review]
Patch v8

>Index: template/en/default/list/edit-multiple.html.tmpl

>+    <option value="[% dontchange FILTER html %]" selected="selected">Do nothing else</option>

"Do nothing else" is meaningless here. Replace it by "--do_not_change--", which is used in all other fields.


Moreover, editing several bugs at once fails if you select an open bug and leave the status field set to "Do not change". The error thrown is: "You cannot set a resolution for open bugs." That's because hidden fields are still passed to the CGI script.

Comment 34

[Frédéric Buclin]

IIRC, display:none doesn't pass the field to the CGI script while visibility:hidden does.

Comment 35

[Frédéric Buclin]

pyrzak, ping? Any progress?

Comment 36

[Alex Eiser]

Pyrzak is currently on vacation, and will be checking email infrequently.

Comment 37

[Guy Pyrzak]

(In reply to comment #32)
> One irritating bug I found in the patch v8 is that when you first scroll down
> the page a bit, then click the "Mark as duplicate" link, it again jumps to the
> top of the page. If you just edited the page once, the "Email sent to" and
> "Excluding" list are displayed first, and if they are big enough, you have to
> scroll down again because the status/resolution fields are out of view. You
> should use the same trick we used when clicking the "Show/Hide Obsolete" link
> in the attachment table.
This is an easy fix.

Comment 38

[Guy Pyrzak]

Attachment: Patch v9

fix for the 2 bugs mentioned.

Comment 39

[Guy Pyrzak]

Attachment: Patch V10

made the fix with commenting out 2 lines, deleted the lines instead, less sloppy. Sorry about that.

Comment 40

[Frédéric Buclin]

Comment on attachment 313151 [details] [diff] [review]
Patch V10

I have a bug in state RESOLVED FIXED. When I change the status to VERIFIED, the resolution field disappears with no reason. If I then click the "Mark as duplicate" link, the duplicate field doesn't appear, and the resolution doesn't change to DUPLICATE as the resolution field is still missing.

I didn't test further.

Comment 41

[Guy Pyrzak]

Attachment: Patch V11

wow, that was an easy fix. not sure why that logic was in there honestly.

Comment 42

[Frédéric Buclin]

Comment on attachment 313768 [details] [diff] [review]
Patch V11

>Index: process_bug.cgi

>+        $b->set_status(
>+            scalar $cgi->param('bug_status'),
>+            {resolution =>  $cgi->param('resolution'),
>+                dupe_of => $cgi->param('dup_id')}
>+            );

You have to add "scalar" before each $cgi->param(), else you get unexpected results.


>+    elsif (should_set('resolution')) {
>+       $b->set_resolution(scalar $cgi->param('resolution'), 
>+                          scalar $cgi->param('dup_id'));
>+    }

Arguments passed to set_resolution() are incorrect. The method takes the resolution as the first argument as a hash as the second argument. So it must be {dupe_of => scalar $cgi->param('dup_id')}.



>Index: Bugzilla/Bug.pm

> sub settable_resolutions {

This subroutine can now go away, and you can replace it by get_legal_field_values('resolution') where it's called, i.e. in buglist.cgi and in Bug.pm.



>Index: js/field.js

I will let Max review the JS code.



>Index: skins/standard/show_bug.css

>-#bz_field_status {
>+#duplicate_settings, #votes_container{
>     white-space: nowrap;
>+    
> }

The reason you remove #bz_field_status despite this id is still in use is because you want the resolution to wrap if the screen is too narrow, isn't it?



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

>+<div id="status">
>+  <div id="status-options">

I don't see the usefulness of having two consecutive <div>. One of them should die as you don't have:

<div>
  <div>
  </div>
  <div>
  </div>
</div>

But:

<div>
  <div>
  </div>
</div>


>-    [% NEXT IF bug.isunconfirmed && bug_status.is_open && !bug.user.canconfirm %]
>-    [% NEXT IF bug.isopened && !bug.isunconfirmed && bug_status.is_open && !bug.user.canedit %]
>-    [% NEXT IF (!bug_status.is_open || !bug.isopened) && !bug.user.canedit && !bug.user.isreporter %]
>+    [% NEXT IF bug.isunconfirmed
>+                && bug_status.is_open
>+                && !bug.user.canconfirm %]
>+    [% NEXT IF bug.isopened
>+                && !bug.isunconfirmed
>+                && bug_status.is_open
>+                && !bug.user.canedit %]
>+    [% NEXT IF (!bug_status.is_open || !bug.isopened)
>+                && !bug.user.canedit
>+                && !bug.user.isreporter %]
>     [%# Special hack to only display UNCO or REOP when reopening, but not both;
>       # for compatibility with older versions. %]
>-    [% NEXT IF !bug.isopened && (bug.everconfirmed && bug_status.name == "UNCONFIRMED"
>-                                 || !bug.everconfirmed && bug_status.name == "REOPENED") %]
>+    [% NEXT IF !bug.isopened
>+                && (bug.everconfirmed && bug_status.name == "UNCONFIRMED"
>+                    || !bug.everconfirmed && bug_status.name == "REOPENED") %]

Please revert all these changes. Not only they are not useful, but they break Bonsai in the sense that it's harder to detect which bug implemented them. They are fine as is.


>+      <option
>            value="[% bug_status.name FILTER html %]">

Write this on a single line. Also, fix the indentation from this line till the next [% END %]. It should be shifted to the left.


>+        [% show_resolution = 1 %]
>+        [% resolution_list = PROCESS append_resolution_list
>+                                          status => bug_status.name  %]

I don't like it. You should rather push() the data to an array rather than building a string with all bug statuses concatenated. Also, I don't understand why you pass a bug *status* to a list of *resolutions*?! Either the name of the variable is wrong, or you pass the incorrect data.


>       [% IF bug.resolution != "MOVED" || bug.user.canmove %]
>         [% PROCESS initial_action %]
>+        [% show_resolution = 1 %]
>       [% END %]

How do you populate the list of valid resolutions from here? I'm not quite sure to understand the job of resolution_list.


>+          <span id="duplicate_display">of [% terms.bug %]
>+          #[%+ "${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]</span>

Remove "#", this doesn't look nice (and we don't use it elsewhere). Maybe could we include "bug" in the link? This is useful when the bug ID has a very few digits, it's easier to click on it. Also, fix the indentation of this block and remove "${ }" around bug.dup_id, this is useless.


>+  [% IF bug.user.canedit || bug.user.isreporter %]  

There is no need to start another block again. You can put this in [% IF bug_status_select_displayed %] because this one is set to 1 in all cases if the user can edit the bug or is the reporter.


>+    <span id="duplicate_settings">of [% terms.bug %]
>+      #<span id="dup_id_container" class="bz_default_hidden">

Here too, remove "#".


>+  var close_status_array = new Array([% resolution_list FILTER replace(',$', '')
>+                                                                FILTER none %]);

With resolution_list being an array, you could now join() the list, which is nicer.

I will let Max review this JS code.


>+    <option selected value="[% bug.bug_status FILTER html %]" >

Nit: remove the whitespace before ">".


>+    [% IF ! bug.isopened  %] 

Nit: remove the whitespace after "!". Also, please remove trailing whitespaces you have here and there at the end of lines.


> [% BLOCK duplicate %]
>+  <option value="DUPLICATE">
>+    DUPLICATE
>+  </option>
> [% END %]

Defining a block for such a minor thing is rather useless. Please write the code directly at its right place.


>+[% BLOCK append_resolution_list %]
>+  [% resolution_list FILTER none %]"[% status FILTER js %]",
>+[% END %]

This can go away with my suggestion above, i.e. using an array.


>\ No newline at end of file

Huh??



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

>   [% ELSIF error == "undefined_field" %]
>     Form field [% field FILTER html %] was not defined.
>     [%# Useful message if browser did not select show_bug radio button %]
>-    [% IF field == "knob" %]
>+    [% IF field == "bug_status" %]
>       Check that the "Leave as..." radio button was selected.
>     [% END %]

undefined_field is never called with field = "knob/bug_status" as the "Leave as..." radio button no longer exists. This part of the error message should go way.



>Index: template/en/default/list/edit-multiple.html.tmpl

> [% IF display_warning %]
>+  <p class="box" id="resolution_settings_warning">
>     (*) Note that the resolution will only be applied to open [% terms.bugs %].
>     Already closed [% terms.bugs %] will keep their resolution unchanged.
>   </p>
> [% END %]

This warning is no longer relevant and is now even incorrect when you have a mix of open and closed bugs in your buglist. This must go away or only be displayed when appropriate.


> <input type="submit" id="commit" value="Commit">

Some vertical space is needed between the groups table and this button.


>+  <select name="bug_status" id="bug_status"> 
>+    <option value="[% dontchange FILTER html %]" selected="selected">[% dontchange FILTER html %]</option>
>+  
>+  [% FOREACH bug_status = new_bug_statuses %]
>+    <option value="[% bug_status.name FILTER html %]">

Please fix the indentation of [% FOREACH %] and the subsequent code. Else it's a pain to read it correctly.


>+    [%# Closed bugs cannot have their resolution changed this way. %]
>+    [% IF !bug_status.is_open && !all_closed_bugs %]
>+      [%# PROCESS select_resolution id = bug_status.id %]

Huh?? The comment is now wrong. There is now no other way to change the resolution of bugs, AFAIK. And [% BLOCK select_resolution %] no longer exists, so you should delete these lines rather than commenting them.


>+      [% select_resolution_for_status = BLOCK %][% select_resolution_for_status FILTER none %]"[% bug_status.name FILTER js %]", [% END %]

As said above, use an array rather than this trick.


>+      [% display_warning = 1 UNLESS all_open_bugs %]

No longer relevant (or confusing), see above.


>+    [% select_resolution_for_status = BLOCK %][% select_resolution_for_status FILTER none %]"[% dontchange FILTER js%]", [% END %]

Huh? You cannot have twice the same name for a BLOCK.


>+  <span id="resolution_settings">
>+  <select id="resolution" name="resolution">

If you display the resolution field unconditionally, with bothering setting the show_resolution variable? It's defined but never used.


>+  <script type="text/javascript">
>+  var close_status_array = new Array([% select_resolution_for_status FILTER replace(', $', '') FILTER none %]);

You can now use join() instead. I will let Max review this JS code.



The good news is: I found nothing wrong while testing your patch. :)

Comment 43

[Guy Pyrzak]

Attachment: Patch V12

Made most of the suggested changes. Only one i didn't do refereed to a useless block than when I looked at it, it wasn't useless. Thanks for the suggestions. Max please look at the JS.

Comment 44

[Frédéric Buclin]

Comment on attachment 314270 [details] [diff] [review]
Patch V12

>Index: process_bug.cgi

>+       $b->set_resolution(scalar $cgi->param('resolution'), 
>+                          scalar $cgi->param('dup_id'));

Please re-read my comment about set_resolution(). This won't work.

Comment 45

[Guy Pyrzak]

Attachment: Patch V13

sorry missed that one perl fix. I think I got the rest.

Comment 46

[Max Kanat-Alexander]

Comment on attachment 314274 [details] [diff] [review]
Patch V13

>Index: js/field.js
>+        else if( close_status_array.indexOf(el.value) > -1 ){

  Just as a note, our usual style is not to have those spaces inside the "if" conditions unless necessary (for example, to disambiguate internal parentheses) and to have a space before the opening curly brace.

>+function showDuplicateItem(e){
> [snip]
>+        if(resolution.value == 'DUPLICATE' && close_status_array.indexOf(bug_status.value) > -1 ){

  Where does close_status_array come from?

>+    YAHOO.util.Event.preventDefault(e);

  Could you add a comment explaining what that does?

>+function duplicateDiscoverable(e, duplicate_or_move_bug_status){

  Discoverable? Perhaps there's some better name for that function?

>Index: skins/standard/buglist.css
>+#commit, #action {
>+  margin-top: 5px;
>+}

  I don't quite understand what a change to buglist.css is doing in this patch. In any case, any reason this is in px instead of in em (say, .1 em or .25em)?

>Index: template/en/default/bug/edit.html.tmpl
>       <td class="field_label">
>         <label for="component" accesskey="m">
>           <b><a href="describecomponents.cgi?product=[% bug.product FILTER url_quote %]">
>-            Co<u>m</u>ponent</a>
>+            Co<u>m</u>ponent</a>:

  Is this whole block relevant to this patch?

>@@ -450,7 +445,7 @@
>-            Target&nbsp;Milestone[% "</a>" IF bug.milestoneurl %]
>+            Target Milestone[% "</a>" IF bug.milestoneurl %]

  Same question here.

>Index: template/en/default/bug/knob.html.tmpl
>+    [% IF  !bug_status.is_open  %]

  Nit: I know I'm being picky, but those extra spaces bug me.

>+      [% filtered_status = FILTER js %][% bug_status.name %][% END %]

  Can't you also do [% SET filtered_status = bug_status.name | js %]?


  These are just my code comments, now I'm going to try the UI.

Comment 47

[Max Kanat-Alexander]

Comment on attachment 314274 [details] [diff] [review]
Patch V13

Works beautifully, both with and without JS.

All my above comments are just nits. (Oh, and I found where close_status_array comes from, you don't have to answer that.)

Comment 48

[Max Kanat-Alexander]

Holding approval until LpSolit r+'s the last patch.

Comment 49

[Guy Pyrzak]

Attachment: Patch V14

Did most of Mkanat's nits as well as added the curly braces that I forgot.

Comment 50

[Max Kanat-Alexander]

Comment on attachment 314352 [details] [diff] [review]
Patch V14

Yeah, looks fine to me. there's still a few places where you don't have a space before your curly brace (and also at least one place where you don't have a space between "if" and the opening paren, which is also our standard style).

Comment 51

[Frédéric Buclin]

Comment on attachment 314352 [details] [diff] [review]
Patch V14

>Index: process_bug.cgi

>+        $b->set_status(
>+            scalar $cgi->param('bug_status'),
>+            {resolution =>  scalar $cgi->param('resolution'),
>+                {dupe_of => scalar $cgi->param('dup_id')}}
>+            );

This is wrong. Don't enclode dupe_of => bla here....


>+       $b->set_resolution(scalar $cgi->param('resolution'), 
>+                          dupe_of => scalar $cgi->param('dup_id'));

... but here.


Also, the patch is broken:

patching file buglist.cgi
patching file process_bug.cgi
patching file Bugzilla/Bug.pm
patching file js/field.js
patching file skins/contrib/Dusk/global.css
patching file skins/standard/buglist.css
patching file skins/standard/show_bug.css
patching file template/en/default/bug/edit.html.tmpl
patch: **** malformed patch at line 336: Index: template/en/default/bug/knob.html.tmpl

Comment 52

[Guy Pyrzak]

Attachment: Patch V15

ok... i think this time I'll get 2 r+s

Comment 53

[Frédéric Buclin]

Comment on attachment 314373 [details] [diff] [review]
Patch V15

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

>+      [% filtered_status = FILTER js %][% bug_status.name %][% END %]

Better is to write: [% filtered_status = bug_status.name FILTER js %]. This is nicer, is what we use elsewhere, and won't make runtests.pl to fail:

t/008filter..........NOK 31
#   Failed test '(en/default) template/en/default/bug/knob.html.tmpl has unfiltered directives:
#   51: bug_status.name
#   136: get_status(bug.bug_status)
# --ERROR'


>+      [% closed_status_array.push( '"' _ filtered_status _ '"' ) %]

You could/should add quotes later, in the JS script, rather than having to do it everytime you add a status to the array. [% closed_status_array.push(filtered_status) %] works just fine.


>     [% ELSE %]
>       [% IF bug.resolution != "MOVED" || bug.user.canmove %]

Nit: this could be combined in a single [% ELSIF ... %] to make this clearer.


>+          <span id="duplicate_display">of [% terms.bug %]
>+          [%+ "${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]</span>

Nit: we should include "bug" in the link, i.e.:
[%+ "$terms.bug ${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]

If you don't want "bug" in the link, write bug.dup_id instead of "${bug.dup_id}".


>+        [% bug.dup_id FILTER bug_link(bug.dup_id) FILTER none%]

Nit: add a whitespace after "none".


>+      [% filtered_status = FILTER js %][% get_status(bug.bug_status) %][% END %]
>+      [% closed_status_array.push('"' _ filtered_status _ '"') %]

Same two comments as above. Especially because this makes runtests.pl to fail (do not forget to run it!).



>Index: template/en/default/list/edit-multiple.html.tmpl

>+  [% display_warning = 0 %]

This variable is no longer in use. Bye bye.


>+      [% IF !bug_status.is_open && !all_closed_bugs %]
>+        [% filtered_status = FILTER js %][% bug_status.name %][% END %]
>+        [% closed_status_array.push( '"' _ filtered_status _ '"') %]
>+      [% END %]

Besides the usual two comments I made above and which makes runtests.pl to fail, this code doesn't work (or brings the user to confusion). If all bugs are resolved and you select e.g. VERIFIED, then the resolution select disappears forever. The only way to get it back in order to change the resolution is to set the status back to --do_not_change--. This means the UI here is more restrictive than the one when editing a single bug. Also, if you have at least one open bug in your buglist, then the UI lets you change both the bug status and the resolution at the same time, which is inconsistent and confusing. To summarize: "&& !all_closed_bugs" should go away.


>+  [%# If all the bugs being changed are closed, allow the user to change their resolution. %]
>+  [% IF all_closed_bugs %]
>+    [% filtered_status = FILTER js %][% dontchange %][% END %]
>+    [% closed_status_array.push( '"' _ filtered_status _ '"') %]

Same two comments, also about runtests.pl failing:

t/008filter..........NOK 94
#   Failed test '(en/default) template/en/default/list/edit-multiple.html.tmpl has unfiltered directives:
#   357: bug_status.name
#   364: dontchange
# --ERROR'

Moreover, this code doesn't work if you have a mix of open and closed bugs in your buglist. You no longer can change the resolution of bugs while leaving the bug status alone (when you select only closed bugs in your buglist). With the change above about all_closed_bugs, this block can go away.


>+    [% display_warning = 1 UNLESS all_open_bugs %]

This variable is no longer used.


>+      <option value="[% dontchange FILTER html %]" selected > [% dontchange FILTER html %]  </option>

Remove whitespaces around the second [% dontchange FILTER html %].


>+  [%+ "(*)" UNLESS all_open_bugs %]

This message is no longer relevant. (*) should go away.


>+  var close_status_array = new Array([% closed_status_array.join(', ') FILTER none %]);

If you removed quotes above when push()ing statuses to the array, you will have to add them from here. Maybe is it a good idea to do it here?


Before uploading a new patch, make sure to run runtests.pl first.

Comment 54

[Guy Pyrzak]

Attachment: Patch v16

Did a bunch of tests and made all the suggested changes and ran runtests.pl full pass. *Crosses Fingers*

Comment 55

[Frédéric Buclin]

Attachment: patch, v16.1

The v16 patch was malformed (missing empty line at the end of edit.html.tmpl) + there was a bug:

[% filtered_status = get_status(bug.bug_status) FILTER js %] must be
[% filtered_status = bug.bug_status FILTER js %]

This patch includes both fixes. r=LpSolit on this one.

Comment 56

[Frédéric Buclin]

Please check in the patch I attached, which contains the fixes I mentioned in my previous comment.

Comment 57

[Guy Pyrzak]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.373; previous revision: 1.372
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.410; previous revision: 1.409
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.240; previous revision: 1.239
done
Checking in js/field.js;
/cvsroot/mozilla/webtools/bugzilla/js/field.js,v  <--  field.js
new revision: 1.7; previous revision: 1.6
done
Checking in skins/contrib/Dusk/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/contrib/Dusk/global.css,v  <--  global.css
new revision: 1.3; previous revision: 1.2
done
Checking in skins/standard/buglist.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/buglist.css,v  <--  buglist.css
new revision: 1.12; previous revision: 1.11
done
Checking in skins/standard/show_bug.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/show_bug.css,v  <--  show_bug.css
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.118; previous revision: 1.117
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.36; previous revision: 1.35
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.105; previous revision: 1.104
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v  <--  edit-multiple.html.tmpl
new revision: 1.49; previous revision: 1.48
done