From 8c0ef61b21cd8cf4fa797f2074388b3420973fe6 Mon Sep 17 00:00:00 2001
From: uhensler <none@none>
Date: Wed, 11 Oct 2017 14:38:14 +0200
Subject: [PATCH] OO-3067: Handle exception if a course reminder rule is
invalid and add some validations in GUI to avoid invalid reminder rules
---
.../gta/ui/BeforeDateTaskRuleEditor.java | 20 ++++++++
.../reminder/ui/AttemptsRuleEditor.java | 23 +++++++--
.../ui/InitialAttemptsRuleEditor.java | 23 +++++++--
.../course/reminder/ui/ScoreRuleEditor.java | 20 ++++++++
.../reminder/manager/ReminderRuleEngine.java | 51 +++++++++++++++----
.../reminder/ui/CourseLaunchRuleEditor.java | 20 ++++++++
...oryEntryLifecycleAfterValidRuleEditor.java | 20 ++++++++
.../ui/_i18n/LocalStrings_de.properties | 1 +
.../ui/_i18n/LocalStrings_en.properties | 1 +
.../manager/ReminderRuleEngineTest.java | 29 +++++++++++
10 files changed, 193 insertions(+), 15 deletions(-)
diff --git a/src/main/java/org/olat/course/nodes/gta/ui/BeforeDateTaskRuleEditor.java b/src/main/java/org/olat/course/nodes/gta/ui/BeforeDateTaskRuleEditor.java
index 01e275aea9b..c56f25166ed 100644
--- a/src/main/java/org/olat/course/nodes/gta/ui/BeforeDateTaskRuleEditor.java
+++ b/src/main/java/org/olat/course/nodes/gta/ui/BeforeDateTaskRuleEditor.java
@@ -214,11 +214,31 @@ public class BeforeDateTaskRuleEditor extends RuleEditorFragment {
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
+
@Override
public ReminderRule getConfiguration() {
ReminderRuleImpl configuredRule = null;
diff --git a/src/main/java/org/olat/course/reminder/ui/AttemptsRuleEditor.java b/src/main/java/org/olat/course/reminder/ui/AttemptsRuleEditor.java
index 82bf022d195..dc3684a3b41 100644
--- a/src/main/java/org/olat/course/reminder/ui/AttemptsRuleEditor.java
+++ b/src/main/java/org/olat/course/reminder/ui/AttemptsRuleEditor.java
@@ -181,13 +181,30 @@ public class AttemptsRuleEditor extends RuleEditorFragment {
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
- } else if(!StringHelper.isLong(valueEl.getValue())) {
- valueEl.setErrorKey("error.int", null);
- allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
@Override
public ReminderRule getConfiguration() {
diff --git a/src/main/java/org/olat/course/reminder/ui/InitialAttemptsRuleEditor.java b/src/main/java/org/olat/course/reminder/ui/InitialAttemptsRuleEditor.java
index 42078729ee6..100332efaf2 100644
--- a/src/main/java/org/olat/course/reminder/ui/InitialAttemptsRuleEditor.java
+++ b/src/main/java/org/olat/course/reminder/ui/InitialAttemptsRuleEditor.java
@@ -185,13 +185,30 @@ public class InitialAttemptsRuleEditor extends RuleEditorFragment {
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
- } else if(!StringHelper.isLong(valueEl.getValue())) {
- valueEl.setErrorKey("error.int", null);
- allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
@Override
public ReminderRule getConfiguration() {
diff --git a/src/main/java/org/olat/course/reminder/ui/ScoreRuleEditor.java b/src/main/java/org/olat/course/reminder/ui/ScoreRuleEditor.java
index 28cd5759355..55fd2852379 100644
--- a/src/main/java/org/olat/course/reminder/ui/ScoreRuleEditor.java
+++ b/src/main/java/org/olat/course/reminder/ui/ScoreRuleEditor.java
@@ -174,10 +174,30 @@ public class ScoreRuleEditor extends RuleEditorFragment {
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
@Override
public ReminderRule getConfiguration() {
diff --git a/src/main/java/org/olat/modules/reminder/manager/ReminderRuleEngine.java b/src/main/java/org/olat/modules/reminder/manager/ReminderRuleEngine.java
index b22df840e70..1d4b0a19793 100644
--- a/src/main/java/org/olat/modules/reminder/manager/ReminderRuleEngine.java
+++ b/src/main/java/org/olat/modules/reminder/manager/ReminderRuleEngine.java
@@ -28,6 +28,8 @@ import java.util.Set;
import org.olat.basesecurity.GroupRoles;
import org.olat.core.id.Identity;
+import org.olat.core.logging.OLog;
+import org.olat.core.logging.Tracing;
import org.olat.modules.reminder.FilterRuleSPI;
import org.olat.modules.reminder.IdentitiesProviderRuleSPI;
import org.olat.modules.reminder.Reminder;
@@ -56,6 +58,8 @@ import org.springframework.stereotype.Service;
@Service
public class ReminderRuleEngine {
+ private static final OLog log = Tracing.createLoggerFor(ReminderRuleEngine.class);
+
public static final String DATE_RULE_TYPE = DateRuleSPI.class.getSimpleName();
public static final String USER_PROP_RULE_TYPE = UserPropertyRuleSPI.class.getSimpleName();
public static final String REPO_ROLE_RULE_TYPE = RepositoryEntryRoleRuleSPI.class.getSimpleName();
@@ -84,11 +88,7 @@ public class ReminderRuleEngine {
}
List<ReminderRule> ruleList = new ArrayList<>(rules.getRules());
- //1. Date rules doesn't need database queries
- boolean allOk = evaluateDateRule(ruleList);
- if(allOk) {
- allOk = evaluateRepositoryEntryRule(reminder.getEntry(), ruleList);
- }
+ boolean allOk = evaluate(reminder, ruleList);
List<Identity> identities;
if(allOk) {
@@ -103,6 +103,21 @@ public class ReminderRuleEngine {
}
return identities;
}
+
+ public boolean evaluate(Reminder reminder, List<ReminderRule> ruleList) {
+ boolean allOk = true;
+ try {
+ // 1. Date rules doesn't need database queries
+ allOk = evaluateDateRule(ruleList);
+ if (allOk) {
+ allOk = evaluateRepositoryEntryRule(reminder.getEntry(), ruleList);
+ }
+ } catch (Exception e) {
+ allOk = false;
+ log.error("", e);
+ }
+ return allOk;
+ }
/**
*
@@ -163,9 +178,7 @@ public class ReminderRuleEngine {
identities = null;
for(ReminderRule rule:identitiesProviderRules) {
- RuleSPI ruleSpi = reminderModule.getRuleSPIByType(rule.getType());
- IdentitiesProviderRuleSPI identitiesProviderRuleSpi = (IdentitiesProviderRuleSPI)ruleSpi;
- List<Identity> members = identitiesProviderRuleSpi.evaluate(entry, rule);
+ List<Identity> members = getMembers(entry, rule);
if(identities == null) {
identities = members;
} else {
@@ -189,6 +202,18 @@ public class ReminderRuleEngine {
}
return identities;
}
+
+ public List<Identity> getMembers(RepositoryEntry entry, ReminderRule rule) {
+ List<Identity> members = new ArrayList<>();
+ try {
+ RuleSPI ruleSpi = reminderModule.getRuleSPIByType(rule.getType());
+ IdentitiesProviderRuleSPI identitiesProviderRuleSpi = (IdentitiesProviderRuleSPI)ruleSpi;
+ members = identitiesProviderRuleSpi.evaluate(entry, rule);
+ } catch (Exception e) {
+ log.error("", e);
+ }
+ return members;
+ }
/**
* Remove identities of the list which not match the user properties rules if any.
@@ -239,11 +264,19 @@ public class ReminderRuleEngine {
}
for(ReminderRule rule:filterRules) {
+ filterByRule(entry, identities, rule);
+ }
+ }
+
+ public void filterByRule(RepositoryEntry entry, List<Identity> identities, ReminderRule rule) {
+ try {
RuleSPI ruleSpi = reminderModule.getRuleSPIByType(rule.getType());
if(ruleSpi instanceof FilterRuleSPI) {
FilterRuleSPI filter = (FilterRuleSPI)ruleSpi;
filter.filter(entry, identities, rule);
- }
+ }
+ } catch (Exception e) {
+ log.error("", e);
}
}
}
diff --git a/src/main/java/org/olat/modules/reminder/ui/CourseLaunchRuleEditor.java b/src/main/java/org/olat/modules/reminder/ui/CourseLaunchRuleEditor.java
index e614199ee8e..8d1eb673093 100644
--- a/src/main/java/org/olat/modules/reminder/ui/CourseLaunchRuleEditor.java
+++ b/src/main/java/org/olat/modules/reminder/ui/CourseLaunchRuleEditor.java
@@ -117,10 +117,30 @@ public class CourseLaunchRuleEditor extends RuleEditorFragment {
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
@Override
public ReminderRule getConfiguration() {
diff --git a/src/main/java/org/olat/modules/reminder/ui/RepositoryEntryLifecycleAfterValidRuleEditor.java b/src/main/java/org/olat/modules/reminder/ui/RepositoryEntryLifecycleAfterValidRuleEditor.java
index e9ed6535563..1c03c5bc348 100644
--- a/src/main/java/org/olat/modules/reminder/ui/RepositoryEntryLifecycleAfterValidRuleEditor.java
+++ b/src/main/java/org/olat/modules/reminder/ui/RepositoryEntryLifecycleAfterValidRuleEditor.java
@@ -121,10 +121,30 @@ public class RepositoryEntryLifecycleAfterValidRuleEditor extends RuleEditorFrag
if(!StringHelper.containsNonWhitespace(valueEl.getValue())) {
valueEl.setErrorKey("form.mandatory.hover", null);
allOk &= false;
+ } else {
+ allOk &= validateInt(valueEl);
}
return allOk;
}
+
+ private boolean validateInt(TextElement el) {
+ boolean allOk = true;
+
+ if(el.isVisible()) {
+ String value = el.getValue();
+ if(StringHelper.containsNonWhitespace(value)) {
+ try {
+ Integer.parseInt(value);
+ } catch(Exception e) {
+ allOk = false;
+ el.setErrorKey("error.wrong.int", null);
+ }
+ }
+ }
+
+ return allOk;
+ }
@Override
public ReminderRule getConfiguration() {
diff --git a/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_de.properties b/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_de.properties
index 9d79a70eae1..25a6646b059 100644
--- a/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_de.properties
+++ b/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_de.properties
@@ -8,6 +8,7 @@ default.send.time=Standard Sendezeit f\u00FCr Erinnerungen
enable.reminders=Kurserinnerungen einschalten
enable.sms.reminders=SMS Erinnerungen
error.group.not.found=Gruppe existiert nicht in diesem Kurs
+error.wrong.int=Falsches Zahlenformat. Beispiele\: -1, 0, 2, +10
interval=Zeitintervall
interval.24=1 Mal pro Tag
interval.12=2 Mal pro Tag
diff --git a/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_en.properties b/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_en.properties
index 2a185d8f5a6..8e6404b98ea 100644
--- a/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_en.properties
+++ b/src/main/java/org/olat/modules/reminder/ui/_i18n/LocalStrings_en.properties
@@ -8,6 +8,7 @@ default.send.time=Default time to send reminders
enable.reminders=Activate course reminders
enable.sms.reminders=SMS reminders
error.group.not.found=Group doesn't exist within this course
+error.wrong.int=Wrong numeral format. Examples\: -1, 0, 2, +10
interval=Interval
interval.24=Once a day
interval.12=Twice a day
diff --git a/src/test/java/org/olat/modules/reminder/manager/ReminderRuleEngineTest.java b/src/test/java/org/olat/modules/reminder/manager/ReminderRuleEngineTest.java
index 55a6408ca70..5ae4df4f5d5 100644
--- a/src/test/java/org/olat/modules/reminder/manager/ReminderRuleEngineTest.java
+++ b/src/test/java/org/olat/modules/reminder/manager/ReminderRuleEngineTest.java
@@ -19,8 +19,11 @@
*/
package org.olat.modules.reminder.manager;
+import static org.assertj.core.api.Assertions.assertThat;
+
import java.util.ArrayList;
import java.util.Calendar;
+import java.util.Collections;
import java.util.Date;
import java.util.List;
@@ -1064,4 +1067,30 @@ public class ReminderRuleEngineTest extends OlatTestCase {
return testNode.getIdent();
}
+
+ @Test
+ public void evaluateRuleListThrowsException() {
+ List<ReminderRule> ruleList = Collections.<ReminderRule>emptyList();
+
+ boolean ollOk = ruleEngine.evaluate(null, ruleList);
+
+ Assert.assertFalse(ollOk);
+ }
+
+ @Test
+ public void getMembersThrowsException() {
+
+ List<Identity> members = ruleEngine.getMembers(null, null);
+
+ assertThat(members).isNotNull().isEmpty();
+ }
+
+ @Test
+ public void getFilterThrowsException() {
+ ReminderRuleImpl rule = new ReminderRuleImpl();
+ rule.setType(ScoreRuleSPI.class.getSimpleName());
+ rule.setRightOperand("no integer");
+
+ ruleEngine.filterByRule(null, null, rule);
+ }
}
--
GitLab