From d2e490bdd8884dc51d65b14dfda6175d8fa3f602 Mon Sep 17 00:00:00 2001
From: Andreas Svanberg <andreass@dsv.su.se>
Date: Mon, 2 Dec 2024 13:31:29 +0100
Subject: [PATCH] Fix project type selection being lost on validation error
while creating application periods
If you have FormComponents in a ListView you need to call setReuseItems(true) on the ListView. Otherwise the ListItems will be recreated before rendering which results in them losing their "converted input" (what Wicket calls the submitted value).
Instead of simply calling setReuseItems(true) on the ListView, which would've solved the problem, it was instead replaced by a proper FormComponent for dealing with this exact case (a CheckboxMultipleChoice component). This reduces the amount of code required and more clearly communicates intent. The change required some minor test refactoring.
---
.../AdminEditApplicationPeriodPage.html | 5 +-
.../AdminEditApplicationPeriodPage.java | 46 +++++--------------
.../AdminEditApplicationPeriodPageTest.java | 11 ++++-
3 files changed, 21 insertions(+), 41 deletions(-)
diff --git a/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.html b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.html
index 04de12622c..4581c0f65b 100644
--- a/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.html
+++ b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.html
@@ -16,10 +16,7 @@
<fieldset class="mb-3">
<legend><wicket:message key="projectTypes"/></legend>
- <div class="form-check" wicket:id="projectTypes">
- <input type="checkbox" wicket:id="checkbox" class="form-check-input"/>
- <label class="form-check-label" wicket:for="checkbox"><span wicket:id="name"></span></label>
- </div>
+ <div wicket:id="projectTypes"></div>
</fieldset>
<div class="form-row">
diff --git a/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.java b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.java
index bbb588c1a2..0a5bb1354a 100644
--- a/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.java
+++ b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.java
@@ -1,16 +1,13 @@
package se.su.dsv.scipro.applicationperiod;
-import org.apache.wicket.extensions.model.AbstractCheckBoxModel;
import org.apache.wicket.feedback.FencedFeedbackPanel;
-import org.apache.wicket.markup.html.basic.Label;
import org.apache.wicket.markup.html.form.*;
-import org.apache.wicket.markup.html.list.ListItem;
-import org.apache.wicket.markup.html.list.ListView;
import org.apache.wicket.markup.html.panel.ComponentFeedbackPanel;
import org.apache.wicket.model.IModel;
import org.apache.wicket.model.LambdaModel;
import org.apache.wicket.model.LoadableDetachableModel;
import org.apache.wicket.request.mapper.parameter.PageParameters;
+import se.su.dsv.scipro.components.BootstrapCheckBoxMultipleChoice;
import se.su.dsv.scipro.components.BootstrapDatePicker;
import se.su.dsv.scipro.components.BootstrapTimePicker;
import se.su.dsv.scipro.components.DatesValidator;
@@ -38,7 +35,6 @@ public class AdminEditApplicationPeriodPage extends AbstractAdminMatchPage imple
public static final String TITLE = "title";
public static final String FEEDBACK = "Feedback";
public static final String TITLE_FEEDBACK = "titleFeedback";
- public static final String CHECKBOX = "checkbox";
@Inject
private ProjectTypeService projectTypeService;
@@ -57,13 +53,16 @@ public class AdminEditApplicationPeriodPage extends AbstractAdminMatchPage imple
RequiredTextField<String> title = new RequiredTextField<>(TITLE, LambdaModel.of(getModel(), ApplicationPeriod::getName, ApplicationPeriod::setName));
add(new ComponentFeedbackPanel(TITLE_FEEDBACK, title));
add(title);
- add(new ListView<>(PROJECT_TYPES, availableProjectTypes()) {
- @Override
- protected void populateItem(ListItem<ProjectType> item) {
- item.add(new CheckBox(CHECKBOX, new ProjectTypeSelectionModel(item.getModel())).setOutputMarkupId(true));
- item.add(new Label("name", item.getModel().map(ProjectType::getName)));
- }
- });
+ add(new BootstrapCheckBoxMultipleChoice<>(
+ PROJECT_TYPES,
+ LambdaModel.of(
+ getModel(),
+ ApplicationPeriod::getProjectTypes,
+ ApplicationPeriod::setProjectTypes
+ ),
+ availableProjectTypes(),
+ new LambdaChoiceRenderer<>(ProjectType::getName, ProjectType::getId)
+ ));
final FormComponent<LocalDate> startDate = addDateField(START_DATE, LambdaModel.of(getModel(), ApplicationPeriod::getStartDate, ApplicationPeriod::setStartDate));
final FormComponent<LocalDate> endDate = addDateField(END_DATE, LambdaModel.of(getModel(), ApplicationPeriod::getEndDate, ApplicationPeriod::setEndDate));
final FormComponent<LocalDate> courseStartDate = addDateField(COURSE_START_DATE, LambdaModel.of(getModel(), ApplicationPeriod::getCourseStartDate, ApplicationPeriod::setCourseStartDate));
@@ -108,29 +107,6 @@ public class AdminEditApplicationPeriodPage extends AbstractAdminMatchPage imple
getRootForm().error(getString("overlapping"));
}
}
-
- private class ProjectTypeSelectionModel extends AbstractCheckBoxModel {
- private final IModel<ProjectType> model;
-
- public ProjectTypeSelectionModel(IModel<ProjectType> model) {
- this.model = model;
- }
-
- @Override
- public boolean isSelected() {
- return getModelObject().getProjectTypes().contains(model.getObject());
- }
-
- @Override
- public void select() {
- getModelObject().addProjectType(model.getObject());
- }
-
- @Override
- public void unselect() {
- getModelObject().removeProjectType(model.getObject());
- }
- }
}
private LoadableDetachableModel<ApplicationPeriod> getLoaded(final PageParameters pp){
diff --git a/view/src/test/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPageTest.java b/view/src/test/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPageTest.java
index 9c6fffbf30..b1f056276e 100644
--- a/view/src/test/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPageTest.java
+++ b/view/src/test/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPageTest.java
@@ -1,7 +1,9 @@
package se.su.dsv.scipro.applicationperiod;
+import org.apache.wicket.Component;
import org.apache.wicket.Page;
import org.apache.wicket.feedback.FeedbackMessage;
+import org.apache.wicket.markup.html.form.CheckBoxMultipleChoice;
import org.apache.wicket.markup.html.form.RequiredTextField;
import org.apache.wicket.request.mapper.parameter.PageParameters;
import org.apache.wicket.util.tester.FormTester;
@@ -38,6 +40,7 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest {
@BeforeEach
public void setUp() throws Exception {
bachelor = new ProjectType(DegreeType.BACHELOR, "Bachelor", "Bachelor");
+ bachelor.setId(8L);
bachelor.addModule(ProjectModule.MATCH);
when(projectTypeService.findWithModule(ProjectModule.MATCH)).thenReturn(Collections.singletonList(bachelor));
startPage();
@@ -49,8 +52,12 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest {
}
@Test
+ @SuppressWarnings("unchecked")
public void contains_project_type_selection() {
- tester.assertModelValue(path(FORM, PROJECT_TYPES), projectTypeService.findWithModule(ProjectModule.MATCH));
+ tester.assertComponent(path(FORM, PROJECT_TYPES), CheckBoxMultipleChoice.class);
+ Component component = tester.getComponentFromLastRenderedPage(path(FORM, PROJECT_TYPES));
+ CheckBoxMultipleChoice<ProjectType> choice = (CheckBoxMultipleChoice<ProjectType>) component;
+ Assertions.assertEquals(projectTypeService.findWithModule(ProjectModule.MATCH), choice.getChoices());
}
@Test
@@ -99,7 +106,7 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest {
when(applicationPeriodService.doesPeriodOverlap(ArgumentMatchers.any(ApplicationPeriod.class))).thenReturn(NOT_OVERLAPPING);
FormTester formTester = tester.newFormTester(FORM);
fillInForm("Title", 0, 1, 2, formTester);
- formTester.setValue(path(PROJECT_TYPES, 0, CHECKBOX), true);
+ formTester.setValue(path(PROJECT_TYPES), String.valueOf(bachelor.getId()));
formTester.submit();
ArgumentCaptor<ApplicationPeriod> captor = ArgumentCaptor.forClass(ApplicationPeriod.class);