Skip to content

Commit

Permalink
Partially fix MID-4455: Approval form errors
Browse files Browse the repository at this point in the history 
Originally we checked for the presence of all object wrapper
fields; now we check only those that are present on the custom
form. It is only a partial fix, as the approval forms that add
extension items currently crash for users that have no extension
before. (Contains also slight refactoring of dynamic forms code.)

(cherry picked from commit d201ab9)
  • Loading branch information
@mederly
mederly committed Feb 21, 2018
1 parent e84469f commit 5ce610e
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 57 deletions.
14 changes: 5 additions & 9 deletions gui/admin-gui/src/main/java/com/evolveum/midpoint/gui/impl/util/GuiImplUtil.java
View file
Expand Up @@ -15,9 +15,8 @@
*/
package com.evolveum.midpoint.gui.impl.util;

import com.evolveum.midpoint.prism.path.ItemPath;
import com.evolveum.midpoint.xml.ns._public.common.common_3.AbstractFormItemType;
import com.evolveum.midpoint.xml.ns._public.common.common_3.VariableBindingDefinitionType;
import com.evolveum.prism.xml.ns._public.types_3.ItemPathType;

/**
* Class for misc GUI util methods (impl).
Expand All @@ -27,15 +26,12 @@
*/
public class GuiImplUtil {

public static ItemPathType getPathType(AbstractFormItemType aItem) {
if (aItem == null) {
public static ItemPath getItemPath(AbstractFormItemType aItem) {
if (aItem != null && aItem.getBinding() != null && aItem.getBinding().getPath() != null) {
return aItem.getBinding().getPath().getItemPath();
} else {
return null;
}
VariableBindingDefinitionType binding = aItem.getBinding();
if (binding == null) {
return null;
}
return binding.getPath();
}

}
2 changes: 1 addition & 1 deletion ...in-gui/src/main/java/com/evolveum/midpoint/web/component/prism/ContainerValueWrapper.java
View file
Expand Up @@ -694,7 +694,7 @@ public boolean checkRequired(PageBase pageBase) {
boolean rv = true;
for (ItemWrapper itemWrapper : getItems()) {
if (!itemWrapper.checkRequired(pageBase)) {
rv = false;
rv = false; // not returning directly as we want to go through all the values
}
}
return rv;
Expand Down
6 changes: 3 additions & 3 deletions gui/admin-gui/src/main/java/com/evolveum/midpoint/web/component/prism/ContainerWrapper.java
View file
Expand Up @@ -419,9 +419,9 @@ public <O extends ObjectType> void collectModifications(ObjectDelta<O> delta) th
@Override
public boolean checkRequired(PageBase pageBase) {
boolean rv = true;
for (ContainerValueWrapper<C> itemWrapper : getValues()) {
if (!itemWrapper.checkRequired(pageBase)) {
rv = false;
for (ContainerValueWrapper<C> valueWrapper : getValues()) {
if (!valueWrapper.checkRequired(pageBase)) {
rv = false; // not returning directly as we want to go through all the values
}
}
return rv;
Expand Down
85 changes: 54 additions & 31 deletions ...n-gui/src/main/java/com/evolveum/midpoint/web/component/prism/DynamicFieldGroupPanel.java
View file
Expand Up @@ -18,16 +18,17 @@
import com.evolveum.midpoint.gui.api.component.BasePanel;
import com.evolveum.midpoint.gui.api.page.PageBase;
import com.evolveum.midpoint.gui.impl.util.GuiImplUtil;
import com.evolveum.midpoint.prism.Containerable;
import com.evolveum.midpoint.prism.ItemDefinition;
import com.evolveum.midpoint.prism.ItemDefinitionImpl;
import com.evolveum.midpoint.prism.PrismContainerDefinition;
import com.evolveum.midpoint.prism.path.ItemPath;
import com.evolveum.midpoint.prism.xml.XsdTypeMapper;
import com.evolveum.midpoint.schema.util.FormTypeUtil;
import com.evolveum.midpoint.util.Holder;
import com.evolveum.midpoint.util.logging.Trace;
import com.evolveum.midpoint.util.logging.TraceManager;
import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
import com.evolveum.prism.xml.ns._public.types_3.ItemPathType;
import org.apache.commons.lang.StringUtils;
import org.apache.wicket.AttributeModifier;
import org.apache.wicket.RestartResponseException;
Expand All @@ -52,7 +53,7 @@ public class DynamicFieldGroupPanel<O extends ObjectType> extends BasePanel<Obje
private List<AbstractFormItemType> formItems;

public DynamicFieldGroupPanel(String id, String groupName, IModel<ObjectWrapper<O>> objectWrapper, List<AbstractFormItemType> formItems, Form<?> mainForm, PageBase parentPage) {
super(id,objectWrapper);
super(id, objectWrapper);
setParent(parentPage);
this.formItems = formItems;
initLayout(groupName, formItems, mainForm);
Expand All @@ -61,12 +62,16 @@ public DynamicFieldGroupPanel(String id, String groupName, IModel<ObjectWrapper<
public DynamicFieldGroupPanel(String id, IModel<ObjectWrapper<O>> objectWrapper, @NotNull FormDefinitionType formDefinition, Form<?> mainForm, PageBase parentPage) {
super(id, objectWrapper);
setParent(parentPage);
String groupName = "Basic";
this.formItems = FormTypeUtil.getFormItems(formDefinition.getFormItems());
initLayout(getGroupName(formDefinition), formItems, mainForm);
}

private String getGroupName(@NotNull FormDefinitionType formDefinition) {
if (formDefinition.getDisplay() != null) {
groupName = formDefinition.getDisplay().getLabel();
return formDefinition.getDisplay().getLabel();
} else {
return "Basic";
}
this.formItems = FormTypeUtil.getFormItems(formDefinition.getFormItems());
initLayout(groupName, formItems, mainForm);
}

private void initLayout(String groupName, List<AbstractFormItemType> formItems, Form<?> mainForm) {
Expand All @@ -81,47 +86,54 @@ private void initLayout(String groupName, List<AbstractFormItemType> formItems,
for (AbstractFormItemType formItem : formItems) {

if (formItem instanceof FormFieldGroupType) {
DynamicFieldGroupPanel<O> dynamicFieldGroupPanel = new DynamicFieldGroupPanel<O>(itemView.newChildId(), formItem.getName(), getModel(), FormTypeUtil.getFormItems(((FormFieldGroupType) formItem).getFormItems()), mainForm, getPageBase());
DynamicFieldGroupPanel<O> dynamicFieldGroupPanel = new DynamicFieldGroupPanel<>(itemView.newChildId(), formItem.getName(), getModel(), FormTypeUtil.getFormItems(((FormFieldGroupType) formItem).getFormItems()), mainForm, getPageBase());
dynamicFieldGroupPanel.setOutputMarkupId(true);
itemView.add(dynamicFieldGroupPanel);
continue;
}

ItemWrapper itemWrapper = createItemWrapper(formItem, getObjectWrapper());
ItemWrapper<?,?,?> itemWrapper = findAndTailorItemWrapper(formItem, getObjectWrapper());

if (itemWrapper instanceof ContainerWrapper) {
PrismContainerPanel containerPanel = new PrismContainerPanel(itemView.newChildId(),
Model.of((ContainerWrapper) itemWrapper), true, mainForm, w -> {return true;}, getPageBase());
containerPanel.setOutputMarkupId(true);
itemView.add(containerPanel);
//noinspection unchecked
ContainerWrapper<Containerable> containerWrapper = (ContainerWrapper<Containerable>) itemWrapper;
PrismContainerPanel<?> containerPanel = new PrismContainerPanel<>(itemView.newChildId(),
Model.of(containerWrapper), true, mainForm, w -> true, getPageBase());
containerPanel.setOutputMarkupId(true);
itemView.add(containerPanel);
} else {
PrismPropertyPanel<?> propertyPanel = new PrismPropertyPanel<>(itemView.newChildId(),
Model.of(itemWrapper), mainForm, null, getPageBase());
propertyPanel.setOutputMarkupId(true);
propertyPanel.add(AttributeModifier.append("class", ((i % 2) == 0) ? "" : "stripe"));
itemView.add(propertyPanel);
}

i++;

}
}

private ItemWrapper createItemWrapper(AbstractFormItemType formField, ObjectWrapper objectWrapper) {
ItemPathType itemPathType = GuiImplUtil.getPathType(formField);
private RepeatingView getRepeatingPropertyView() {
return (RepeatingView) get(ID_PROPERTY);
}

@NotNull
private ItemWrapper<?,?,?> findAndTailorItemWrapper(AbstractFormItemType formField, ObjectWrapper<O> objectWrapper) {
ItemWrapper<?, ?, ?> itemWrapper = findItemWrapper(formField, objectWrapper);
applyFormDefinition(itemWrapper, formField);
return itemWrapper;
}

if (itemPathType == null) {
@NotNull
private ItemWrapper<?, ?, ?> findItemWrapper(AbstractFormItemType formField, ObjectWrapper<O> objectWrapper) {
ItemPath path = GuiImplUtil.getItemPath(formField);
if (path == null) {
getSession().error("Bad form item definition. It has to contain reference to the real attribute");
LOGGER.error("Bad form item definition. It has to contain reference to the real attribute");
throw new RestartResponseException(getPageBase());
}

ItemPath path = itemPathType.getItemPath();

ItemDefinition itemDef = objectWrapper.getObject().getDefinition().findItemDefinition(path);

ItemWrapper itemWrapper;

ItemWrapper<?,?,?> itemWrapper;
ItemDefinition<?> itemDef = objectWrapper.getObject().getDefinition().findItemDefinition(path);
if (itemDef instanceof PrismContainerDefinition) {
itemWrapper = objectWrapper.findContainerWrapper(path);
} else {
Expand All @@ -132,10 +144,7 @@ private ItemWrapper createItemWrapper(AbstractFormItemType formField, ObjectWrap
LOGGER.error("Bad form item definition. No attribute with path: " + path + " was found");
throw new RestartResponseException(getPageBase());
}

applyFormDefinition(itemWrapper, formField);
return itemWrapper;

}

private void applyFormDefinition(ItemWrapper itemWrapper, AbstractFormItemType formField) {
Expand All @@ -150,19 +159,15 @@ private void applyFormDefinition(ItemWrapper itemWrapper, AbstractFormItemType f
if (StringUtils.isNotEmpty(displayType.getLabel())) {
itemDef.setDisplayName(displayType.getLabel());
}

if (StringUtils.isNotEmpty(displayType.getHelp())) {
itemDef.setHelp(displayType.getHelp());
}

if (StringUtils.isNotEmpty(displayType.getMaxOccurs())) {
itemDef.setMaxOccurs(XsdTypeMapper.multiplicityToInteger(displayType.getMaxOccurs()));
}

if (StringUtils.isNotEmpty(displayType.getMinOccurs())) {
itemDef.setMinOccurs(XsdTypeMapper.multiplicityToInteger(displayType.getMinOccurs()));
}

}

public ObjectWrapper<O> getObjectWrapper() {
Expand All @@ -173,5 +178,23 @@ public List<AbstractFormItemType> getFormItems() {
return formItems;
}


/**
* Checks embedded properties if they are the minOccurs check.
* Experimental implementation. Please do not rely on it too much.
*/
public boolean checkRequiredFields(PageBase pageBase) {
Holder<Boolean> rvHolder = new Holder<>(true);
getRepeatingPropertyView().visitChildren((component, iVisit) -> {
if (component instanceof PrismPropertyPanel) {
IModel<?> model = component.getDefaultModel();
if (model != null && model.getObject() instanceof ItemWrapper) {
ItemWrapper<?, ?, ?> itemWrapper = (ItemWrapper<?, ?, ?>) model.getObject();
if (!itemWrapper.checkRequired(pageBase)) {
rvHolder.setValue(false);
}
}
}
});
return rvHolder.getValue();
}
}
14 changes: 9 additions & 5 deletions gui/admin-gui/src/main/java/com/evolveum/midpoint/web/component/prism/DynamicFormPanel.java
View file
Expand Up @@ -149,8 +149,12 @@ public ObjectDelta<O> getObjectDelta() throws SchemaException {
}

public boolean checkRequiredFields(PageBase pageBase) {
// TODO - or check only fields present in the form itself? (But other required attributes should be present as well.)
return wrapperModel.getObject().checkRequiredFields(pageBase);
return getFormFields().checkRequiredFields(pageBase);
}

@SuppressWarnings("unchecked")
public DynamicFieldGroupPanel<O> getFormFields() {
return (DynamicFieldGroupPanel<O>) get(ID_FORM_FIELDS);
}

public PrismObject<O> getObject() throws SchemaException {
Expand All @@ -171,9 +175,9 @@ public List<ItemPath> getChangedItems() {

private void collectItemPaths(List<AbstractFormItemType> items, List<ItemPath> paths) {
for (AbstractFormItemType aItem : items) {
ItemPathType itemPathType = GuiImplUtil.getPathType(aItem);
if (itemPathType != null) {
paths.add(itemPathType.getItemPath());
ItemPath itemPath = GuiImplUtil.getItemPath(aItem);
if (itemPath != null) {
paths.add(itemPath);
}
if (aItem instanceof FormFieldGroupType) {
collectItemPaths(FormTypeUtil.getFormItems(((FormFieldGroupType) aItem).getFormItems()), paths);
Expand Down
19 changes: 12 additions & 7 deletions gui/admin-gui/src/main/java/com/evolveum/midpoint/web/component/prism/ObjectWrapper.java
View file
Expand Up @@ -578,17 +578,22 @@ public void copyRuntimeStateTo(ObjectWrapper<O> newWrapper) {
newWrapper.setReadonly(this.isReadonly());
}

// returns true if everything is OK
// to be used when enforceRequiredFields is false, so we want to check them
// only when really needed
// (e.g. MID-3876: when rejecting a work item, fields marked as required
// need not be present)
/**
* To be used when enforceRequiredFields is false, so we want to check them
* only when really needed (e.g. MID-3876: when rejecting a work item,
* fields marked as required need not be present).
*
* Currently unused; we use DynamicFormPanel.checkRequiredFields instead. Might be
* useful in the future.
*
* @return true if everything is OK
*/
@SuppressWarnings("unused")
public boolean checkRequiredFields(PageBase pageBase) {
boolean rv = true;
for (ContainerWrapper<? extends Containerable> container : containers) {
if (!container.checkRequired(pageBase)) {
rv = false; // continuing to display messages for all missing
// fields
rv = false; // continuing to display messages for all missing fields
}
}
return rv;
Expand Down
2 changes: 1 addition & 1 deletion ...admin-gui/src/main/java/com/evolveum/midpoint/web/component/prism/PrismPropertyPanel.java
View file
Expand Up @@ -65,7 +65,7 @@ public class PrismPropertyPanel<IW extends ItemWrapper> extends Panel {
private boolean labelContainerVisible = true;

public PrismPropertyPanel(String id, final IModel<IW> model, Form form, ItemVisibilityHandler visibilityHandler, PageBase pageBase) {
super(id);
super(id, model);
Validate.notNull(model, "no model");
this.pageBase = pageBase;

Expand Down

0 comments on commit 5ce610e

Please sign in to comment.