Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range

2013-06-13 Thread Ana Krivokapic 
On 06/12/2013 12:14 PM, Martin Kosek wrote:
 On 06/12/2013 11:40 AM, Tomas Babej wrote:
 On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
 On Mon, 10 Jun 2013, Ana Krivokapic wrote:
 And once here(added by your patch):

 +if not self.validate_range(*keys, **options):
 +raise errors.ValidationError(
 +name=_('id range'),
 +error=_(
 +'An id range already exists for this trust. '
 +'You should either delete the old range, or '
 +'exclude --base-id/--range-size options from the
 command.'

 I'd suggest we have one common place, say validate_range() function as
 we have now,
 that contains all the checks (more coming in the future for sure).

 That would mean that the whole trust-add command will fail in the case
 that ID range
 with the same name but different domain SID already exists, since we
 now call validate_range()
 within execute() method. I checked with Alexander and we agreed that
 this is more desired behaviour.

 This would also result to reducement of the number of API calls, which
 is a nice benefit.

 Tomas
 This updated patch completely separates validation logic and object
 creation logic of the trust_add command. I added two new methods:

 * validate_range(), which ensures that the options and environment
 related to idrange is valid, and
 * validate_options, which takes care of other general validation

 One change introduced in this patch is making the
 __populate_remote_domain() method of TrustDomainJoins class public, and
 calling it from trust_add. This was necessary in order to enable
 discovering details of the trusted domain in validation phase.
 Looks good overall but I'd suggest to wrap populate_remote_domain()
 calls in join_ad_* methods instead of removing them. If remote domain is
 not initialized (i.e. 
 not(isinstance(self.remote_domain,TrustedDomainInstance)),
 then call populate_remote_domain() as it was before.

Fixed.

 I tested the patch and it works fine.

 There's a lot of refactoring done, but the changes preserve equal state. 
 Aside
 from
 Alexander's comment up here, I have but one nitpick.

 There are few constructs of the form:

 try:
value = dictionary['keyword']
 except KeyError:
value = None

 or

 if 'keyword' in dictionary:
 value = dictionary['keyword']
 else:
 value = None

 Can you please substitute these with value = dictionary.get(keyword, None)?
 Not everywhere, but there are places where it can improve readability of the 
 code.
 You can even use just value = dictionary.get(keyword) as None is the default
 return value:

 print {}.get(foo)

Fixed.

 None

 Martin

 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel

Updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 370b91c93c3c93f63aa45069feece618375c406b Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range

When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.

https://fedorahosted.org/freeipa/ticket/3635
---
 ipalib/plugins/trust.py | 215 +---
 ipaserver/dcerpc.py |  15 +++-
 2 files changed, 159 insertions(+), 71 deletions(-)

diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..5c9360b57982ef1284ec741d3ee6114345e2e7c2 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
 autofill=True,
 )
 
+DEFAULT_RANGE_SIZE = 20
+
 def trust_type_string(level):
 
 Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class trust_add(LDAPCreate):
 Int('range_size?',
 cli_name='range_size',
 label=_('Size of the ID range reserved for the trusted domain'),
-default=20,
+default=DEFAULT_RANGE_SIZE,
 autofill=True
 ),
 )
@@ -296,20 +298,12 @@ class trust_add(LDAPCreate):
 has_output_params = LDAPCreate.has_output_params + trust_output_params
 
 def execute(self, *keys, **options):
-if not _murmur_installed and 'base_id' not in options:
-raise errors.ValidationError(name=_('missing base_id'),
-error=_('pysss_murmur is not available on the server ' \
-'and no base-id is given.'))
+full_join = self.validate_options(*keys, **options)
+old_range, range_name, dom_sid = self.validate_range(*keys, **options)
+result = self.execute_ad(full_join, *keys, **options)
 
-if 'trust_type' in

Re: [Freeipa-devel] [PATCH] 420 Regression fix: rule table with ext. member support doesn't offer any items

2013-06-13 Thread Petr Vobornik 

On 06/13/2013 02:46 PM, Ana Krivokapic wrote:

On 06/13/2013 12:30 PM, Petr Vobornik wrote:

Rule tables with external member has more than one column and
therefore exclude parameter for adder dialog is not array of strings
but array of objects. normalize_values function can't work with it and
causes JS error.

This patch creates proper exclude array before passing it to adder
dialog.

https://fedorahosted.org/freeipa/ticket/3711



ACK




I've noticed that there is a leftover from a different approach:

 pkey: pkey,
+param: that.name,
 other_entity: that.other_entity,

The line is removed, updated patch attached.
--
Petr Vobornik
From 93e3b2a4cec826029beb2f58beb16049886cb04d Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 12 Jun 2013 16:01:26 +0200
Subject: [PATCH] Regression fix: rule table with ext. member support doesn't
 offer any items

There is a JS error.

Rule tables with external member has more than one column and therefore exclude parameter for adder dialog is not array of strings but array of objects. normalize_values function can't work with it and causes JS error.

This patch creates proper exclude array before passing it to adder dialog.

https://fedorahosted.org/freeipa/ticket/3711
---
 install/ui/src/freeipa/rule.js | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/rule.js b/install/ui/src/freeipa/rule.js
index 959aec462d1c1e36220d3569fe2e776a60ab2c11..f573b56d469ac8c6652f8e49c66fd6fd1259db90 100644
--- a/install/ui/src/freeipa/rule.js
+++ b/install/ui/src/freeipa/rule.js
@@ -149,6 +149,14 @@ IPA.rule_association_table_widget = function(spec) {
 title = title.replace('${primary_key}', pkey);
 title = title.replace('${other_entity}', other_entity_label);
 
+var exclude = that.values;
+if (that.external) {
+exclude = [];
+for (var i=0; ithat.values.length; i++) {
+exclude.push(that.values[i][that.name]);
+}
+}
+
 return IPA.rule_association_adder_dialog({
 title: title,
 pkey: pkey,
@@ -156,7 +164,7 @@ IPA.rule_association_table_widget = function(spec) {
 attribute_member: that.attribute_member,
 entity: that.entity,
 external: that.external,
-exclude: that.values
+exclude: exclude
 });
 };
 
-- 
1.8.1.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 420 Regression fix: rule table with ext. member support doesn't offer any items

2013-06-13 Thread Ana Krivokapic 
On 06/13/2013 03:24 PM, Petr Vobornik wrote:
 On 06/13/2013 02:46 PM, Ana Krivokapic wrote:
 On 06/13/2013 12:30 PM, Petr Vobornik wrote:
 Rule tables with external member has more than one column and
 therefore exclude parameter for adder dialog is not array of strings
 but array of objects. normalize_values function can't work with it and
 causes JS error.

 This patch creates proper exclude array before passing it to adder
 dialog.

 https://fedorahosted.org/freeipa/ticket/3711

 ACK



 I've noticed that there is a leftover from a different approach:

  pkey: pkey,
 +param: that.name,
  other_entity: that.other_entity,

 The line is removed, updated patch attached.


 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel

I missed it too in the first patch. ACK for updated patch.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0036 Fix displaying of success message

2013-06-13 Thread Ana Krivokapic 
On 06/13/2013 05:18 PM, Petr Vobornik wrote:
 On 06/13/2013 04:16 PM, Ana Krivokapic wrote:
 Hello,

 Make sure that the success message in web UI is properly populated with 
 actual
 number of items that were successfully added/removed.

 https://fedorahosted.org/freeipa/ticket/3708


 The changes look good. But I've found an existing issue in the code which was
 moved to get_succeeded:

 When no operation succeeds following code behaves incorrectly:

 +var succeeded = data.result.completed;
 +
 +if (!succeeded) {

 The condition is evaluated as true when data.result.completed === 0. It's not
 desired because the subsequent commands in the if block will raise JS error
 (data.result doesn't have results obj).

 So we should check for:
 if (typeof succeeded !== 'number')



Nice catch, thanks. Updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From e9b240b21c37751e106d8e54e974fd8a58e25c93 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 13 Jun 2013 16:10:33 +0200
Subject: [PATCH] Fix displaying of success message

Make sure that the success message is properly populated with actual number of
items that were successfully added/removed.

https://fedorahosted.org/freeipa/ticket/3708
---
 install/ui/src/freeipa/association.js | 42 +--
 install/ui/src/freeipa/ipa.js | 15 +
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/install/ui/src/freeipa/association.js b/install/ui/src/freeipa/association.js
index 15e164963e93faddc3e9ddfbc428c1c74df3bf95..c60c7b8afe9c16ae55e5147574664c60afc43d3e 100644
--- a/install/ui/src/freeipa/association.js
+++ b/install/ui/src/freeipa/association.js
@@ -609,10 +609,13 @@ IPA.association_table_widget = function (spec) {
 dialog.execute = function() {
 that.add(
 dialog.get_selected_values(),
-function() {
+function(data) {
 that.refresh();
 dialog.close();
-IPA.notify_success('@i18n:association.added');
+
+var succeeded = IPA.get_succeeded(data);
+var msg = text.get('@i18n:association.added').replace('${count}', succeeded);
+IPA.notify_success(msg);
 },
 function() {
 that.refresh();
@@ -671,9 +674,12 @@ IPA.association_table_widget = function (spec) {
 dialog.execute = function() {
 that.remove(
 selected_values,
-function() {
+function(data) {
 that.refresh();
-IPA.notify_success('@i18n:association.removed');
+
+var succeeded = IPA.get_succeeded(data);
+var msg = text.get('@i18n:association.removed').replace('${count}', succeeded);
+IPA.notify_success(msg);
 },
 function() {
 that.refresh();
@@ -1084,17 +1090,8 @@ exp.association_facet = IPA.association_facet = function (spec, no_init) {
 on_success: function(data) {
 that.refresh();
 dialog.close();
-var succeeded = data.result.completed;
-
-if (!succeeded) {
-succeeded = 0;
-for (var i = 0; i data.result.results.length; i++) {
-if (data.result.results[i].completed === 1) {
-succeeded++;
-}
-}
-}
 
+var succeeded = IPA.get_succeeded(data);
 var msg = text.get('@i18n:association.added').replace('${count}', succeeded);
 IPA.notify_success(msg);
 },
@@ -1148,17 +1145,7 @@ exp.association_facet = IPA.association_facet = function (spec, no_init) {
 on_success: function(data) {
 that.refresh();
 
-var succeeded = data.result.completed;
-
-if (!succeeded) {
-succeeded = 0;
-for (var i = 0; i data.result.results.length; i++) {
-if (data.result.results[i].completed === 1) {
-succeeded++;
-}
-}
-}
-
+var succeeded = IPA.get_succeeded(data);
 var msg = text.get('@i18n:association.removed').replace('${count}', succeeded);
 IPA.notify_success(msg);
 },
@@ -1417,7 +1404,10 @@ exp.attribute_facet = IPA.attribute_facet = function(spec, no_init) {
 function(data) {
 that.load(data);
 that.show_content();
-

Re: [Freeipa-devel] [PATCH] 0036 Fix displaying of success message

2013-06-13 Thread Petr Vobornik 

On 06/13/2013 05:33 PM, Ana Krivokapic wrote:

On 06/13/2013 05:18 PM, Petr Vobornik wrote:

On 06/13/2013 04:16 PM, Ana Krivokapic wrote:

Hello,

Make sure that the success message in web UI is properly populated with actual
number of items that were successfully added/removed.

https://fedorahosted.org/freeipa/ticket/3708



The changes look good. But I've found an existing issue in the code which was
moved to get_succeeded:

When no operation succeeds following code behaves incorrectly:

+var succeeded = data.result.completed;
+
+if (!succeeded) {

The condition is evaluated as true when data.result.completed === 0. It's not
desired because the subsequent commands in the if block will raise JS error
(data.result doesn't have results obj).

So we should check for:
 if (typeof succeeded !== 'number')




Nice catch, thanks. Updated patch attached.



ACK, pushed to master, ipa-3-2.
--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 420 Regression fix: rule table with ext. member support doesn't offer any items

2013-06-13 Thread Petr Vobornik 

On 06/13/2013 05:13 PM, Ana Krivokapic wrote:

On 06/13/2013 03:24 PM, Petr Vobornik wrote:

On 06/13/2013 02:46 PM, Ana Krivokapic wrote:

On 06/13/2013 12:30 PM, Petr Vobornik wrote:

Rule tables with external member has more than one column and
therefore exclude parameter for adder dialog is not array of strings
but array of objects. normalize_values function can't work with it and
causes JS error.

This patch creates proper exclude array before passing it to adder
dialog.

https://fedorahosted.org/freeipa/ticket/3711



ACK



I've noticed that there is a leftover from a different approach:

 pkey: pkey,
+param: that.name,
 other_entity: that.other_entity,

The line is removed, updated patch attached.



I missed it too in the first patch. ACK for updated patch.



pushed to master, ipa-3-2.



--
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.


--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel