[node-patches] Change in ovirt-node[master]: [RFE] ui: Rework validation and exception handling
fabiand at fedoraproject.org
fabiand at fedoraproject.org
Mon Aug 19 08:13:31 UTC 2013
Fabian Deutsch has uploaded a new change for review.
Change subject: [RFE] ui: Rework validation and exception handling
......................................................................
[RFE] ui: Rework validation and exception handling
Previously the actual widget of a toolkit was responsible to call and
handle the validation stuff. The pros of this approach were that e.g.
handling simple validation errors was easy and could be directly
implemented by the widget. Problems arose with the introduction with
e.g. the ConfirmedEntry (which is a complex ui.Element in the UI
abstraktion layer). With the introduction of these complex ui.Elements
we also wanted - and needed - to pull the validation and exception
handling into the UI abstraction layer. This is necessary because now it
can be controlle din the UI abstraction layer how invalid data shall be
handled.
Change-Id: I4542a2641c6ce6f9f9126db608abca57a4ca453b
Signed-off-by: Fabian Deutsch <fabiand at fedoraproject.org>
---
M src/ovirt/node/app.py
M src/ovirt/node/base.py
M src/ovirt/node/plugins.py
M src/ovirt/node/setup/core/security_page.py
M src/ovirt/node/ui/__init__.py
M src/ovirt/node/ui/urwid_builder.py
6 files changed, 193 insertions(+), 172 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-node refs/changes/77/18277/1
diff --git a/src/ovirt/node/app.py b/src/ovirt/node/app.py
index 8e57b35..1afce97 100644
--- a/src/ovirt/node/app.py
+++ b/src/ovirt/node/app.py
@@ -192,7 +192,7 @@
self.current_plugin()._on_ui_reset()
def call_on_ui_change(d):
- self.current_plugin()._on_ui_change(d)
+ return self.current_plugin()._on_ui_change(d)
def call_on_ui_reload(d):
self.switch_to_plugin(self.current_plugin(), False)
@@ -200,11 +200,9 @@
def call_quit(d):
self.quit()
- def display_exception_as_notice(e):
- self.logger.debug(e, exc_info=True)
- notice = ui.InfoDialog("dialog.notice", "An exception occurred",
- "%s" % e)
- self.show(notice)
+ def do_display_notice(target, is_valid, reason=None):
+ if not is_valid and reason:
+ self.show_exception(reason)
# All known handlers
handlers = {ui.SaveAction: call_on_ui_save,
@@ -213,7 +211,6 @@
ui.ChangeAction: call_on_ui_change,
ui.ReloadAction: call_on_ui_reload,
ui.QuitAction: call_quit,
- ui.DisplayExceptionNotice: display_exception_as_notice
}
for element in elements:
@@ -225,6 +222,7 @@
self.logger.debug("Setting %s.%s to %s" %
(element, cb, action))
cb.callback = action
+
if type(element) is ui.SaveButton:
# http://stackoverflow.com/questions/2731111/
# python-lambdas-and-variable-bindings
@@ -232,10 +230,9 @@
e.enabled(v)
plugin.on_valid.connect(toggle_savebutton_disabled)
- elif type(element) is ui.ConfirmedEntry and element.is_password:
- def set_validity(target, is_secure):
- plugin.on_valid(True if is_secure else False)
- element.on_password_security_change.connect(set_validity)
+ if ui.InputElement in type(element).mro():
+ if not element.on_notice_change.callbacks:
+ element.on_notice_change.connect(do_display_notice)
def populate_with_values(self, ui_container):
"""Take values from model and inject them into the appropriate UI
@@ -286,6 +283,14 @@
raise Exception("Unknown container: %s" % ui_container)
return ui_container
+ def show_exception(self, e):
+ """Show an exception
+ """
+ self.logger.debug(e, exc_info=True)
+ notice = ui.InfoDialog("dialog.notice", "An exception occurred",
+ "%s" % e)
+ self.show(notice)
+
@property
def product(self):
return system.ProductInformation()
diff --git a/src/ovirt/node/base.py b/src/ovirt/node/base.py
index c728213..6dc270a 100644
--- a/src/ovirt/node/base.py
+++ b/src/ovirt/node/base.py
@@ -18,12 +18,11 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
+import logging
"""
Base for all classes
"""
-
-import logging
class Base(object):
@@ -81,6 +80,7 @@
(idx + 1, len(self.callbacks), self, cb))
if cb(self.target, userdata) is False:
self.logger.debug("Breaking callback sequence")
+ break
return self
def connect(self, cb):
diff --git a/src/ovirt/node/plugins.py b/src/ovirt/node/plugins.py
index c94b28c..09e928b 100644
--- a/src/ovirt/node/plugins.py
+++ b/src/ovirt/node/plugins.py
@@ -93,7 +93,6 @@
Errors are propagated back by using Errors/Exceptions.
"""
- validate_changes = True
only_merge_on_valid_changes = True
on_valid = None
@@ -101,7 +100,7 @@
def __init__(self, application):
super(NodePlugin, self).__init__()
self.application = application
- self.__changes = {}
+ self.__changes = Changeset()
self.__invalid_changes = Changeset()
self.widgets = UIElements()
@@ -144,44 +143,6 @@
A dict of validators
"""
raise NotImplementedError()
-
- def validate(self, changes):
- """Test changes against the validators
-
- Args:
- changes: A dict of (path, value) to be checked
-
- Returns:
- True on a valid value or if there is no validator for a path
- Raises:
- InvalidData on any invalid data
- """
- validators = self.validators()
- for path, value in changes.items():
- if path in validators:
- msg = None
- try:
- msg = validators[path](value)
- except exceptions.InvalidData as e:
- msg = e.message
- self.logger.debug("Failed to validate %s: %s" % (path, e))
-
- # True and None are allowed values
- if msg in [True, None]:
- self.__invalid_changes.drop([path])
- else:
- self.logger.debug("Failed to validate " +
- "'%s' with '%s'" % (path, value))
- self.__invalid_changes.update({path: value})
- raise exceptions.InvalidData(msg)
-
- # Is valid if no invalid_changes
- is_valid = self.__invalid_changes.is_empty()
- self.logger.debug("Valid yet? %s (%s)" % (is_valid,
- self.__invalid_changes))
- self.on_valid(is_valid)
-
- return True
def ui_name(self):
"""Returns the UI friendly name for this plugin.
@@ -293,8 +254,68 @@
"check: %s" % model)
is_valid = False
finally:
- self.__changes = {}
+ self.__changes = Changeset()
return is_valid
+
+ def __validate(self, changes):
+ """Test changes against the validators
+
+ Args:
+ changes: A dict of (path, value) to be checked
+
+ Returns:
+ True on a valid value or if there is no validator for a path
+ Raises:
+ InvalidData on any invalid data
+ """
+ validators = self.validators()
+ widgets = self.widgets
+
+ for change in changes.items():
+ path, value = change
+
+ problems = []
+
+ # We assume that the change is invalid, as long as it
+ # isn't validated
+ self.__invalid_changes.update({path: value})
+
+ self.logger.debug("Validation of path %s" % str(change))
+
+ try:
+ if path in validators:
+ problems.append(validators[path](value))
+ except exceptions.InvalidData as e:
+ msg = e.message or str(e)
+ self.logger.debug("Validation failed on validator with: %s"
+ % msg)
+ problems.append(msg)
+
+ try:
+ if path in widgets:
+ problems.append(widgets[path]._validates())
+ except exceptions.InvalidData as e:
+ msg = e.message or str(e)
+ self.logger.debug("Validation failed on widget with: %s"
+ % msg)
+ problems.append(msg)
+
+ problems = [p for p in problems if p not in [True, None]]
+
+ if problems:
+ txt = "\n".join(problems)
+ self.logger.debug("Validation failed with: %s" % problems)
+ raise exceptions.InvalidData(txt)
+
+ # If we reach this line, then it's valid data
+ self.__invalid_changes.drop([path])
+
+ validates = self.__invalid_changes.is_empty()
+ self.logger.debug("Validates? %s (%s)" % (validates,
+ self.__invalid_changes))
+ self.on_valid(validates)
+
+ return validates
def _on_ui_change(self, change):
"""Called when some widget was changed
@@ -307,18 +328,43 @@
self.logger.debug("Passing UI change to callback on_change: %s" %
change)
+
+ is_valid = False
+ msg = None
+ self.__changes.drop(change.keys())
+
try:
- if self.validate_changes:
- self.validate(change)
+ # Run validators
+ self.__validate(change)
+
+ # Run custom validation
self.on_change(change)
- except exceptions.InvalidData:
+
+ is_valid = True
+ self.__changes.update(change)
+
+ except exceptions.InvalidData as e:
+ msg = e.message
+
+ except Exception as e:
self.on_valid(False)
- raise
- self.__changes.update(change)
- self.logger.debug("Sum of all UI changes up to now: %s" %
+ self.application.show_exception(e)
+
+ for path in change.keys():
+ if path in self.widgets:
+ self.logger.debug("Updating %s widgets validity: %s (%s)"
+ % (path, is_valid, msg))
+ self.widgets[path].valid(is_valid)
+ self.widgets[path].notice(msg)
+
+ self.logger.debug("All valid changes: %s" %
self.__changes)
- self.on_valid(self.is_valid_changes())
- return True
+ self.logger.debug("All invalid changes: %s" %
+ self.__invalid_changes)
+
+ self.on_valid(is_valid)
+
+ return is_valid
def _on_ui_save(self):
"""Called when data should be saved
@@ -331,7 +377,7 @@
effective_changes)
try:
- is_valid = self.validate(effective_changes)
+ is_valid = self._on_ui_change(effective_changes)
except exceptions.InvalidData as e:
self.logger.info("Changes to be merged are invalid: %s" %
e.message)
@@ -350,7 +396,7 @@
if successfull_merge:
self.logger.info("Changes were merged successfully")
- self.__changes = {}
+ self.__changes = Changeset()
else:
self.logger.info("Changes were not merged.")
diff --git a/src/ovirt/node/setup/core/security_page.py b/src/ovirt/node/setup/core/security_page.py
index e81d689..84adef7 100644
--- a/src/ovirt/node/setup/core/security_page.py
+++ b/src/ovirt/node/setup/core/security_page.py
@@ -52,7 +52,7 @@
valid.Empty()
return {"strongrng.num_bytes": number_or_empty,
"passwd.admin.password":
- valid.Empty() | valid.Text(min_length=3)
+ valid.Empty() | valid.Text(min_length=5)
}
def ui_content(self):
diff --git a/src/ovirt/node/ui/__init__.py b/src/ovirt/node/ui/__init__.py
index 2fa2ca3..81a4b4b 100644
--- a/src/ovirt/node/ui/__init__.py
+++ b/src/ovirt/node/ui/__init__.py
@@ -20,6 +20,7 @@
# also available at http://www.gnu.org/copyleft/gpl.html.
from ovirt.node import base
from ovirt.node.utils import console, security
+from ovirt.node.exceptions import InvalidData
"""
This contains abstract UI Elements
@@ -50,7 +51,7 @@
super(Element, self).__init__()
self.path = path
self.on_value_change = self.new_signal()
- self.on_exception = self.new_signal()
+ self.on_notice_change = self.new_signal()
self.logger.debug("Initializing %s" % self)
def value(self, value=None):
@@ -61,6 +62,11 @@
def elements(self):
return [self]
+
+ def notice(self, txt):
+ """Protoype command to show a notice associated with this element
+ """
+ self.on_notice_change(txt)
def __repr__(self):
return "<%s path='%s' at %s>" % (self.__class__.__name__, self.path,
@@ -97,18 +103,32 @@
self.on_change.connect(ChangeAction())
def enabled(self, is_enabled=None):
+ """Enable or disable the widget wrt user input
+ """
if is_enabled in [True, False]:
self.on_enabled_change(is_enabled)
self._enabled = is_enabled
return self._enabled
- def valid(self, is_valid):
+ def valid(self, is_valid=None):
+ """Get or set the validity of this element.
+ If a reason is given show it as a notice.
+ """
if is_valid in [True, False]:
self.on_valid_change(is_valid)
self._valid = is_valid
return self._valid
+ def _validates(self):
+ """Validate the value of this widget against the validator
+ This funcion is mainly needed to implement widget specific
+ validation methods
+ """
+ pass
+
def text(self, text=None):
+ """Get or set the textual value
+ """
if text is not None:
self.on_value_change(text)
self._text = text
@@ -232,12 +252,6 @@
pass
-class DisplayExceptionNotice(Action):
- """Display a given Exception as a Notice
- """
- pass
-
-
class Row(ContainerElement):
"""Align elements horizontally in one row
"""
@@ -304,10 +318,11 @@
class ConfirmedEntry(ContainerElement):
- """A container for elements which must be identical"""
+ """A container for elements which must be identical
+ """
on_change = None
- on_password_security_change = None
+ on_valid_change = None
_primary = None
_secondary = None
@@ -316,79 +331,77 @@
is_password = False
min_length = 0
+ _additional_notice = None
def __init__(self, path, label, is_password=False, min_length=0):
self.on_change = self.new_signal()
- self.on_password_security_change = self.new_signal()
+ self.on_valid_change = self.new_signal()
self._changes = {}
children = []
- entry_class = PasswordEntry if is_password else Entry
+ entry_class = PasswordEntry if is_password else Entry
self._primary = entry_class("%s[0]" % path,
label)
self._secondary = entry_class("%s[1]" % path,
"Confirm %s" % label)
+ self._notice = Notice("%s.notice" % path, "")
+ children += [self._primary, self._secondary, self._notice]
for child in [self._primary, self._secondary]:
self._changes[child.path] = ""
- # Remove all callbacks - so nothing is passed to the UI
+ # Remove all callbacks - so we don't triggre on_change and friends
+ # We redirect it to call the validation methods of this widget
child.on_change.clear()
- child.on_change.connect(self.__do_validation)
- children.append(child)
-
- self.on_change.connect(ChangeAction())
+ child.on_change.connect(self.__on_change)
if is_password:
- # If it's a password then also do checks!
- # This work by adding a function which does the pw check
- # and calls a specific signal (on_password_security_change)
self.is_password = is_password
self.min_length = min_length
- self._notice = Notice("%s.notice" % path, "")
- children.append(self._notice)
-
- def do_security_check(target, changes):
- pw, pwc = target.values()
- msg = ""
- is_secure = False
- try:
- msg = security.password_check(pw, pwc,
- min_length=self.min_length)
- is_secure = True
- except ValueError as e:
- msg = e.message
- self._notice.text(msg or "")
- self.on_password_security_change(is_secure)
- self.on_change.connect(do_security_check)
+ self.on_change.connect(ChangeAction())
super(ConfirmedEntry, self).__init__(path, children)
- def __do_validation(self, target, change):
- """Is called when primary or secondary changes
- """
+ def _validates(self):
+ if self.is_password:
+ self.logger.debug("Doing security check")
+ msg = ""
+ pw, pwc = self._values()
+ try:
+ msg = security.password_check(pw, pwc,
+ min_length=self.min_length)
+ except ValueError as e:
+ msg = e.message
+ if msg:
+ raise InvalidData(msg)
+ self._additional_notice = msg
+
+ def __on_change(self, target, change):
+ self._additional_notice = ""
self._changes.update(change)
- if self.valid():
- self.valid(True)
- else:
- self.valid(False)
self.on_change({self.path: self.value()})
- def values(self):
- return [v for _, v in sorted(self._changes.items())]
+ def _values(self):
+ return (self._changes[self._primary.path],
+ self._changes[self._secondary.path])
def value(self, new_value=None):
if new_value is not None:
pass
- return self._changes[self.path + "[0]"] if self.valid() else None
+ return self._values()[0]
def valid(self, is_valid=None):
if is_valid in [True, False]:
self._primary.valid(is_valid)
self._secondary.valid(is_valid)
- is_valid = len(set(self._changes.values())) == 1
- return is_valid
+ self.on_valid_change(is_valid)
+ return self._primary.valid()
+
+ def notice(self, txt=""):
+ self.logger.debug("nooooooootice %s" % self._additional_notice)
+ msg = "\n".join(t for t in [txt, self._additional_notice] if t)
+ self._notice.text(msg)
class Button(InputElement):
@@ -587,7 +600,6 @@
self.selection(selected_item or self.items[0][0])
self.on_activate.connect(ChangeAction())
self.on_activate.connect(SaveAction())
- self.on_exception.connect(DisplayExceptionNotice())
def selection(self, selected=None):
"""Get/Select the given item (key) or multiple items if multi
diff --git a/src/ovirt/node/ui/urwid_builder.py b/src/ovirt/node/ui/urwid_builder.py
index 05900f3..113c160 100644
--- a/src/ovirt/node/ui/urwid_builder.py
+++ b/src/ovirt/node/ui/urwid_builder.py
@@ -18,7 +18,7 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
-from ovirt.node import ui, exceptions, base
+from ovirt.node import ui, base
from ovirt.node.ui import widgets as uw
import os
import urwid
@@ -107,13 +107,7 @@
def on_widget_click_cb(widget, data=None):
change = {ui_button.path: True}
self.logger.debug("Button click: %s" % change)
- try:
- ui_button.on_activate(change)
- except Exception as e:
- if ui_button.on_exception.callbacks:
- ui_button.on_exception(e)
- else:
- raise
+ ui_button.on_activate(change)
urwid.connect_signal(widget, "click", on_widget_click_cb)
@@ -170,6 +164,11 @@
ui_entry.on_valid_change.connect(on_item_valid_change_cb)
+ def on_item_notice_change_cb(w, n):
+ widget.notice = n or ""
+
+ ui_entry.on_notice_change.connect(on_item_notice_change_cb)
+
def on_item_text_change_cb(w, v):
self.logger.debug("Setting entry tooo: %s" % v)
widget.set_text(v)
@@ -179,27 +178,8 @@
def on_widget_value_change(widget, new_value):
self.logger.debug("Entry %s changed, calling callback: '%s'" %
(widget, ui_entry.path))
+ ui_entry.on_change({ui_entry.path: new_value})
- try:
- # FIXME this logic needs to go somewhere else - more generic
- change = {ui_entry.path: new_value}
- ui_entry.on_change(change)
- widget.notice = ""
- widget.valid(True)
-
- except exceptions.InvalidData as e:
- self.logger.error("Invalid data when updating: %s" % e)
- if widget._selectable:
- widget.notice = e.message
- widget.valid(False)
-
- except Exception as e:
- if ui_entry.on_exception.callbacks:
- ui_entry.on_exception(e)
- else:
- raise
-
- #tui.force_redraw()
urwid.connect_signal(widget, 'change', on_widget_value_change)
return widget
@@ -217,13 +197,7 @@
def on_widget_change_cb(widget, data):
ui_options.option(data)
self.logger.debug("Options changed, calling callback: %s" % data)
- try:
- ui_options.on_change({ui_options.path: data})
- except Exception as e:
- if ui_options.on_exception.callbacks:
- ui_options.on_exception(e)
- else:
- raise
+ ui_options.on_change({ui_options.path: data})
urwid.connect_signal(widget, "change", on_widget_change_cb)
@@ -241,15 +215,10 @@
def on_widget_change_cb(widget, data=None):
ui_checkbox.state(data)
self.logger.debug("Checkbox changed, calling callback: %s" % data)
- try:
- ui_checkbox.on_change({ui_checkbox.path: data})
- except Exception as e:
- if ui_checkbox.on_exception.callbacks:
- ui_checkbox.on_exception(e)
- else:
- raise
+ ui_checkbox.on_change({ui_checkbox.path: data})
urwid.connect_signal(widget, "change", on_widget_change_cb)
+
return widget
def _build_progressbar(self, ui_progressbar):
@@ -290,13 +259,7 @@
else:
ui_table.selection(w._key)
- try:
- ui_table.on_change({ui_table.path: w._key})
- except Exception as e:
- if ui_table.on_exception.callbacks:
- ui_table.on_exception(e)
- else:
- raise
+ ui_table.on_change({ui_table.path: w._key})
urwid.connect_signal(widget, "changed", on_change_cb)
@@ -312,15 +275,10 @@
c._key = key
def on_activate_cb(w, data):
- try:
- ui_table.selection(w._table.selection())
- ui_table.on_change({ui_table.path: w._key})
- ui_table.on_activate({ui_table.path: w._key})
- except Exception as e:
- if ui_table.on_exception.callbacks:
- ui_table.on_exception(e)
- else:
- raise
+ ui_table.selection(w._table.selection())
+ ui_table.on_change({ui_table.path: w._key})
+ ui_table.on_activate({ui_table.path: w._key})
+
urwid.connect_signal(c, "activate", on_activate_cb)
return c
--
To view, visit http://gerrit.ovirt.org/18277
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4542a2641c6ce6f9f9126db608abca57a4ca453b
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Fabian Deutsch <fabiand at fedoraproject.org>
More information about the node-patches
mailing list