From c6bd17d9ad5124a9cb334701a181bc4a06a83653 Mon Sep 17 00:00:00 2001 From: Andreas Svanberg <andreass@dsv.su.se> Date: Mon, 16 Dec 2024 11:24:33 +0100 Subject: [PATCH 1/2] Fix grade calculator being serialized (#59) The new calculator that's based on templates has a reference to the @Entity for the template which should not be serialized. Fixes #40 ## How to test/replicate 1. Log in as a supervisor 1. Open a project that's new enough to use a grading report template with grade limits 1. Go to the "Finishing up" tab 1. Go to the sub-tab for an individual author Reviewed-on: https://gitea.dsv.su.se/DMC/scipro/pulls/59 Reviewed-by: Nico Athanassiadis <nico@dsv.su.se> Co-authored-by: Andreas Svanberg <andreass@dsv.su.se> Co-committed-by: Andreas Svanberg <andreass@dsv.su.se> --- .../su/dsv/scipro/report/GradeCalculator.java | 4 +--- .../grading/GradingReportPointsPanel.java | 20 ++++++------------- .../IndividualAuthorAssessmentPanel.java | 4 ++-- .../grading/GradingReportPointsPanelTest.java | 2 +- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/se/su/dsv/scipro/report/GradeCalculator.java b/core/src/main/java/se/su/dsv/scipro/report/GradeCalculator.java index 55cd0c3343..67abd527b3 100644 --- a/core/src/main/java/se/su/dsv/scipro/report/GradeCalculator.java +++ b/core/src/main/java/se/su/dsv/scipro/report/GradeCalculator.java @@ -1,8 +1,6 @@ package se.su.dsv.scipro.report; -import java.io.Serializable; - -public interface GradeCalculator extends Serializable { +public interface GradeCalculator { GradingReport.Grade getGrade(GradingReport gradingReport); long getPoints(GradingReport gradingReport); diff --git a/view/src/main/java/se/su/dsv/scipro/grading/GradingReportPointsPanel.java b/view/src/main/java/se/su/dsv/scipro/grading/GradingReportPointsPanel.java index 593ee532c5..4f95d80881 100644 --- a/view/src/main/java/se/su/dsv/scipro/grading/GradingReportPointsPanel.java +++ b/view/src/main/java/se/su/dsv/scipro/grading/GradingReportPointsPanel.java @@ -4,7 +4,6 @@ import org.apache.wicket.markup.html.WebMarkupContainer; import org.apache.wicket.markup.html.basic.Label; import org.apache.wicket.markup.html.panel.Panel; import org.apache.wicket.model.IModel; -import org.apache.wicket.model.LoadableDetachableModel; import se.su.dsv.scipro.components.OppositeVisibility; import se.su.dsv.scipro.report.GradeCalculator; import se.su.dsv.scipro.report.GradingReport; @@ -18,15 +17,13 @@ public class GradingReportPointsPanel extends Panel { public GradingReportPointsPanel( String id, final IModel<? extends GradingReport> gradingReportIModel, - final GradeCalculator gradeCalculator + final IModel<GradeCalculator> gradeCalculator ) { super(id, gradingReportIModel); - final IModel<GradingReport.Grade> gradeModel = new LoadableDetachableModel<>() { - @Override - protected GradingReport.Grade load() { - return gradingReportIModel.getObject().getGrade(gradeCalculator); - } - }; + final IModel<GradingReport.Grade> gradeModel = gradingReportIModel.combineWith( + gradeCalculator, + GradingReport::getGrade + ); final Label grade = new Label(GRADE, gradeModel.map(GradingReport.Grade::name)) { @Override protected void onConfigure() { @@ -36,12 +33,7 @@ public class GradingReportPointsPanel extends Panel { }; add(grade); - final IModel<Long> points = new LoadableDetachableModel<>() { - @Override - protected Long load() { - return gradingReportIModel.getObject().getPoints(gradeCalculator); - } - }; + final IModel<Long> points = gradingReportIModel.combineWith(gradeCalculator, GradingReport::getPoints); add(new Label(POINTS_LABEL, points)); add(new WebMarkupContainer(NO_GRADE_EXPLANATION).add(new OppositeVisibility(grade))); diff --git a/view/src/main/java/se/su/dsv/scipro/grading/IndividualAuthorAssessmentPanel.java b/view/src/main/java/se/su/dsv/scipro/grading/IndividualAuthorAssessmentPanel.java index 6d4ba8e0d6..c9c0938a58 100644 --- a/view/src/main/java/se/su/dsv/scipro/grading/IndividualAuthorAssessmentPanel.java +++ b/view/src/main/java/se/su/dsv/scipro/grading/IndividualAuthorAssessmentPanel.java @@ -271,8 +271,8 @@ public class IndividualAuthorAssessmentPanel extends GenericPanel<User> { new TemplatePanel("points_to_grade_conversion", gradingReport.map(SupervisorGradingReport::getProject)) ); - GradeCalculator supervisorCalculator = gradeCalculatorService.getSupervisorCalculator( - gradingReport.getObject().getProject() + IModel<GradeCalculator> supervisorCalculator = LoadableDetachableModel.of(() -> + gradeCalculatorService.getSupervisorCalculator(gradingReport.getObject().getProject()) ); add(new GradingReportPointsPanel("points", gradingReport, supervisorCalculator)); diff --git a/view/src/test/java/se/su/dsv/scipro/grading/GradingReportPointsPanelTest.java b/view/src/test/java/se/su/dsv/scipro/grading/GradingReportPointsPanelTest.java index caa5835e3f..f358d419e6 100644 --- a/view/src/test/java/se/su/dsv/scipro/grading/GradingReportPointsPanelTest.java +++ b/view/src/test/java/se/su/dsv/scipro/grading/GradingReportPointsPanelTest.java @@ -62,7 +62,7 @@ public class GradingReportPointsPanelTest extends SciProTest { private void startPanel() { panel = tester.startComponentInPage( - new GradingReportPointsPanel("id", Model.of(gradingReport), gradeCalculator) + new GradingReportPointsPanel("id", Model.of(gradingReport), () -> gradeCalculator) ); } From f67f37ecdd6bf4f45b36e9fb30039f6ad4b7f142 Mon Sep 17 00:00:00 2001 From: Andreas Svanberg <andreass@dsv.su.se> Date: Mon, 16 Dec 2024 13:23:37 +0100 Subject: [PATCH 2/2] Keep and validate project type selection when creating/editing application periods (#47) 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. Fixes #33 --- Now requires at least one project type to be selected before saving. Fixes #34 --- ## How to test 1. Go to "Admin" / "Match" / "Application periods" 2. Click create new 3. Submit without selecting any types 4. See that there's proper feedback 5. Leave name blank and select some types 6. Submit 7. See that the project type selection sticks around Co-authored-by: Nico Athanassiadis <nico@dsv.su.se> Reviewed-on: https://gitea.dsv.su.se/DMC/scipro/pulls/47 Reviewed-by: Nico Athanassiadis <nico@dsv.su.se> Co-authored-by: Andreas Svanberg <andreass@dsv.su.se> Co-committed-by: Andreas Svanberg <andreass@dsv.su.se> --- .../AdminEditApplicationPeriodPage.html | 6 +-- .../AdminEditApplicationPeriodPage.java | 50 ++++--------------- .../AdminEditApplicationPeriodPage.properties | 3 +- .../AdminEditApplicationPeriodPageTest.java | 25 +++++++++- 4 files changed, 36 insertions(+), 48 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..8c5597de96 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,8 @@ <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> + <div wicket:id="projectTypesFeedback"></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 2409d13d83..8781578a73 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 @@ -4,17 +4,14 @@ import jakarta.inject.Inject; import java.time.LocalDate; import java.time.LocalTime; import java.util.List; -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; @@ -39,7 +36,6 @@ public class AdminEditApplicationPeriodPage 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; @@ -62,19 +58,15 @@ public class AdminEditApplicationPeriodPage ); 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))); - } - } + BootstrapCheckBoxMultipleChoice<ProjectType> projectTypeChoice = new BootstrapCheckBoxMultipleChoice<>( + PROJECT_TYPES, + LambdaModel.of(getModel(), ApplicationPeriod::getProjectTypes, ApplicationPeriod::setProjectTypes), + availableProjectTypes(), + new LambdaChoiceRenderer<>(ProjectType::getName, ProjectType::getId) ); + projectTypeChoice.setRequired(true); + add(projectTypeChoice); + add(new FencedFeedbackPanel("projectTypesFeedback", projectTypeChoice)); final FormComponent<LocalDate> startDate = addDateField( START_DATE, LambdaModel.of(getModel(), ApplicationPeriod::getStartDate, ApplicationPeriod::setStartDate) @@ -139,30 +131,6 @@ public class AdminEditApplicationPeriodPage 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/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.properties b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.properties index f5095052d4..dc945a5788 100644 --- a/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.properties +++ b/view/src/main/java/se/su/dsv/scipro/applicationperiod/AdminEditApplicationPeriodPage.properties @@ -14,4 +14,5 @@ success= Application period saved. overlapping= Overlapping application period already exists. date.Required= You need to specify a valid date. hours.Required= Hours field is required. -minutes.Required= Minutes field is required. \ No newline at end of file +minutes.Required= Minutes field is required. +projectTypes.Required=You must select at least one project type. 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 7b68730909..a6bc599bd9 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 @@ -7,8 +7,10 @@ import java.io.Serializable; import java.time.LocalDate; import java.util.Collections; import java.util.List; +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; @@ -36,6 +38,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(); @@ -47,8 +50,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 @@ -105,7 +112,7 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest { ); 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); @@ -113,12 +120,25 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest { MatcherAssert.assertThat(captor.getValue().getProjectTypes(), Matchers.hasItem(bachelor)); } + @Test + public void requires_at_least_one_project_type_to_be_selected() { + FormTester formTester = tester.newFormTester(FORM); + fillInFormWithValidValues(formTester); + formTester.setValue(path(PROJECT_TYPES), ""); + formTester.submit(); + tester.assertErrorMessages(tester.getLastRenderedPage().getString("projectTypes.Required")); + } + private void submitForm(String title, int startDate, int endDate, int courseStartDate) { FormTester formTester = tester.newFormTester(FORM); fillInForm(title, startDate, endDate, courseStartDate, formTester); formTester.submit(); } + private void fillInFormWithValidValues(FormTester formTester) { + fillInForm("Title", 0, 1, 2, formTester); + } + private void fillInForm(String title, int startDate, int endDate, int courseStartDate, FormTester formTester) { formTester.setValue(TITLE, title); final LocalDate now = LocalDate.now(); @@ -126,6 +146,7 @@ public class AdminEditApplicationPeriodPageTest extends SciProTest { formTester.setValue(END_DATE, now.plusDays(endDate).toString()); formTester.setValue(COURSE_START_DATE, now.plusDays(courseStartDate).toString()); formTester.setValue("courseStartTime", "08:00"); + formTester.setValue(PROJECT_TYPES, String.valueOf(bachelor.getId())); } private void startPage() {