On 06/18/2015 12:52 PM, Jan Cholasta wrote:
Dne 18.6.2015 v 09:30 Jan Cholasta napsal(a):
Dne 15.6.2015 v 17:29 thierry bordaz napsal(a):
On 06/15/2015 05:00 PM, Simo Sorce wrote:
On Mon, 2015-06-15 at 16:48 +0200, Petr Vobornik wrote:
On 06/09/2015 02:02 PM, Jan Cholasta wrote:
Dne 20.5.2015 v 11:26 Jan Cholasta napsal(a):
Dne 18.5.2015 v 10:33 thierry bordaz napsal(a):
On 05/15/2015 04:44 PM, David Kupka wrote:
Hello Thierry,
thanks for the patch set. Overall functionality of ULC feature
looks
good to
me and is definitely "alpha ready".

I found following issues but don't insist on fixing it right now:

1) When stageuser-activate fails due to already existent
active/deleted user.
DN is show instead of user name that's used in other commands
(user-add,
stageuser-add).
$ ipa user-add tuser --first Test --last User
$ ipa stageuser-add tuser --first Test --last User
$ ipa stageuser-activate tuser
ipa: ERROR: Active user
uid=tuser,cn=users,cn=accounts,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com






already exists
Hi David, Jan,

Thanks you so much for all those tests and feedback. I agree, some
minor
bugs can be fixed separatly from this main patches.

You are right, It should return the user ID not the DN.

2) According to the design there should be '--only-delete' and
'--also-delete'
options for user-find command instead there is '--preserved'
option.
Honza proposed adding virtual boolean attribute 'deleted' to user
entry and
filter on it.
The 'deleted' attribute would be useful also in user-show where
is no
way to
tell if the displayed user is active or deleted. (Except running
with
--all
and looking on the dn).
Yes a bit late to resynch the design.
The final option is 'preserved' for user-find and 'preserve' for
user-del. '--only-delete' or 'also-delete' are old name that I
need to
replace in the design.

About the 'deleted' attribute, do you think adding a DS cos virtual
attribute ?
See the attached patch.
Can someone please review the patch?

3) uidNumber and gidNumber can't be set back to '-1' once set to
other
value.
This would be useful when admin changes its mind and want IPA to
assign them.
IIUC, there should be no validation in cn=staged user container.
All
validation should be done during stageuser-activate.
Yes that comes from user plugin that enforce the number to be >0.
That is a good point giving the ability to reset
uidNumber/gidNumber.
I will check if it is possible, how (give a value or an option to
reset), and also if it would not create other issue.
4) Support for deleted -> stage workflow is still missing. But I'm
unsure if we
agreed to finish it now or later.
Yes thanks
5) Twice deleting user with '--preserve' deletes him permanently.
$ ipa user-add tuser --first Test --last User
$ ipa user-del tuser --preserve
$ ipa user-del tuser --preserve
$ ipa user-find --preserved
------------------------
0 (delete) users matched
------------------------
----------------------------
Number of entries returned 0
----------------------------
Deleting a deleted (preserved) entry, should permanently remove the
entry.
+1, but no-op if default behavior is "preserve"

Now if the second time the preserve option is present, it makes
sense to
not delete it.
+1, should be no-op

BTW: I might be stating the obvious here, but it would be better to
use
one boolean parameter rather than two mutually exclusive flags in
user-del.
I would like an opinion on this as well.

So the proposal is, e.g.,:

Replace:
    ipa user del fbar --preserve
    ipa user del fbar --permanently
with:
    ipa user del fbar --permanently=False
    ipa user del fbar --permanently=True
and
    ipa user del fbar
uses the default behavior(permanently atm.)

I don't think there is a big difference. A boolean is easier for
scripting. 2 options are more descriptive for humans. With a single
boolean, I would be afraid that omitting it would imply False to some
users which is not always the same as "the default behavior" [1].

With Web UI developer hat I would vote for single boolean but as a CLI
user I would like the current options.

Given that Web UI or any other API client should not define CLI, I
would
keep the current options.

my 2c

[1]
http://www.freeipa.org/page/V4/User_Life-Cycle_Management#Delete_User
--
Petr Vobornik

+1 --preserve is 100x better for a human than --permanently=False

I also prefere --preserve for usability of  'user del'.

In addition we have 'user find|show --preserved' to retrieve users that
have been preserved. So it seems to me better that the action that
preserved the user uses the option '--preserve' rather
'--permanently=False'.

It's ridiculous that the CLI taints the RPC API and it should be fixed.

Also on a more nitpicky side, I think the flag should be called
--no-preserve rather than --permanently. There is plenty of commands
(rm, cp, ...) which have --no-preserve as opposite of --preserve.

The attached patch fixes both.

... and it also accidentaly changes the default behavior.

Updated patch attached.


ACK if others are ok with changing --permanently to --no-preserve.

Patch 446 fixed also issue #5, patch 446.1 doesn't fix it. Could be fixed separately.

Attaching patch which addresses this API change in Web UI.
--
Petr Vobornik
From 0da4bd1487b83bfc24d42ffad66e2396e10ebb6a Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Thu, 18 Jun 2015 12:59:05 +0200
Subject: [PATCH] webui: adjust user deleter dialog to new api

In user_del, flags 'permanently' and 'preserve' were replaced with single
bool option 'preserve'

part of: https://fedorahosted.org/freeipa/ticket/3813
---
 install/ui/src/freeipa/user.js     | 22 ++++++++++------------
 install/ui/src/freeipa/widget.js   |  4 ++--
 install/ui/test/data/ipa_init.json |  3 +++
 ipalib/plugins/internal.py         |  3 +++
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/install/ui/src/freeipa/user.js b/install/ui/src/freeipa/user.js
index 09336c3cf2317ef5da6a355275cf5cf0748665a6..bf84ba3ac194e7ff7e172f70475a726ca6607a9c 100644
--- a/install/ui/src/freeipa/user.js
+++ b/install/ui/src/freeipa/user.js
@@ -833,13 +833,13 @@ IPA.user.deleter_dialog = function(spec) {
         });
 
         that.option_radio = IPA.radio_widget({
-            name: 'deletemode',
-            label: 'Delete mode',
+            name: 'preserve',
+            label: '@i18n:objects.user.delete_mode',
             options: [
-                { label: 'default', value: '' },
-                { label: 'delete permanently', value: 'permanently' },
-                { label: 'preserve', value: 'preserve' }
-            ]
+                { label: '@i18n:objects.user.mode_delete', value: 'false' },
+                { label: '@i18n:objects.user.mode_preserve', value: 'true' }
+            ],
+            default_value: 'false'
         });
 
         var html = that.option_layout.create([that.option_radio]);
@@ -849,13 +849,11 @@ IPA.user.deleter_dialog = function(spec) {
 
     that.create_command = function() {
         var batch = that.search_deleter_dialog_create_command();
-        var option = that.option_radio.get_value()[0];
+        var preserve = that.option_radio.get_value()[0];
 
-        if (option) {
-            for (var i=0; i<batch.commands.length; i++) {
-                var command = batch.commands[i];
-                command.set_option(option, true);
-            }
+        for (var i=0; i<batch.commands.length; i++) {
+            var command = batch.commands[i];
+            command.set_option('preserve', preserve);
         }
 
         return batch;
diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index e985cff227b39221da262b3a0c509c0e07ce606a..434a4b1bbe2ce1e71914f8543410de9212b389fe 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1425,8 +1425,8 @@ IPA.option_widget_base = function(spec, that) {
 
     // classic properties
     that.name = spec.name;
-    that.label = spec.label;
-    that.title = spec.title;
+    that.label = text.get(spec.label);
+    that.title = text.get(spec.title);
     that.sort = spec.sort === undefined ? false : spec.sort;
     that.value_changed = that.value_changed || IPA.observer();
 
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index e5d306cd0cc9d39e05ebce0c45538a859eb89992..1290db2c430354afdd79024dbda8752330d11aaf 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -578,11 +578,14 @@
                             "account_status": "Account Status",
                             "activeuser_label": "Active users",
                             "contact": "Contact Settings",
+                            "delete_mode": "Delete mode",
                             "employee": "Employee Information",
                             "error_changing_status": "Error changing account status",
                             "krbpasswordexpiration": "Password expiration",
                             "mailing": "Mailing Address",
                             "misc": "Misc. Information",
+                            "mode_delete": "delete",
+                            "mode_preserve": "preserve",
                             "noprivate": "No private group",
                             "status_confirmation": "Are you sure you want to ${action} the user?<br/>The change will take effect immediately.",
                             "status_link": "Click to ${action}",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index ff096616d5e9bdc6737ee2d8b087fbded18ca2af..270a228b2713ac6b17cebb5f23158bc631d5e42d 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -724,11 +724,14 @@ class i18n_messages(Command):
                 "account_status": _("Account Status"),
                 "activeuser_label": _("Active users"),
                 "contact": _("Contact Settings"),
+                "delete_mode": _("Delete mode"),
                 "employee": _("Employee Information"),
                 "error_changing_status": _("Error changing account status"),
                 "krbpasswordexpiration": _("Password expiration"),
                 "mailing": _("Mailing Address"),
                 "misc": _("Misc. Information"),
+                "mode_delete": _("delete"),
+                "mode_preserve": _("preserve"),
                 "noprivate": _("No private group"),
                 "status_confirmation": _("Are you sure you want to ${action} the user?<br/>The change will take effect immediately."),
                 "status_link": _("Click to ${action}"),
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to