From 9cac6602821383dc2e03607066c0ea7ec7d01af7 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 27 Jan 2015 19:47:51 +0200 Subject: [PATCH] Add methods for getting invalid fields from a FieldGroup (#13775) * Method for retrieving all failing fields exceptions from a CommitException * Methods for handling commit errors in Grid (#16515) * Show editor row validation errors only on the fields (#16509) Change-Id: Iabef662579e4ccae3803a513205e46542c41cce2 --- .../vaadin/data/fieldgroup/FieldGroup.java | 107 +++++++++++--- server/src/com/vaadin/ui/Grid.java | 138 ++++++++++++++++++ .../component/fieldgroup/FieldGroupTest.java | 94 ++++++++++++ .../basicfeatures/server/GridEditorTest.java | 7 +- .../tests/fieldgroup/AbstractBasicCrud.java | 64 +++++++- .../BasicCrudGridEditorRowTest.java | 40 ++++- 6 files changed, 424 insertions(+), 26 deletions(-) diff --git a/server/src/com/vaadin/data/fieldgroup/FieldGroup.java b/server/src/com/vaadin/data/fieldgroup/FieldGroup.java index 069cb2e153..8f1bf8b40a 100644 --- a/server/src/com/vaadin/data/fieldgroup/FieldGroup.java +++ b/server/src/com/vaadin/data/fieldgroup/FieldGroup.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import com.vaadin.data.Item; import com.vaadin.data.Property; @@ -465,46 +466,74 @@ public class FieldGroup implements Serializable { try { firePreCommitEvent(); - List invalidValueExceptions = commitFields(); + Map, InvalidValueException> invalidValueExceptions = commitFields(); if (invalidValueExceptions.isEmpty()) { firePostCommitEvent(); commitTransactions(); } else { - throwInvalidValueException(invalidValueExceptions); + throw new FieldGroupInvalidValueException( + invalidValueExceptions); } } catch (Exception e) { rollbackTransactions(); - - throw new CommitException("Commit failed", e); + throw new CommitException("Commit failed", this, e); } } - private List commitFields() { - List invalidValueExceptions = new ArrayList(); + /** + * Tries to commit all bound fields one by one and gathers any validation + * exceptions in a map, which is returned to the caller + * + * @return a propertyId to validation exception map which is empty if all + * commits succeeded + */ + private Map, InvalidValueException> commitFields() { + Map, InvalidValueException> invalidValueExceptions = new HashMap, InvalidValueException>(); for (Field f : fieldToPropertyId.keySet()) { try { f.commit(); } catch (InvalidValueException e) { - invalidValueExceptions.add(e); + invalidValueExceptions.put(f, e); } } return invalidValueExceptions; } - private void throwInvalidValueException( - List invalidValueExceptions) { - if (invalidValueExceptions.size() == 1) { - throw invalidValueExceptions.get(0); - } else { - InvalidValueException[] causes = invalidValueExceptions - .toArray(new InvalidValueException[invalidValueExceptions - .size()]); + /** + * Exception which wraps InvalidValueExceptions from all invalid fields in a + * FieldGroup + * + * @since 7.4 + */ + public static class FieldGroupInvalidValueException extends + InvalidValueException { + private Map, InvalidValueException> invalidValueExceptions; - throw new InvalidValueException(null, causes); + /** + * Constructs a new exception with the specified validation exceptions. + * + * @param invalidValueExceptions + * a property id to exception map + */ + public FieldGroupInvalidValueException( + Map, InvalidValueException> invalidValueExceptions) { + super(null, invalidValueExceptions.values().toArray( + new InvalidValueException[invalidValueExceptions.size()])); + this.invalidValueExceptions = invalidValueExceptions; + } + + /** + * Returns a map containing fields which failed validation and the + * exceptions the corresponding validators threw. + * + * @return a map with all the invalid value exceptions + */ + public Map, InvalidValueException> getInvalidFields() { + return invalidValueExceptions; } } @@ -1006,26 +1035,64 @@ public class FieldGroup implements Serializable { return fieldName.toLowerCase().replace("_", ""); } + /** + * Exception thrown by a FieldGroup when the commit operation fails. + * + * Provides information about validation errors through + * {@link #getInvalidFields()} if the cause of the failure is that all bound + * fields did not pass validation + * + */ public static class CommitException extends Exception { + private FieldGroup fieldGroup; + public CommitException() { super(); - // TODO Auto-generated constructor stub + } + + public CommitException(String message, FieldGroup fieldGroup, + Throwable cause) { + super(message, cause); + this.fieldGroup = fieldGroup; } public CommitException(String message, Throwable cause) { super(message, cause); - // TODO Auto-generated constructor stub } public CommitException(String message) { super(message); - // TODO Auto-generated constructor stub } public CommitException(Throwable cause) { super(cause); - // TODO Auto-generated constructor stub + } + + /** + * Returns a map containing the fields which failed validation and the + * exceptions the corresponding validators threw. + * + * @since 7.4 + * @return a map with all the invalid value exceptions. Can be empty but + * not null + */ + public Map, InvalidValueException> getInvalidFields() { + if (getCause() instanceof FieldGroupInvalidValueException) { + return ((FieldGroupInvalidValueException) getCause()) + .getInvalidFields(); + } + return new HashMap, InvalidValueException>(); + } + + /** + * Returns the field group where the exception occurred + * + * @since 7.4 + * @return the field group + */ + public FieldGroup getFieldGroup() { + return fieldGroup; } } diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java index 3c0afbc484..0ba771b283 100644 --- a/server/src/com/vaadin/ui/Grid.java +++ b/server/src/com/vaadin/ui/Grid.java @@ -48,6 +48,7 @@ import com.vaadin.data.Item; import com.vaadin.data.Property; import com.vaadin.data.RpcDataProviderExtension; import com.vaadin.data.RpcDataProviderExtension.DataProviderKeyMapper; +import com.vaadin.data.Validator.InvalidValueException; import com.vaadin.data.fieldgroup.DefaultFieldGroupFieldFactory; import com.vaadin.data.fieldgroup.FieldGroup; import com.vaadin.data.fieldgroup.FieldGroup.BindException; @@ -90,6 +91,7 @@ import com.vaadin.shared.ui.grid.GridStaticSectionState.RowState; import com.vaadin.shared.ui.grid.HeightMode; import com.vaadin.shared.ui.grid.ScrollDestination; import com.vaadin.shared.util.SharedUtil; +import com.vaadin.ui.Notification.Type; import com.vaadin.ui.renderer.ObjectRenderer; import com.vaadin.ui.renderer.Renderer; import com.vaadin.util.ReflectTools; @@ -238,6 +240,102 @@ public class Grid extends AbstractComponent implements SelectionNotifier, } } + /** + * Error handler for the editor + */ + public interface EditorErrorHandler extends Serializable { + + /** + * Called when an exception occurs while the editor row is being saved + * + * @param event + * An event providing more information about the error + */ + void commitError(CommitErrorEvent event); + } + + /** + * An event which is fired when saving the editor fails + */ + public static class CommitErrorEvent extends Component.Event { + + private CommitException cause; + + public CommitErrorEvent(Grid grid, CommitException cause) { + super(grid); + this.cause = cause; + } + + /** + * Retrieves the cause of the failure + * + * @return the cause of the failure + */ + public CommitException getCause() { + return cause; + } + + @Override + public Grid getComponent() { + return (Grid) super.getComponent(); + } + + /** + * Checks if validation exceptions caused this error + * + * @return true if the problem was caused by a validation error + */ + public boolean isValidationFailure() { + return cause.getCause() instanceof InvalidValueException; + } + + } + + /** + * Default error handler for the editor + * + */ + public class DefaultEditorErrorHandler implements EditorErrorHandler { + + @Override + public void commitError(CommitErrorEvent event) { + Map, InvalidValueException> invalidFields = event + .getCause().getInvalidFields(); + + if (!invalidFields.isEmpty()) { + // Validation error, show first failure as + // ": " + FieldGroup fieldGroup = event.getCause().getFieldGroup(); + Object propertyId = getFirstPropertyId(fieldGroup, + invalidFields.keySet()); + Field field = fieldGroup.getField(propertyId); + String caption = getColumn(propertyId).getHeaderCaption(); + // TODO This should be shown in the editor component once + // there is a place for that. Optionally, all errors should be + // shown + Notification.show(caption + ": " + + invalidFields.get(field).getLocalizedMessage(), + Type.ERROR_MESSAGE); + + } else { + com.vaadin.server.ErrorEvent.findErrorHandler(Grid.this).error( + new ConnectorErrorEvent(Grid.this, event.getCause())); + } + } + + private Object getFirstPropertyId(FieldGroup fieldGroup, + Set> keySet) { + for (Column c : getColumns()) { + Object propertyId = c.getPropertyId(); + Field f = fieldGroup.getField(propertyId); + if (keySet.contains(f)) { + return propertyId; + } + } + return null; + } + } + /** * Selection modes representing built-in {@link SelectionModel * SelectionModels} that come bundled with {@link Grid}. @@ -2622,6 +2720,8 @@ public class Grid extends AbstractComponent implements SelectionNotifier, */ private boolean defaultContainer = true; + private EditorErrorHandler editorErrorHandler = new DefaultEditorErrorHandler(); + private static final Method SELECTION_CHANGE_METHOD = ReflectTools .findMethod(SelectionListener.class, "select", SelectionEvent.class); @@ -2861,6 +2961,15 @@ public class Grid extends AbstractComponent implements SelectionNotifier, try { saveEditor(); success = true; + } catch (CommitException e) { + try { + getEditorErrorHandler().commitError( + new CommitErrorEvent(Grid.this, e)); + } catch (Exception ee) { + // A badly written error handler can throw an exception, + // which would lock up the Grid + handleError(ee); + } } catch (Exception e) { handleError(e); } @@ -4713,6 +4822,35 @@ public class Grid extends AbstractComponent implements SelectionNotifier, editorFieldGroup.setFieldFactory(fieldFactory); } + /** + * Sets the error handler for the editor. + * + * The error handler is called whenever there is an exception in the editor. + * + * @param editorErrorHandler + * The editor error handler to use + * @throws IllegalArgumentException + * if the error handler is null + */ + public void setEditorErrorHandler(EditorErrorHandler editorErrorHandler) + throws IllegalArgumentException { + if (editorErrorHandler == null) { + throw new IllegalArgumentException( + "The error handler cannot be null"); + } + this.editorErrorHandler = editorErrorHandler; + } + + /** + * Gets the error handler used for the editor + * + * @see #setErrorHandler(com.vaadin.server.ErrorHandler) + * @return the editor error handler, never null + */ + public EditorErrorHandler getEditorErrorHandler() { + return editorErrorHandler; + } + @Override public void addItemClickListener(ItemClickListener listener) { addListener(GridConstants.ITEM_CLICK_EVENT_ID, ItemClickEvent.class, diff --git a/server/tests/src/com/vaadin/tests/server/component/fieldgroup/FieldGroupTest.java b/server/tests/src/com/vaadin/tests/server/component/fieldgroup/FieldGroupTest.java index da86c83771..d77a2e190b 100644 --- a/server/tests/src/com/vaadin/tests/server/component/fieldgroup/FieldGroupTest.java +++ b/server/tests/src/com/vaadin/tests/server/component/fieldgroup/FieldGroupTest.java @@ -15,10 +15,23 @@ */ package com.vaadin.tests.server.component.fieldgroup; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; + import org.junit.Assert; import org.junit.Test; +import com.vaadin.data.Item; +import com.vaadin.data.Property; +import com.vaadin.data.Validator.InvalidValueException; import com.vaadin.data.fieldgroup.FieldGroup; +import com.vaadin.data.fieldgroup.FieldGroup.CommitException; +import com.vaadin.data.util.AbstractProperty; +import com.vaadin.ui.Field; import com.vaadin.ui.TextField; /** @@ -52,4 +65,85 @@ public class FieldGroupTest { Assert.assertFalse("Field is not writable", field.isReadOnly()); } + + @Test + public void commit_validationFailed_allValidationFailuresAvailable() + throws CommitException { + FieldGroup fieldGroup = new FieldGroup(); + + fieldGroup.setItemDataSource(new TestItem()); + + TextField field1 = new TextField(); + field1.setRequired(true); + fieldGroup.bind(field1, "prop1"); + + TextField field2 = new TextField(); + field2.setRequired(true); + fieldGroup.bind(field2, "prop2"); + + Set set = new HashSet(Arrays.asList(field1, + field2)); + + try { + fieldGroup.commit(); + Assert.fail("No commit exception is thrown"); + } catch (CommitException exception) { + Map, ? extends InvalidValueException> invalidFields = exception + .getInvalidFields(); + for (Entry, ? extends InvalidValueException> entry : invalidFields + .entrySet()) { + set.remove(entry.getKey()); + } + Assert.assertEquals( + "Some fields are not found in the invalid fields map", 0, + set.size()); + Assert.assertEquals( + "Invalid value exception should be thrown for each field", + 2, invalidFields.size()); + } + } + + private static class TestItem implements Item { + + @Override + public Property getItemProperty(Object id) { + return new StringProperty(); + } + + @Override + public Collection getItemPropertyIds() { + return Arrays.asList("prop1", "prop2"); + } + + @Override + public boolean addItemProperty(Object id, Property property) + throws UnsupportedOperationException { + return false; + } + + @Override + public boolean removeItemProperty(Object id) + throws UnsupportedOperationException { + return false; + } + + } + + private static class StringProperty extends AbstractProperty { + + @Override + public String getValue() { + return null; + } + + @Override + public void setValue(String newValue) throws Property.ReadOnlyException { + } + + @Override + public Class getType() { + return String.class; + } + } + } diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorTest.java index 0218fffe61..c6eb8d042b 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorTest.java +++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorTest.java @@ -187,9 +187,10 @@ public class GridEditorTest extends GridBasicFeaturesTest { intField.clear(); intField.sendKeys("banana phone"); editor.save(); - assertTrue( - "No exception on invalid value.", - logContainsText("Exception occured, com.vaadin.data.fieldgroup.FieldGroup$CommitException: Commit failed")); + WebElement n = $(NotificationElement.class).first(); + assertEquals("Column 7: Could not convert value to Integer", + n.getText()); + n.click(); editor.cancel(); selectMenuPath(EDIT_ITEM_100); diff --git a/uitest/src/com/vaadin/tests/fieldgroup/AbstractBasicCrud.java b/uitest/src/com/vaadin/tests/fieldgroup/AbstractBasicCrud.java index 30cc5676bb..c88047e414 100644 --- a/uitest/src/com/vaadin/tests/fieldgroup/AbstractBasicCrud.java +++ b/uitest/src/com/vaadin/tests/fieldgroup/AbstractBasicCrud.java @@ -16,16 +16,20 @@ package com.vaadin.tests.fieldgroup; import java.util.Iterator; +import java.util.Map; import com.vaadin.annotations.Theme; import com.vaadin.data.Property.ValueChangeEvent; import com.vaadin.data.Property.ValueChangeListener; +import com.vaadin.data.Validator.InvalidValueException; import com.vaadin.data.fieldgroup.BeanFieldGroup; import com.vaadin.data.fieldgroup.FieldGroup.CommitException; import com.vaadin.data.fieldgroup.PropertyId; import com.vaadin.data.util.BeanItem; import com.vaadin.data.util.BeanItemContainer; +import com.vaadin.data.validator.IntegerRangeValidator; import com.vaadin.server.VaadinRequest; +import com.vaadin.shared.ui.label.ContentMode; import com.vaadin.shared.util.SharedUtil; import com.vaadin.tests.components.AbstractTestUIWithLog; import com.vaadin.ui.Alignment; @@ -34,10 +38,15 @@ import com.vaadin.ui.Button.ClickEvent; import com.vaadin.ui.Button.ClickListener; import com.vaadin.ui.CheckBox; import com.vaadin.ui.ComboBox; +import com.vaadin.ui.Component; import com.vaadin.ui.Field; import com.vaadin.ui.GridLayout; import com.vaadin.ui.HorizontalLayout; +import com.vaadin.ui.Label; +import com.vaadin.ui.Notification; +import com.vaadin.ui.Notification.Type; import com.vaadin.ui.TextField; +import com.vaadin.ui.themes.ValoTheme; @Theme("valo") public abstract class AbstractBasicCrud extends AbstractTestUIWithLog { @@ -100,6 +109,7 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog { private TextField birthDate = new TextField("Birth date"); private TextField age = new TextField("Age"); private CheckBox alive = new CheckBox("Alive"); + private Label errorLabel = new Label((String) null, ContentMode.HTML); @PropertyId("address.streetAddress") private TextField address_streetAddress = new TextField( @@ -120,17 +130,51 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog { address_country.setNullRepresentation(""); birthDate.setNullRepresentation(""); + age.addValidator(new IntegerRangeValidator( + "Must be between 0 and 100", 0, 100)); + setDefaultComponentAlignment(Alignment.MIDDLE_LEFT); addComponents(firstName, lastName, gender, birthDate, age, alive, address_streetAddress, address_postalCode, address_city, address_country); + errorLabel.addStyleName(ValoTheme.LABEL_COLORED); + setRows(3); + addComponent(errorLabel, 0, 2, getColumns() - 1, 2); + HorizontalLayout hl = new HorizontalLayout(save, cancel); hl.setSpacing(true); addComponent(hl); } + @Override + protected void handleCommitException(CommitException e) { + String message = ""; + // Produce error message in the order in which the fields are in the + // layout + for (Component c : this) { + if (!(c instanceof Field)) { + continue; + } + Field f = (Field) c; + Map, InvalidValueException> exceptions = e + .getInvalidFields(); + if (exceptions.containsKey(f)) { + message += f.getCaption() + ": " + + exceptions.get(f).getLocalizedMessage() + + "
\n"; + } + } + + errorLabel.setValue(message); + } + + @Override + protected void discard() { + super.discard(); + errorLabel.setValue(null); + } } protected abstract void deselectAll(); @@ -153,6 +197,7 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog { fieldGroup.commit(); log("Saved " + fieldGroup.getItemDataSource()); } catch (CommitException e) { + handleCommitException(e); log("Commit failed: " + e.getMessage()); } } @@ -161,11 +206,28 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog { @Override public void buttonClick(ClickEvent event) { log("Discarded " + fieldGroup.getItemDataSource()); - deselectAll(); + discard(); } }); } + protected void discard() { + deselectAll(); + } + + protected void handleCommitException(CommitException e) { + String message = ""; + for (Object propertyId : e.getInvalidFields().keySet()) { + Field f = e.getFieldGroup().getField(propertyId); + message += f.getCaption() + ": " + + e.getInvalidFields().get(propertyId); + } + + if (!message.isEmpty()) { + Notification.show(message, Type.ERROR_MESSAGE); + } + } + public void edit(BeanItem item) { fieldGroup.setItemDataSource(item); } diff --git a/uitest/src/com/vaadin/tests/fieldgroup/BasicCrudGridEditorRowTest.java b/uitest/src/com/vaadin/tests/fieldgroup/BasicCrudGridEditorRowTest.java index 3ccd547417..bdf8603c48 100644 --- a/uitest/src/com/vaadin/tests/fieldgroup/BasicCrudGridEditorRowTest.java +++ b/uitest/src/com/vaadin/tests/fieldgroup/BasicCrudGridEditorRowTest.java @@ -15,25 +15,61 @@ */ package com.vaadin.tests.fieldgroup; +import org.junit.Assert; +import org.junit.Before; import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.Keys; +import org.openqa.selenium.WebElement; import org.openqa.selenium.interactions.Actions; +import com.vaadin.testbench.elements.DateFieldElement; import com.vaadin.testbench.elements.GridElement; import com.vaadin.testbench.elements.GridElement.GridCellElement; +import com.vaadin.testbench.elements.GridElement.GridEditorElement; +import com.vaadin.tests.annotations.TestCategory; import com.vaadin.tests.tb3.MultiBrowserTest; +@TestCategory("grid") public class BasicCrudGridEditorRowTest extends MultiBrowserTest { + private GridElement grid; + + @Before + public void openTest() { + openTestURL(); + grid = $(GridElement.class).first(); + + } @Test public void lookAndFeel() throws Exception { - openTestURL(); - GridElement grid = $(GridElement.class).first(); GridCellElement ritaBirthdate = grid.getCell(2, 3); compareScreen("grid"); // Open editor row new Actions(getDriver()).doubleClick(ritaBirthdate).perform(); compareScreen("editorrow"); + } + + @Test + public void editorRowOneInvalidValue() throws Exception { + GridCellElement ritaBirthdate = grid.getCell(2, 3); + // Open editor row + new Actions(getDriver()).doubleClick(ritaBirthdate).perform(); + GridEditorElement editor = grid.getEditor(); + DateFieldElement dateField = editor.$(DateFieldElement.class).first(); + WebElement input = dateField.findElement(By.xpath("input")); + // input.click(); + input.sendKeys("Invalid", Keys.TAB); + editor.save(); + + Assert.assertTrue("Editor wasn't displayed.", editor.isDisplayed()); + Assert.assertTrue("DateField wasn't displayed.", + dateField.isDisplayed()); + + Assert.assertTrue("DateField didn't have 'v-invalid' css class.", + hasCssClass(dateField, "v-datefield-error")); } + } -- 2.39.5