Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On 30.6.2014 16:23, Endi Sukma Dewata wrote: On 6/27/2014 3:54 AM, Petr Vobornik wrote: On 27.6.2014 09:48, Fraser Tweedale wrote: On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote: Patch 618 fixes a bug. Patches 680 and 681 were implemented along with it. They address pspacek's usability rant :). [PATCH] 680 webui: show notification instead of modal dialog on validation error [PATCH] 681 webui: fix required error notification in multivalued widget [PATCH] 682 webui: focus invalid widget on validation error -- Petr Vobornik ACK on 680 and 682. On 681: diff makes sense; I'm not 100% sure my testing has covered cases that were previously failing. ACK if you're confident, otherwise could you provide steps to verify? You need to find a required multivalued field. One is in Identity/Realm Domains. Delete all values and hit update. It's little bit related to ticket: https://fedorahosted.org/freeipa/ticket/4057 Also when verifying validators in multivalued field, it's good to check if errors are provided only for invalid values, etc.. good test field is in DNS/DNS Zones/some zone/Settings/ there is Allow query field which accepts network address, any or none. ACK. pushed to master: * 93de5db39e5b2e5991c32a57958cedb0f8b41848 webui: show notification instead of modal dialog on validation error * c693b28babf97d22c14d37e024d551b583c4327f webui: fix required error notification in multivalued widget * 99c5f0511f697cc54a9de7994c3e6999c6fd119f webui: focus invalid widget on validation error This should be sufficient to close #4057. But just wondering, the Realm Domains page right now is implemented as a details page with a multi-valued widget. Would it make more sense to be a list page instead? The realmdomains-mod CLI is kind of unusual too with the --add/del-domain parameters. Why not use realmdomain-add/del commands? Are there other commands implemented in this fashion? Wrt CLI: Depends how you look at it and if/how much should our CLI/API reflect data structure. If you say, we have an object which contains/manages the realm domains used by IPA server, then it's implemented as in every other similar object with multivalued attribute. But it's also a singleton therefore the closest similar thing is a configuration. There could also be an argument that this functionality could have been added to config itself. If you say that each realm domain is an object or if you don't want to reflect the data structure and just focus on API then it's not implemented the best way. Wrt Web UI: Makes sense. The list UI would fit better (until the realmdomains object is extended by additional attr). -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On 7/1/2014 3:47 AM, Petr Vobornik wrote: This should be sufficient to close #4057. But just wondering, the Realm Domains page right now is implemented as a details page with a multi-valued widget. Would it make more sense to be a list page instead? The realmdomains-mod CLI is kind of unusual too with the --add/del-domain parameters. Why not use realmdomain-add/del commands? Are there other commands implemented in this fashion? Wrt CLI: Depends how you look at it and if/how much should our CLI/API reflect data structure. If you say, we have an object which contains/manages the realm domains used by IPA server, then it's implemented as in every other similar object with multivalued attribute. But it's also a singleton therefore the closest similar thing is a configuration. There could also be an argument that this functionality could have been added to config itself. If you say that each realm domain is an object or if you don't want to reflect the data structure and just focus on API then it's not implemented the best way. Wrt Web UI: Makes sense. The list UI would fit better (until the realmdomains object is extended by additional attr). I think we should design CLI/UI from user's perspective because it's an interface after all, regardless of how it's stored on the server. Right now the design seems to be based on assumption that there won't be many realm domains (e.g. no need to search, no need to page), and there's no other attributes required for each realm domain (e.g. no description, no owner, etc.). But as soon as the assumption is no longer correct the current interface is no longer adequate. Treating realm domains as separate objects I think is more future-proof. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On 30.6.2014 06:41, Fraser Tweedale wrote: On Fri, Jun 27, 2014 at 10:54:39AM +0200, Petr Vobornik wrote: On 27.6.2014 09:48, Fraser Tweedale wrote: On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote: Patch 618 fixes a bug. Patches 680 and 681 were implemented along with it. They address pspacek's usability rant :). [PATCH] 680 webui: show notification instead of modal dialog on validation error [PATCH] 681 webui: fix required error notification in multivalued widget [PATCH] 682 webui: focus invalid widget on validation error -- Petr Vobornik ACK on 680 and 682. On 681: diff makes sense; I'm not 100% sure my testing has covered cases that were previously failing. ACK if you're confident, otherwise could you provide steps to verify? You need to find a required multivalued field. One is in Identity/Realm Domains. Delete all values and hit update. It's little bit related to ticket: https://fedorahosted.org/freeipa/ticket/4057 Also when verifying validators in multivalued field, it's good to check if errors are provided only for invalid values, etc.. good test field is in DNS/DNS Zones/some zone/Settings/ there is Allow query field which accepts network address, any or none. I can't discern any difference in behaviour of the multi-value fields from before and after your patch. I tried with the Identity/Realm Domains as suggested. Maybe I missed something. Has someone else been able to verify? Expected result is: http://pvoborni.fedorapeople.org/images/multivalued-w-required-msg.png Without the patch, there is no red highlight and no error message. Fraser Cheers, Fraser -- Petr Vobornik -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On 6/27/2014 3:54 AM, Petr Vobornik wrote: On 27.6.2014 09:48, Fraser Tweedale wrote: On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote: Patch 618 fixes a bug. Patches 680 and 681 were implemented along with it. They address pspacek's usability rant :). [PATCH] 680 webui: show notification instead of modal dialog on validation error [PATCH] 681 webui: fix required error notification in multivalued widget [PATCH] 682 webui: focus invalid widget on validation error -- Petr Vobornik ACK on 680 and 682. On 681: diff makes sense; I'm not 100% sure my testing has covered cases that were previously failing. ACK if you're confident, otherwise could you provide steps to verify? You need to find a required multivalued field. One is in Identity/Realm Domains. Delete all values and hit update. It's little bit related to ticket: https://fedorahosted.org/freeipa/ticket/4057 Also when verifying validators in multivalued field, it's good to check if errors are provided only for invalid values, etc.. good test field is in DNS/DNS Zones/some zone/Settings/ there is Allow query field which accepts network address, any or none. ACK. This should be sufficient to close #4057. But just wondering, the Realm Domains page right now is implemented as a details page with a multi-valued widget. Would it make more sense to be a list page instead? The realmdomains-mod CLI is kind of unusual too with the --add/del-domain parameters. Why not use realmdomain-add/del commands? Are there other commands implemented in this fashion? -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On Fri, Jun 27, 2014 at 10:54:39AM +0200, Petr Vobornik wrote: On 27.6.2014 09:48, Fraser Tweedale wrote: On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote: Patch 618 fixes a bug. Patches 680 and 681 were implemented along with it. They address pspacek's usability rant :). [PATCH] 680 webui: show notification instead of modal dialog on validation error [PATCH] 681 webui: fix required error notification in multivalued widget [PATCH] 682 webui: focus invalid widget on validation error -- Petr Vobornik ACK on 680 and 682. On 681: diff makes sense; I'm not 100% sure my testing has covered cases that were previously failing. ACK if you're confident, otherwise could you provide steps to verify? You need to find a required multivalued field. One is in Identity/Realm Domains. Delete all values and hit update. It's little bit related to ticket: https://fedorahosted.org/freeipa/ticket/4057 Also when verifying validators in multivalued field, it's good to check if errors are provided only for invalid values, etc.. good test field is in DNS/DNS Zones/some zone/Settings/ there is Allow query field which accepts network address, any or none. I can't discern any difference in behaviour of the multi-value fields from before and after your patch. I tried with the Identity/Realm Domains as suggested. Maybe I missed something. Has someone else been able to verify? Fraser Cheers, Fraser -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote:
Patch 618 fixes a bug.
Patches 680 and 681 were implemented along with it. They address pspacek's
usability rant :).
[PATCH] 680 webui: show notification instead of modal dialog on validation
error
[PATCH] 681 webui: fix required error notification in multivalued widget
[PATCH] 682 webui: focus invalid widget on validation error
--
Petr Vobornik
ACK on 680 and 682.
On 681: diff makes sense; I'm not 100% sure my testing has covered
cases that were previously failing. ACK if you're confident,
otherwise could you provide steps to verify?
Cheers,
Fraser
From 5c3fcd240d61f378a8bb7e8cd4c2129cd11930df Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 25 Jun 2014 15:17:26 +0200
Subject: [PATCH] webui: focus invalid widget on validation error
---
install/ui/src/freeipa/add.js | 7 +--
install/ui/src/freeipa/details.js | 4 +++-
install/ui/src/freeipa/widget.js | 22 ++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/install/ui/src/freeipa/add.js b/install/ui/src/freeipa/add.js
index
a4b5d3649d2df2933cb7eb7c8a24a18b7b789f50..78f3890ad2320cbc3afd5cb9ae1e4ae2359d8023
100644
--- a/install/ui/src/freeipa/add.js
+++ b/install/ui/src/freeipa/add.js
@@ -20,7 +20,7 @@
*/
define(['./ipa', './jquery', './navigation', './rpc', './text', './field',
'./widget', './dialog'],
- function(IPA, $, navigation, rpc, text) {
+ function(IPA, $, navigation, rpc, text, field_mod, widget_mod) {
/**
* Entity adder dialog
@@ -219,7 +219,10 @@ IPA.entity_adder_dialog = function(spec) {
*/
that.add = function(on_success, on_error) {
-if (!that.validate()) return;
+if (!that.validate()) {
+widget_mod.focus_invalid(that);
+return;
+}
var record = {};
that.save(record);
diff --git a/install/ui/src/freeipa/details.js
b/install/ui/src/freeipa/details.js
index
ed057e98c14e8ee72e5f1596ed24599eb27abfa5..7aa4c0ef6541900d6fa5b14b16ec964b50349015
100644
--- a/install/ui/src/freeipa/details.js
+++ b/install/ui/src/freeipa/details.js
@@ -31,9 +31,10 @@ define([
'./rpc',
'./spec_util',
'./text',
+'./widget',
'./facet',
'./add'],
-function(lang, builder, IPA, $, phases, reg, rpc, su, text) {
+function(lang, builder, IPA, $, phases, reg, rpc, su, text, widget_mod) {
/**
* Details module
@@ -1436,6 +1437,7 @@ exp.update_action = IPA.update_action = function(spec) {
if (!facet.validate()) {
facet.show_validation_error();
+widget_mod.focus_invalid(facet);
return;
}
diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
3d3f427af43e429124f5e19a2604bd443a6e39c8..ab86c72d1abfae87984fa2212040a3e6c63f2f31
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -5747,6 +5747,28 @@ exp.activity_widget = IPA.activity_widget =
function(spec) {
};
/**
+ * Find and focus first focusable invalid widget
+ * @member widget
+ * @param {IPA.widget|facet.facet} widget Widget container
+ * @return {boolean} A widget was focused
+ */
+exp.focus_invalid = function(widget) {
+
+var widgets = widget.widgets.widgets;
+var focused = false;
+for (var i=0, l=widgets.length; il; i++) {
+var w = widgets.values[i];
+if (w.valid === false w.focus_input) {
+w.focus_input();
+focused = true;
+}
+else if (w.widgets) focused = exp.focus_invalid(w);
+if (focused) break;
+}
+return focused;
+};
+
+/**
* pre_op operations for widgets
* - sets facet and entity if present in context
* @member widget
--
1.9.0
From 431dad35cbcb02351786fd75bd34aa86781d42fb Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 25 Jun 2014 14:50:16 +0200
Subject: [PATCH] webui: fix required error notification in multivalued widget
---
install/ui/src/freeipa/widget.js | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
a49c6282b1977008e80a9c02754df44de31c8b02..3d3f427af43e429124f5e19a2604bd443a6e39c8
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -909,7 +909,7 @@ IPA.multivalued_widget = function(spec) {
var old = that.valid;
that.valid = result.valid;
-if (!result.valid result.errors) {
+if (!result.valid result.results) {
var offset = 0;
for (var i=0; ithat.rows.length; i++) {
@@ -928,10 +928,9 @@ IPA.multivalued_widget = function(spec) {
var error_link = that.get_error_link();
Re: [Freeipa-devel] [PATCH] 680-682 webui: validation reporting improvements
On 27.6.2014 09:48, Fraser Tweedale wrote: On Wed, Jun 25, 2014 at 06:58:52PM +0200, Petr Vobornik wrote: Patch 618 fixes a bug. Patches 680 and 681 were implemented along with it. They address pspacek's usability rant :). [PATCH] 680 webui: show notification instead of modal dialog on validation error [PATCH] 681 webui: fix required error notification in multivalued widget [PATCH] 682 webui: focus invalid widget on validation error -- Petr Vobornik ACK on 680 and 682. On 681: diff makes sense; I'm not 100% sure my testing has covered cases that were previously failing. ACK if you're confident, otherwise could you provide steps to verify? You need to find a required multivalued field. One is in Identity/Realm Domains. Delete all values and hit update. It's little bit related to ticket: https://fedorahosted.org/freeipa/ticket/4057 Also when verifying validators in multivalued field, it's good to check if errors are provided only for invalid values, etc.. good test field is in DNS/DNS Zones/some zone/Settings/ there is Allow query field which accepts network address, any or none. Cheers, Fraser -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
