Assignee and QA contact fields always become revealed (editable)

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
User Interface
--
minor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Guy Pyrzak, Assigned: Guy Pyrzak)

Tracking

3.1.2
Bugzilla 3.2
Bug Flags:
approval +
blocking3.2 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

10.52 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
6.42 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
This is caused by the reassign to default checkbox value being 1 and that not matching the value that it is being compared to the wrong value.

This is also a problem because it is checking the wrong value in the JS. If the value being compared is a checkbox it should check if the box is checked or not.
Flags: blocking3.2?

Updated

10 years ago
Flags: blocking3.2? → blocking3.2+
Summary: Assignee and QA contact fields always become revealed → Assignee and QA contact fields always become revealed (editable)
(Assignee)

Comment 1

10 years ago
Another bug i found related to this (why i didn't find it in testing it the first time around). This won't happen if the QA field is disabled b/c it doesn't go through the whole function. So... I need to fix that as part of this patch.
(Assignee)

Comment 2

10 years ago
Created attachment 304829 [details] [diff] [review]
Patch v1

Also made some fixes to the comments on the blocks in edit and added myself to the contributor lists :)
Attachment #304829 - Flags: review?(mkanat)

Comment 3

10 years ago
Comment on attachment 304829 [details] [diff] [review]
Patch v1

Looks good. Fix some of those super-long JS lines when you check this in, though. Lines should be limited to 80 characters.

Also, change that one comment to "Depends On / Blocks" instead of "Depends on blocks"
Attachment #304829 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval+

Updated

10 years ago
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 4

10 years ago
Created attachment 306669 [details] [diff] [review]
Patch v2

You approved this already. However, that last patch had a bunch of crap in there that wasn't supposed to be in there. This patch is all cleaned up.
Attachment #304829 - Attachment is obsolete: true
Attachment #306669 - Flags: review?(mkanat)

Comment 5

10 years ago
Comment on attachment 306669 [details] [diff] [review]
Patch v2

>-    YAHOO.util.Event.addListener(action, 'click', showEditableField, new Array(container, input) );
>+    YAHOO.util.Event.addListener(action,
>+                                 'click',
>+                                 showEditableField,
>+                                 new Array(container, input)
>+                                 );

  Nit: That's *too many* lines. You can just make things look like this:

YAHOO.util.Event.addListener(action, 'click', showEditableField,
                             new Array(container, input));

  Also, I think we should add a global alias YUE for YAHOO.util.Event, and YUD for YAHOO.util.Dom.

  Otherwise, this looks fine. I assume that you swear on a stack of...whatever's important to HCI people, Neilsen or something...that you tested this and it works, because I did not test this patch.
Attachment #306669 - Flags: review?(mkanat) → review+

Comment 6

10 years ago
Before committing this patch, I would like the "That's *too many* lines" comment to be addressed first. I agree that's too many, and this doesn't make the code more readable.

Also, how do you explain that the patch increases from 4.76 Kb to 10.52 Kb? I don't get it. Could we focus on the topic of the bug only? It seems to me that many changes are unrelated to this bug (indentation only, or something like that).
(Assignee)

Comment 7

10 years ago
ack! I committed the patch last night b/c i thought it was approved so i committed it. I thought the cvs automatically updated the bmo page... but it didn't. If someone can revert the change that would be good or I can just make another patch to remove the new lines. I'll not commit again till I hear otherwise.

To address LpSolit's concerns about why it got so much bigger. I went through the whole fields.js file and fixed ALL the lines that were longer than 80 characters long (which was more than just the original 3 line fix that i did) if you do a quick diff of them you can see that. Sorry about that. I was just trying to meet max's concerns about the 80 lines.

I agree with max's suggestion of making an alias to YAHOO.util.Event and YAHOO.util.Dom.

Let me know what you guys would like I'll make another patch with max's suggestions. And also fix the other things with the too many lines issue as well. 
(Assignee)

Comment 8

10 years ago
filed bug 420494 to address mkanat's suggestion.
(Assignee)

Comment 9

10 years ago
Created attachment 306778 [details] [diff] [review]
Indent Fix

fixes the committed code to not be so many lines. see bug 420494 which addresses mknanat's other suggestions. Sorry again for prematurely committing the patch with too many lines in it.
Attachment #306778 - Flags: review?(mkanat)
Attachment #306778 - Flags: review?(LpSolit)

Comment 10

10 years ago
Comment on attachment 306778 [details] [diff] [review]
Indent Fix

Nit: you should be consistent about spaces after commas. But that's minor.

r=LpSolit a=LpSolit for this patch. Do not forget to paste the output of CVS here this time, and mark this bug as FIXED. (it completely went out of my radar because the previous checkin has never been reported to this bug)
Attachment #306778 - Flags: review?(mkanat)
Attachment #306778 - Flags: review?(LpSolit)
Attachment #306778 - Flags: review+

Comment 11

10 years ago
Checking in js/field.js;
/cvsroot/mozilla/webtools/bugzilla/js/field.js,v  <--  field.js
new revision: 1.6; previous revision: 1.5
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.