Re: [Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
On 02/18/2011 04:10 AM, Martin Kosek wrote:
On Thu, 2011-02-17 at 12:29 -0500, Adam Young wrote:
Looks good. Only problem is on braces. we have a code standard that
is like this
IPA.something = function () {
not
IPA.something = function ()
{
This is due to Javascript being ambiguous in certain circumstances
about where it puts an implicit end of statement.
https://fedorahosted.org/freeipa/wiki/Javascript_Coding_Standards
Yes. The same convention is for C/Python code. All those functions
violating a code standard were already in UI, I just moved them to
sub-namespace in the preceding patch.
Nevertheless, I went through all function definitions and I believe I
fixed all occurrences of this issue.
For name shortening, sudo.sudorule_ should be sudo.rule_
Obviously :-)
On the patch I sent you as an example, I broke the View Cert
button. I didn't test that here. Did you make sure that still
works?
Yes, this was already fixed. It was also related to the JSLint warnings
in your patch that you mentioned earlier. But just to be sure I
double-checked this and its OK.
Patch attached. JSLint, test suite OK.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACK. Pushed to master
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
On Wed, 2011-02-16 at 10:46 -0500, Adam Young wrote:
Almost there.
I'd like to pull the sudo namespace out of ipa.js and put it into
sudorule.js, then indicate that the other sudo files depend on sudo
rule.
I guess I should have been clearer: stuff like facets and widgets
don't need to go into a sub, namespace, just custom code called by
them. I'm thinking that widgets and facets in the long term should
become a sub-namespace of IPA themselseves: so IPA.widget.text,
IPA.facet.details, and then the more specific ones. While I don't
want to do that in this patch, keep that in mind when deciding which
namespace to put something into. A good rul of thumb is that an
entity name should not be repeated in a function name, so something
like IPA.sudo.sudorule_details_facet should be
IPA.sudorule_details_facet but any custom functions it calls should
be in IPA.sudo.
I have prepared a next version of patch with the above comments applied.
Facets and widgets are in IPA namespace now. Still, I cannot do much of
a renaming with sub-namespace custom methods that are called by *_widget
or *_facet functions - they would collide. E.g.
IPA.sudo.sudorule_add_dialog cannot be renamed to IPA.sudo.add_dialog
because it would collide with renamed IPA.sudo.sudocmd_add_dialog.
I'm being a bit picky here as this is probably the last major cleanup
we'll get to do before GA, and this is the code that people will look
at. I want it to be as understandable as possible.
I know that since you have worked on WebUI for a long time, you have a
pretty clear picture what it should look like. I hope this patch version
is consistent with the plan.
Martin
From 90fbbcbf5d5eeaad317666f2c347b90c21786b54 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Wed, 16 Feb 2011 14:26:35 +0100
Subject: [PATCH] Remove WebUI identifiers from global namespace
Many WebUI identifiers were defined in a global namespace. This is
not a good programming practice and may result in name clashes,
for example with other libraries.
This patch moves these variables to IPA namespace or its
sub-namespaces, when meaningful.
https://fedorahosted.org/freeipa/ticket/212
---
install/ui/certificate.js| 726 +-
install/ui/entity.js |5 +-
install/ui/host.js | 14 +-
install/ui/ipa.js|1 -
install/ui/navigation.js | 259 ++--
install/ui/policy.js | 16 +-
install/ui/search.js | 71
install/ui/service.js| 14 +-
install/ui/sudocmd.js|6 +-
install/ui/sudocmdgroup.js |6 +-
install/ui/sudorule.js | 22 +-
install/ui/test/certificate_tests.js | 36 +-
install/ui/test/navigation_tests.js | 60 ++--
install/ui/webui.js |6 +-
14 files changed, 592 insertions(+), 650 deletions(-)
diff --git a/install/ui/certificate.js b/install/ui/certificate.js
index 3158d04883af8cb7eb1f9a0f02e936801f0ea358..56995282f29e36c3999535b811abed37225f589f 100755
--- a/install/ui/certificate.js
+++ b/install/ui/certificate.js
@@ -20,377 +20,379 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
-var BEGIN_CERTIFICATE = '-BEGIN CERTIFICATE-';
-var END_CERTIFICATE = '-END CERTIFICATE-';
-
-var BEGIN_CERTIFICATE_REQUEST = '-BEGIN CERTIFICATE REQUEST-';
-var END_CERTIFICATE_REQUEST = '-END CERTIFICATE REQUEST-';
-
-var CRL_REASON = [
-'Unspecified',
-'Key Compromise',
-'CA Compromise',
-'Affiliation Changed',
-'Superseded',
-'Cessation of Operation',
-'Certificate Hold',
-null,
-'Remove from CRL',
-'Privilege Withdrawn',
-'AA Compromise'
-];
-
-var CERTIFICATE_STATUS_MISSING = 0;
-var CERTIFICATE_STATUS_VALID = 1;
-var CERTIFICATE_STATUS_REVOKED = 2;
-
-function certificate_parse_dn(dn) {
-
-var result = {};
-if (!dn) return result;
-
-// TODO: Use proper LDAP DN parser
-var rdns = dn.split(',');
-for (var i=0; irdns.length; i++) {
-var rdn = rdns[i];
-if (!rdn) continue;
-
-var parts = rdn.split('=');
-var name = $.trim(parts[0].toLowerCase());
-var value = $.trim(parts[1]);
-
-var old_value = result[name];
-if (!old_value) {
-result[name] = value;
-} else if (typeof old_value == string) {
-result[name] = [old_value, value];
-} else {
-result[name].push(value);
-}
-}
-return result;
-}
-
-function certificate_get_dialog(spec) {
-var that = {};
-spec = spec || {};
-
-that.title = spec.title || '';
-that.usercertificate = spec.usercertificate || '';
-
-var dialog = $('div/', {
-'title': that.title
-});
-
-var textarea = $('textarea/', {
-readonly: 'yes',
-style: 'width: 100%; height:
Re: [Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
On 02/17/2011 05:21 AM, Martin Kosek wrote:
On Wed, 2011-02-16 at 10:46 -0500, Adam Young wrote:
Almost there.
I'd like to pull the sudo namespace out of ipa.js and put it into
sudorule.js, then indicate that the other sudo files depend on sudo
rule.
I guess I should have been clearer: stuff like facets and widgets
don't need to go into a sub, namespace, just custom code called by
them. I'm thinking that widgets and facets in the long term should
become a sub-namespace of IPA themselseves: so IPA.widget.text,
IPA.facet.details, and then the more specific ones. While I don't
want to do that in this patch, keep that in mind when deciding which
namespace to put something into. A good rul of thumb is that an
entity name should not be repeated in a function name, so something
like IPA.sudo.sudorule_details_facet should be
IPA.sudorule_details_facet but any custom functions it calls should
be in IPA.sudo.
I have prepared a next version of patch with the above comments applied.
Facets and widgets are in IPA namespace now. Still, I cannot do much of
a renaming with sub-namespace custom methods that are called by *_widget
or *_facet functions - they would collide. E.g.
IPA.sudo.sudorule_add_dialog cannot be renamed to IPA.sudo.add_dialog
because it would collide with renamed IPA.sudo.sudocmd_add_dialog.
I'm being a bit picky here as this is probably the last major cleanup
we'll get to do before GA, and this is the code that people will look
at. I want it to be as understandable as possible.
I know that since you have worked on WebUI for a long time, you have a
pretty clear picture what it should look like. I hope this patch version
is consistent with the plan.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Looks good. Only problem is on braces. we have a code standard that is
like this
IPA.something = function () {
not
IPA.something = function ()
{
This is due to Javascript being ambiguous in certain circumstances about
where it puts an implicit end of statement.
https://fedorahosted.org/freeipa/wiki/Javascript_Coding_Standards
For name shortening, sudo.sudorule_ should be sudo.rule_
On the patch I sent you as an example, I broke the View Cert
button. I didn't test that here. Did you make sure that still works?
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
On 02/16/2011 10:16 AM, Martin Kosek wrote: On Tue, 2011-02-15 at 13:26 -0500, Adam Young wrote: On 02/15/2011 08:25 AM, Martin Kosek wrote: Many WebUI identifiers were defined in a global namespace. This is not a good programming practice and may result in name clashes, for example with other libraries. This patch moves these variables to IPA namespace or its sub-namespaces, if required. https://fedorahosted.org/freeipa/ticket/212 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Martin, he is the patch I did for the cert portion. I'll toss it, but you can see what I was thinking as far as hoe to shorten the names: BTW, you should reverse the names of your patch so that they start with freeipa, and then your user id. Adam, thanks for all the remarks. Here is a second version of the patch based on your work on certificate module. I tried to keep the patch simple without too much changes. There are 3 new subnamespaces in this patch - sudo, cert and nav. I have removed a lot of unused code in search.js, you may want to check it closer. This enabled me to move search_generate_checkbox_td functionality just to a place where its needed. Almost there. I'd like to pull the sudo namespace out of ipa.js and put it into sudorule.js, then indicate that the other sudo files depend on sudo rule. I guess I should have been clearer: stuff like facets and widgets don't need to go into a sub, namespace, just custom code called by them. I'm thinking that widgets and facets in the long term should become a sub-namespace of IPA themselseves: so IPA.widget.text, IPA.facet.details, and then the more specific ones. While I don't want to do that in this patch, keep that in mind when deciding which namespace to put something into. A good rul of thumb is that an entity name should not be repeated in a function name, so something like IPA.sudo.sudorule_details_facet should be IPA.sudorule_details_facet but any custom functions it calls should be in IPA.sudo. I'm being a bit picky here as this is probably the last major cleanup we'll get to do before GA, and this is the code that people will look at. I want it to be as understandable as possible. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
Many WebUI identifiers were defined in a global namespace. This is
not a good programming practice and may result in name clashes,
for example with other libraries.
This patch moves these variables to IPA namespace or its
sub-namespaces, if required.
https://fedorahosted.org/freeipa/ticket/212
From e22a16fe897bcd61d231091a05c87dd77e8c349d Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Mon, 14 Feb 2011 16:43:19 +0100
Subject: [PATCH] Remove WebUI identifiers from global namespace
Many WebUI identifiers were defined in a global namespace. This is
not a good programming practice and may result in name clashes,
for example with other libraries.
This patch moves these variables to IPA namespace or its
sub-namespaces, if required.
https://fedorahosted.org/freeipa/ticket/212
---
install/ui/associate.js | 12 ++--
install/ui/certificate.js| 88 +-
install/ui/entity.js |6 +--
install/ui/host.js | 10 ++--
install/ui/ipa.js|1 +
install/ui/navigation.js | 46 +-
install/ui/policy.js |2 +-
install/ui/search.js | 18
install/ui/serverconfig.js |2 +-
install/ui/service.js| 10 ++--
install/ui/test/association_tests.js |4 +-
install/ui/test/certificate_tests.js | 18
install/ui/test/navigation_tests.js | 54 ++--
install/ui/webui.js |6 +-
14 files changed, 137 insertions(+), 140 deletions(-)
diff --git a/install/ui/associate.js b/install/ui/associate.js
index 2d416f0fd7482bb53ffa80addec5e92c2299cdb8..359c29d7c407c25981d4b85b6325484b19d88fab 100644
--- a/install/ui/associate.js
+++ b/install/ui/associate.js
@@ -51,7 +51,7 @@ IPA.associator = function (spec) {
/**
*This associator is built for the case where each association requires a separate rpc
*/
-function serial_associator(spec) {
+IPA.serial_associator = function (spec) {
spec = spec || {};
@@ -90,7 +90,7 @@ function serial_associator(spec) {
*This associator is for the common case where all the asociations can be sent
in a single rpc
*/
-function bulk_associator(spec) {
+IPA.bulk_associator = function (spec) {
spec = spec || {};
@@ -271,7 +271,7 @@ IPA.association_table_widget = function (spec) {
that.other_entity = spec.other_entity;
that.attribute_member = spec.attribute_member;
-that.associator = spec.associator || bulk_associator;
+that.associator = spec.associator || IPA.bulk_associator;
that.add_method = spec.add_method || 'add_member';
that.remove_method = spec.remove_method || 'remove_member';
@@ -300,7 +300,7 @@ IPA.association_table_widget = function (spec) {
var column;
if (association) {
if (association.associator) {
-that.associator = association.associator == 'serial' ? serial_associator : bulk_associator;
+that.associator = association.associator == 'serial' ? IPA.serial_associator : IPA.bulk_associator;
}
if (association.add_method) that.add_method = association.add_method;
@@ -575,7 +575,7 @@ IPA.association_facet = function (spec) {
that.facet_group = spec.facet_group;
that.attribute_member = spec.attribute_member;
-that.associator = spec.associator || bulk_associator;
+that.associator = spec.associator || IPA.bulk_associator;
that.add_method = spec.add_method || 'add_member';
that.remove_method = spec.remove_method || 'remove_member';
@@ -626,7 +626,7 @@ IPA.association_facet = function (spec) {
if (association) {
if (association.associator) {
-that.associator = association.associator == 'serial' ? serial_associator : bulk_associator;
+that.associator = association.associator == 'serial' ? IPA.serial_associator : IPA.bulk_associator;
}
if (association.add_method) that.add_method = association.add_method;
diff --git a/install/ui/certificate.js b/install/ui/certificate.js
index 3158d04883af8cb7eb1f9a0f02e936801f0ea358..d01443ce5f88429364c2e59552ac43c750ad24fa 100755
--- a/install/ui/certificate.js
+++ b/install/ui/certificate.js
@@ -20,13 +20,13 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
-var BEGIN_CERTIFICATE = '-BEGIN CERTIFICATE-';
-var END_CERTIFICATE = '-END CERTIFICATE-';
+IPA.certificates.BEGIN_CERTIFICATE = '-BEGIN CERTIFICATE-';
+IPA.certificates.END_CERTIFICATE = '-END CERTIFICATE-';
-var BEGIN_CERTIFICATE_REQUEST = '-BEGIN CERTIFICATE REQUEST-';
-var END_CERTIFICATE_REQUEST = '-END CERTIFICATE REQUEST-';
+IPA.certificates.BEGIN_CERTIFICATE_REQUEST = '-BEGIN CERTIFICATE REQUEST-';
+IPA.certificates.END_CERTIFICATE_REQUEST = '-END CERTIFICATE
Re: [Freeipa-devel] [PATCH] 031 Remove WebUI identifiers from global namespace
On 02/15/2011 08:25 AM, Martin Kosek wrote:
Many WebUI identifiers were defined in a global namespace. This is
not a good programming practice and may result in name clashes,
for example with other libraries.
This patch moves these variables to IPA namespace or its
sub-namespaces, if required.
https://fedorahosted.org/freeipa/ticket/212
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Martin, he is the patch I did for the cert portion. I'll toss it, but
you can see what I was thinking as far as hoe to shorten the names:
BTW, you should reverse the names of your patch so that they start with
freeipa, and then your user id.
From f7f1007a60938f98156ca5ab73a713c315f288a4 Mon Sep 17 00:00:00 2001
From: Adam Young ayo...@redhat.com
Date: Mon, 14 Feb 2011 11:17:10 -0500
Subject: [PATCH] certificate into IPA namespace
Cleans up the certificate handling code such that all the identifiers fall within the IPA namespace
---
install/ui/certificate.js| 725 +-
install/ui/host.js |2 +-
install/ui/service.js|2 +-
install/ui/test/certificate_tests.js | 36 +-
4 files changed, 383 insertions(+), 382 deletions(-)
diff --git a/install/ui/certificate.js b/install/ui/certificate.js
index 3158d04883af8cb7eb1f9a0f02e936801f0ea358..c286231a8dc1d1adb68fb61e209f257161187cb1 100755
--- a/install/ui/certificate.js
+++ b/install/ui/certificate.js
@@ -20,377 +20,378 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
-var BEGIN_CERTIFICATE = '-BEGIN CERTIFICATE-';
-var END_CERTIFICATE = '-END CERTIFICATE-';
-
-var BEGIN_CERTIFICATE_REQUEST = '-BEGIN CERTIFICATE REQUEST-';
-var END_CERTIFICATE_REQUEST = '-END CERTIFICATE REQUEST-';
-
-var CRL_REASON = [
-'Unspecified',
-'Key Compromise',
-'CA Compromise',
-'Affiliation Changed',
-'Superseded',
-'Cessation of Operation',
-'Certificate Hold',
-null,
-'Remove from CRL',
-'Privilege Withdrawn',
-'AA Compromise'
-];
-
-var CERTIFICATE_STATUS_MISSING = 0;
-var CERTIFICATE_STATUS_VALID = 1;
-var CERTIFICATE_STATUS_REVOKED = 2;
-
-function certificate_parse_dn(dn) {
-
-var result = {};
-if (!dn) return result;
-
-// TODO: Use proper LDAP DN parser
-var rdns = dn.split(',');
-for (var i=0; irdns.length; i++) {
-var rdn = rdns[i];
-if (!rdn) continue;
-
-var parts = rdn.split('=');
-var name = $.trim(parts[0].toLowerCase());
-var value = $.trim(parts[1]);
-
-var old_value = result[name];
-if (!old_value) {
-result[name] = value;
-} else if (typeof old_value == string) {
-result[name] = [old_value, value];
-} else {
-result[name].push(value);
-}
-}
-return result;
-}
-
-function certificate_get_dialog(spec) {
-var that = {};
-spec = spec || {};
-
-that.title = spec.title || '';
-that.usercertificate = spec.usercertificate || '';
-
-var dialog = $('div/', {
-'title': that.title
-});
-
-var textarea = $('textarea/', {
-readonly: 'yes',
-style: 'width: 100%; height: 275px;'
-}).appendTo(dialog);
-
-textarea.val(
-BEGIN_CERTIFICATE+'\n'+
-that.usercertificate+'\n'+
-END_CERTIFICATE );
-
-that.open = function() {
-dialog.dialog({
-modal: true,
-width: 500,
-height: 400,
-buttons: {
-'Close': function() {
-dialog.dialog('destroy');
-}
+IPA.cert = {
+BEGIN_CERTIFICATE : '-BEGIN CERTIFICATE-',
+END_CERTIFICATE : '-END CERTIFICATE-',
+BEGIN_CERTIFICATE_REQUEST : '-BEGIN CERTIFICATE REQUEST-',
+END_CERTIFICATE_REQUEST : '-END CERTIFICATE REQUEST-',
+CRL_REASON : [
+'Unspecified',
+'Key Compromise',
+'CA Compromise',
+'Affiliation Changed',
+'Superseded',
+'Cessation of Operation',
+'Certificate Hold',
+null,
+'Remove from CRL',
+'Privilege Withdrawn',
+'AA Compromise'
+],
+CERTIFICATE_STATUS_MISSING : 0,
+CERTIFICATE_STATUS_VALID : 1,
+CERTIFICATE_STATUS_REVOKED : 2,
+
+parse_dn : function (dn) {
+
+var result = {};
+if (!dn) return result;
+
+// TODO: Use proper LDAP DN parser
+var rdns = dn.split(',');
+for (var i=0; irdns.length; i++) {
+var rdn = rdns[i];
+if (!rdn) continue;
+
+var parts = rdn.split('=');
+var name = $.trim(parts[0].toLowerCase());
+var value = $.trim(parts[1]);
+
+var old_value = result[name];
+if (!old_value) {
+result[name]
