144563 - Need to disable UI when they are locked

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P1)

Description

[tao]

When email/news account preferences are locked, the associated GUI should be
greyed out should users won't be able to modify them.

Comment 1

[tao]

nominating

Comment 2

[Rodney Velasco]

QA over to me.  customization issue.

Comment 3

[racham]

Attachment: patch, v1.0

Comment 4

[racham]

Attachment: ns (commercial tree) patch, v1.0

Comment 5

[racham]

Attachment: sample file used in locking a preconfigured account

This file contains pre-configured settings in all.js and more in mn_prefs.txt
(which I used to generate a cfg file using make_cfg tool). In mn_prefs.txt, I
have added comments next to each pref (from all those AccountManager panels)
about the pref and the values we can set in case of certain prefs.

For locking Addressing panel prefs, use following prefs.

mail.identity.<idKey>.overrideGlobal_Pref  this is a boolean pref
mail.identity.<idKey>.directoryServer this is a string pref and the value is
"ldap_2.servers.<server-name>"

Comment 6

[racham]

I have requested Rodney & Ninoschka to run their test suites with the optimized
build I have provided them.

Also, I have requested Srilatha and Seth to take a look at the patch and provide
any feedback.

Comment 7

[(not reading, please use [e-mail] instead)]

looks good.  some minor comments / questions:

1)

Trying to understand...instead of disabling / enabling the ui using the
prefstring trick, we've got another pref.

-  if (nsPrefBranch.prefIsLocked(addAccountButton.getAttribute("prefstring")))
+  if (!nsPrefBranch.getBoolPref("mail.enable_new_account_addition"))
     canCreate = false;
-  //if (nsPrefBranch.prefIsLocked(duplicateButton.getAttribute("prefstring")))
-  //  canDuplicate = false;
-  if (nsPrefBranch.prefIsLocked(setDefaultButton.getAttribute("prefstring")))
+  if (!nsPrefBranch.getBoolPref("mail.enable_default_account_setting"))
     canSetDefault = false;
-  if (nsPrefBranch.prefIsLocked(removeButton.getAttribute("prefstring")))
+  if (!nsPrefBranch.getBoolPref("mail.enable_account_deletion"))
     canDelete = false;
 

-      <button label="&addAccountButton.label;" oncommand="onAddAccount(event);"
id="addAccountButton"
-              prefstring="mail.accountmanager.accounts"/>
+      <button label="&addAccountButton.label;" oncommand="onAddAccount(event);"
id="addAccountButton"/>
       <button label="&setDefaultButton.label;" oncommand="onSetDefault(event);"
disabled="true" id="setDefaultButton"
               prefstring="mail.disable_button.set_default_account"/>

Since you removed these three lines,

-  if (nsPrefBranch.prefIsLocked(addAccountButton.getAttribute("prefstring")))
-  if (nsPrefBranch.prefIsLocked(setDefaultButton.getAttribute("prefstring")))
-  if (nsPrefBranch.prefIsLocked(removeButton.getAttribute("prefstring")))

you should have removed three prefstring attributes from AccountManager.xul,
right?  I think you missed two.

2)

Does this mean if the fcc is locked, we aren't going to set it up correctly, or
am I misreading the code?

You'd think we'd want to set it up, and then disable it.

 // Check the Fcc Self item and setup associated picker state 
 function setupFccItems()
 { 
+  if (!gFccRadioElemChoiceLocked) {
     var broadcaster = document.getElementById("broadcaster_doFcc");
 
     var checked = document.getElementById("identity.doFcc").checked;
@@ -313,6 +315,7 @@
 	}
     else
         broadcaster.setAttribute("disabled", "true");
+  }
 }

3)

did you just choose another pref, to keep pref names uniform?  Does this require
a change to CCK?
 
-      { prefstring:"disable_button.selectFolder", id:"selectFolderButton"}
+      { prefstring:"selectFolderButton", id:"selectFolderButton"}
     ];
 
4)
 
+      { prefstring:"select_custom_prefs", id:"identity.select_custom_prefs"},
+      { prefstring:"select_global_prefs", id:"identity.select_global_prefs"},

and

       <radiogroup id="identity.use_custom_prefs" wsm_persist="true"
genericattr="true" 
                   preftype="bool"
prefstring="mail.identity.%identitykey%.use_custom_prefs" 
                   oncommand="EnableDisableCustomSettings();">
-        <radio value="false" label="&useGlobalPrefs.label;"/>
-        <radio value="true" label="&useCustomPrefs.label;"/> 
+        <radio id="identity.select_global_prefs" value="false"
label="&useGlobalPrefs.label;"/>
+        <radio id="identity.select_custom_prefs" value="true"
label="&useCustomPrefs.label;"/> 
       </radiogroup>
     </hbox>
 
it looks like the pref string is already there
(mail.identity.%identitykey%.use_custom_prefs) to lock the radiogroup.
was it not working?  It now looks like you are locking the individual radio
elements.  It seems like locking the radiogroup would be enough.

5)

 function CreateWebMailAccount()
 {
     try {
+      if (!prefs.getBoolPref("mail.enable_new_account_addition"))
+        return null;

While this is creating a new account, do we want an entire new pref for this?

Comment 8

[racham]

Seth,

Thanks for the reviews.

* Comment 1 : Though this works, I guess I can get back to prefstrings mechanism
as that eliminates need for adding new default prefs.

* Comment 2 : It works. But, bit round about. SetupFccPickerState() can be
modified to achieve the control we want in a better way. I will remove that
check locking in there. Thanks.

* Comment 3 : While doing locking, I have noticed for buttons, value of true or
false didn't matter. So, I thought it is just fine to have a pref that suggest
that we are locking without any verbs like diable and enable. I will check other
such instances also.

* Comment 4 : This particular guess, under use custom prefs choice, we have a
whole new set of item (when user selects custom receipts). If we simply lock
everything based on radio group lock, we would take away the flexibility of
locking subcomponents under the custom receipts item. With the current patch,
one can lock elements of choice under custom receipts choice.

* Comment 5 : I have to check for this pref here as we need to block any account
creation activity here.

thanks,
Bhuvan

Comment 9

[racham]

Attachment: patch, v1.1

Comment 10

[racham]

Attachment: ns (commercial tree) patch, v1.1

Comment 11

[racham]

Attachment: new sample pre-config file

This file has few more new prefs to be part of global prefs file and also some
new prefs to be locked in order to achieve locking without introducing new
prefs.

Comment 12

[David :Bienvenu]

Comment on attachment 87907 [details] [diff] [review]
patch, v1.1

sr=bienvenu

Comment 13

[David :Bienvenu]

Comment on attachment 87908 [details] [diff] [review]
ns (commercial tree) patch, v1.1

sr=bienvenu

Comment 14

[Rodney Velasco]

Using bhuvans second optimized build I was having some problems with with the
security preferences:

lockPref("mail.identity.id9991.signingCertSelectButton", false);          
lockPref("mail.identity.id9991.encryptionCertSelectButton", true);       

The radio buttons are locked down from user interaction at first launch, but the
text area field to specify a valid certificate unlocks the check box and radio
box if you enter in random text in that field, exit the mail news account
settings pref panel, and open it up again. If we lock down the two above
preferences we should lock down the text area field as well. 

All other prefs are locked down as expected.  No other unexpected behavior seen
with the rest of the prefs defined in the attached sample pre-config file.

Comment 15

[racham]

Those text areas fields (Security panel) not editable. Cert names are filled in
those boxes only after user clicks on the button and selects a cert. Once we
lock those buttons that panel is pretty much locked. Anyway, you can list the
steps that led you to achieve the unlocking. Thanks.

Comment 16

[Srilatha Moturi]

Comment on attachment 87908 [details] [diff] [review]
ns (commercial tree) patch, v1.1

r=srilatha

Comment 17

[Srilatha Moturi]

Comment on attachment 87907 [details] [diff] [review]
patch, v1.1

r=srilatha

Comment 18

[racham]

Fix checked in on the trunk. Marking Fixed. Thanks for reviews and comments.

Comment 19

[Rodney Velasco]

Great work bhuvan, all mail/news preferences defined in the latest trunk builds
are lockable.  

A note to code reveiewers, since bhuvan locked all the prefs on a per pref basis
it would be nice for any new preferences added to the mail/news account settings
UI follow the locking guidelines as defined in this newsgroup posting:

news://news.mozilla.org/3AC8F5D2.6CA02623%40netscape.com

I'm not aware of anymore preferences being added to the mail/news account
settings this late in the game, but any future work on prefs should be lockable
from a customization point of view (unless bug 79305 ever gets fixed), or else
I'll be filing bugs to try and get them locked.  Thanks again bhuvan for fixing
this for the customization group.

Verified on Trunk
Macos X.1.3 2002061908
Macos9.2.2 2002061908
Linux 2.4.7-10 2002061908
Windows NT 4.0 2002061908

Comment 20

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch.  Please get
drivers approval before checking in. When you check this into the branch, please
change the mozilla1.0.1+ keyword to fixed1.0.1

Comment 21

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 22

[racham]

Thanks for the approvals. Will check this in as soon as branch opens.

Comment 23

[racham]

Fix checked in on the branch. Files (all right ones) got commited accidentally 
without checkin comments. So, I am providing comments for the record here 
(already added to the comment box on the tinderbox).

Fixing bug 144563. Disable mailnews AccountManager panel elements when the 
corresponding preferences are locked. This is a useful feature for 
Administrators and vendors. Special configuration tool is need to be used for 
generating a config file to feed the locked prefs to the app. r=srilatha, 
sr=bienvenu, a=drivers,adt.

Adding fixed1.0.1 keyword.

Comment 24

[Rodney Velasco]

verified on commercial branch

Linux 2.4.7-10 2002062806
Windows NT 4.0 2002062808
Mac OS X.1 2002062809
Mac OS 9.2.2 2002062805

Comment 25

[racham]

*** Bug 96152 has been marked as a duplicate of this bug. ***