Re: [Freeipa-devel] [PATCH] 336 Added policies into user details page.

2012-01-10 Thread Petr Vobornik

On 01/10/2012 08:35 AM, Endi Sukma Dewata wrote:

On 1/6/2012 7:50 AM, Petr Vobornik wrote:

1) you are calling krbtpolicy-show without any user specific information
so it always get the global policy. It should be call with an user
argument.


Fixed. Right now it's read only. I think we should provide an interface
to edit the Kerberos ticket policy for each user, but I don't think it's
as simple as making the fields editable because there are 2 operations
that we need to support: Update (krbtpolicy-mod) and Reset
(krbtpolicy-reset). The krbtpolicy-mod probably can be called together
with user-mod when we click Update, but we need a new button for the
Reset operation because it's completely different than the details
facet's Reset button.


I wouldn't modify it there too. Maybe the original ticket (new facet) 
wasn't a bad idea. But for reading this implementation is really fine.





Minor:
2) Why not call pwpolicy-show --user=user_login instead of getting the
policy's name from dn?


Fixed. The password policy is intentionally made read only because the
policy belongs to the group, not the user. If we make it editable it
might confuse the admin into thinking that he's changing the policy for
the user only whereas he's actually changing the policy for the whole
group.


Agree


We might be able to show the password policy in group details
page too, but I'm not sure if it's necessary.


We will see if some user will want it.




Combining 1), 2) and user-show will allow to get all necessary
information for the facet in a single batch at refresh.


This will be done in the next patch.


ACK and pushed to master

--
Petr Vobornik

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


Re: [Freeipa-devel] [PATCH] 336 Added policies into user details page.

2012-01-09 Thread Endi Sukma Dewata

On 1/6/2012 7:50 AM, Petr Vobornik wrote:

1) you are calling krbtpolicy-show without any user specific information
so it always get the global policy. It should be call with an user
argument.


Fixed. Right now it's read only. I think we should provide an interface 
to edit the Kerberos ticket policy for each user, but I don't think it's 
as simple as making the fields editable because there are 2 operations 
that we need to support: Update (krbtpolicy-mod) and Reset 
(krbtpolicy-reset). The krbtpolicy-mod probably can be called together 
with user-mod when we click Update, but we need a new button for the 
Reset operation because it's completely different than the details 
facet's Reset button.



Minor:
2) Why not call pwpolicy-show --user=user_login instead of getting the
policy's name from dn?


Fixed. The password policy is intentionally made read only because the 
policy belongs to the group, not the user. If we make it editable it 
might confuse the admin into thinking that he's changing the policy for 
the user only whereas he's actually changing the policy for the whole 
group. We might be able to show the password policy in group details 
page too, but I'm not sure if it's necessary.



Combining 1), 2) and user-show will allow to get all necessary
information for the facet in a single batch at refresh.


This will be done in the next patch.

--
Endi S. Dewata
From 645fb7c46517b3bacad48f042655f3b999d207bf Mon Sep 17 00:00:00 2001
From: Endi Sukma Dewata edew...@redhat.com
Date: Mon, 19 Dec 2011 18:31:35 -0600
Subject: [PATCH] Added policies into user details page.

The user details page has been modified to show the password policy
and Kerberos ticket policy that apply to the user. The policies are
currently displayed as read-only.

Ticket #703
---
 install/ui/details.js   |   46 +++--
 install/ui/dns.js   |2 +-
 install/ui/field.js |9 +-
 install/ui/hbac.js  |4 +-
 install/ui/ipa.js   |  154 +
 install/ui/sudo.js  |4 +-
 install/ui/test/data/ipa_init.json  |2 +-
 install/ui/test/data/user_get_policies.json |  135 ++
 install/ui/test/data/user_show.json |6 +
 install/ui/user.js  |  257 +--
 ipalib/plugins/internal.py  |2 +-
 11 files changed, 460 insertions(+), 161 deletions(-)
 create mode 100644 install/ui/test/data/user_get_policies.json

diff --git a/install/ui/details.js b/install/ui/details.js
index c201dad5df0501532c1cbb02b28cea4eb3c1553a..4adc2770bb169ccc1bdc3c1d93644e899ee0e767 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -512,11 +512,11 @@ IPA.details_facet = function(spec) {
 };
 
 
-that.on_update_success = function(data, text_status, xhr) {
+that.update_on_success = function(data, text_status, xhr) {
 that.load(data);
 };
 
-that.on_update_error = function(xhr, text_status, error_thrown) {
+that.update_on_error = function(xhr, text_status, error_thrown) {
 };
 
 that.add_fields_to_command = function(update_info, command) {
@@ -559,9 +559,8 @@ IPA.details_facet = function(spec) {
 var new_update_info = IPA.update_info_builder.copy(update_info);
 
 if (update_info.fields.length  0) {
-new_update_info.append_command(
-that.create_fields_update_command(update_info),
-IPA.config.default_priority);
+var command = that.create_fields_update_command(update_info);
+new_update_info.append_command(command, IPA.config.default_priority);
 }
 
 new_update_info.commands.sort(function(a, b) {
@@ -609,12 +608,12 @@ IPA.details_facet = function(spec) {
 var command = that.create_update_command();
 
 command.on_success = function(data, text_status, xhr) {
-that.on_update_success(data, text_status, xhr);
+that.update_on_success(data, text_status, xhr);
 if (on_success) on_success.call(this, data, text_status, xhr);
 };
 
 command.on_error = function(xhr, text_status, error_thrown) {
-that.on_update_error(xhr, text_status, error_thrown);
+that.update_on_error(xhr, text_status, error_thrown);
 if (on_error) on_error.call(this, xhr, text_status, error_thrown);
 };
 
@@ -641,7 +640,16 @@ IPA.details_facet = function(spec) {
 return command;
 };
 
-that.refresh = function() {
+that.refresh_on_success = function(data, text_status, xhr) {
+that.load(data);
+};
+
+that.refresh_on_error = function(xhr, text_status, error_thrown) {
+that.redirect_error(error_thrown);
+that.report_error(error_thrown);
+};
+
+that.refresh = function(on_success, on_error) {
 
 that.pkey = 

Re: [Freeipa-devel] [PATCH] 336 Added policies into user details page.

2012-01-06 Thread Petr Vobornik

On 01/06/2012 06:14 AM, Endi Sukma Dewata wrote:

The user details page has been modified to show the password policy
and Kerberos ticket policy that apply to the user. The policy are
displayed as read-only since they may affect other users. Changing
a policy should be done in the respective policy details page.

Ticket #703



NACK

1) you are calling krbtpolicy-show without any user specific information 
so it always get the global policy. It should be call with an user argument.


Minor:
2) Why not call pwpolicy-show --user=user_login instead of getting the 
policy's name from dn?


Combining 1), 2) and user-show will allow to get all necessary 
information for the facet in a single batch at refresh.


Otherwise it seems fine.

--
Petr Vobornik

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