Skip to content

Commit

Permalink
improved SEB Restriction handling error handling and logging
Browse files Browse the repository at this point in the history
  • Loading branch information
anhefti committed Dec 14, 2023
1 parent 0544d7a commit fc31059
Show file tree
Hide file tree
Showing 19 changed files with 77 additions and 139 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,7 @@ private Result<T> handleClosed(

this.state = State.HALF_OPEN;
this.failingCount.set(0);
return Result.ofError(new RuntimeException(
"Set CircuitBeaker to half-open state. Cause: " + result.getError(),
result.getError()));
return result;
} else {
// try again
return protectedRun(supplier);
Expand Down Expand Up @@ -258,7 +256,7 @@ private Result<T> attempt(final Supplier<T> supplier) {
return Result.ofError(e);
} catch (final ExecutionException e) {
future.cancel(false);
if (log.isWarnEnabled()) {
if (log.isDebugEnabled()) {
log.warn("Attempt error: {}, {}", e.getMessage(), this.state);
}
final Throwable cause = e.getCause();
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/ch/ethz/seb/sebserver/gbl/util/Result.java
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ public T getOrThrow() {
if (this.error instanceof RuntimeException) {
throw (RuntimeException) this.error;
} else {
throw new RuntimeException("RuntimeExceptionWrapper cause: " + this.error.getMessage(), this.error);
String cause = this.error.getMessage() != null ? this.error.getMessage() : this.error.toString();
throw new RuntimeException("RuntimeExceptionWrapper cause: " + cause, this.error);
}
}

Expand Down
23 changes: 8 additions & 15 deletions src/main/java/ch/ethz/seb/sebserver/gui/content/exam/ExamForm.java
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ public void compose(final PageContext pageContext) {
.withURIVariable(API.PARAM_MODEL_ID, exam.getModelId())
.call()
.onError(e -> log.error("Unexpected error while trying to verify seb restriction settings: ", e))
.getOr(false);
.getOr(exam.sebRestriction);
final boolean sebRestrictionMismatch = readonly &&
sebRestrictionAvailable &&
isRestricted != exam.sebRestriction &&
Expand Down Expand Up @@ -525,7 +525,6 @@ private FormHandle<Exam> createEditForm(
final boolean newExam = exam.id == null;
final boolean hasLMS = exam.lmsSetupId != null;
final boolean importFromLMS = newExam && hasLMS;
final DateTimeZone timeZone = this.pageService.getCurrentUser().get().timeZone;
final LocTextKey statusTitle = new LocTextKey("sebserver.exam.status." + exam.status.name());

return this.pageService.formBuilder(formContext.copyOf(content))
Expand Down Expand Up @@ -585,9 +584,7 @@ private FormHandle<Exam> createEditForm(
exam.getDescription())
.asArea()
.readonly(hasLMS))
.withAdditionalValueMapping(
QuizData.QUIZ_ATTR_DESCRIPTION,
QuizData.QUIZ_ATTR_DESCRIPTION)
.withAdditionalValueMapping(QuizData.QUIZ_ATTR_DESCRIPTION)

.addField(FormBuilder.dateTime(
Domain.EXAM.ATTR_QUIZ_START_TIME,
Expand All @@ -599,21 +596,16 @@ private FormHandle<Exam> createEditForm(
.addField(FormBuilder.dateTime(
Domain.EXAM.ATTR_QUIZ_END_TIME,
FORM_END_TIME_TEXT_KEY,
exam.endTime != null
? exam.endTime
: DateTime.now(timeZone).plusHours(1))
.readonly(hasLMS)
.mandatory(!hasLMS))
exam.endTime)
.readonly(hasLMS))

.addField(FormBuilder.text(
QuizData.QUIZ_ATTR_START_URL,
FORM_QUIZ_URL_TEXT_KEY,
exam.getStartURL())
.readonly(hasLMS)
.mandatory(!hasLMS))
.withAdditionalValueMapping(
QuizData.QUIZ_ATTR_START_URL,
QuizData.QUIZ_ATTR_START_URL)
.withAdditionalValueMapping(QuizData.QUIZ_ATTR_START_URL)

.addField(FormBuilder.singleSelection(
Domain.EXAM.ATTR_TYPE,
Expand All @@ -636,15 +628,16 @@ private FormHandle<Exam> createEditForm(
}

private Exam newExamNoLMS() {
final DateTimeZone timeZone = this.pageService.getCurrentUser().get().timeZone;
return new Exam(
null,
this.pageService.getCurrentUser().get().institutionId,
null,
UUID.randomUUID().toString(),
true,
null,
null,
null,
DateTime.now(timeZone),
DateTime.now(timeZone).plusHours(1),
Exam.ExamType.UNDEFINED,
null,
null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ void build(final FormBuilder builder) {
final Composite fieldGrid = createFieldGrid(builder.formParent, this.spanInput);

if (readonly) {
final Text label = new Text(fieldGrid, SWT.NONE);
label.setText(builder.i18nSupport.formatDisplayDateTime(value) + " " + builder.i18nSupport.getUsersTimeZoneTitleSuffix());
label.setLayoutData(new GridData(SWT.FILL, SWT.TOP, true, true));
builder.form.putReadonlyField(this.name, titleLabel, label);
final Text readonlyLabel = builder.widgetFactory.textInput(fieldGrid, this.label);
readonlyLabel.setEditable(false);
readonlyLabel.setText(builder.i18nSupport.formatDisplayDateWithTimeZone(value));
readonlyLabel.setLayoutData(new GridData(SWT.FILL, SWT.TOP, true, true));
builder.form.putReadonlyField(this.name, titleLabel, readonlyLabel);
return;
}

Expand Down
9 changes: 6 additions & 3 deletions src/main/java/ch/ethz/seb/sebserver/gui/form/Form.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import ch.ethz.seb.sebserver.gbl.api.API;
import ch.ethz.seb.sebserver.gui.widget.*;
import com.fasterxml.jackson.databind.JsonNode;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.StringUtils;
import org.eclipse.rap.rwt.RWT;
Expand Down Expand Up @@ -105,6 +104,10 @@ public void putAdditionalValueMapping(final String fieldName, final String attrN
this.additionalAttributeMapping.put(fieldName, attrName);
}

public void putAdditionalValueMapping(final String fieldName) {
this.additionalAttributeMapping.put(fieldName, fieldName);
}

public String getStaticValue(final String name) {
return this.staticValues.get(name);
}
Expand Down Expand Up @@ -188,7 +191,7 @@ Form putField(final String name, final Control label, final DateTimeSelector dat
Form removeField(final String name) {
if (this.formFields.containsKey(name)) {
final List<FormFieldAccessor> list = this.formFields.remove(name);
list.forEach(ffa -> ffa.dispose());
list.forEach(FormFieldAccessor::dispose);
}

return this;
Expand Down Expand Up @@ -318,7 +321,7 @@ private void flush() {
additionalAttrs.put(entry.getValue(), fieldValue);
}
}
if (additionalAttrs != null) {
if (!additionalAttrs.isEmpty()) {
this.objectRoot.putIfAbsent(
API.PARAM_ADDITIONAL_ATTRIBUTES,
jsonMapper.valueToTree(additionalAttrs));
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/ch/ethz/seb/sebserver/gui/form/FormBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,21 @@ public FormBuilder withAdditionalValueMapping(final String fieldName, final Stri
return this;
}

public FormBuilder withAdditionalValueMapping(final String fieldName) {
this.form.putAdditionalValueMapping(fieldName);
return this;
}

public FormBuilder withAdditionalValueMappingIf(
final BooleanSupplier condition,
final Supplier<String> fieldNameSupplier) {

if (condition.getAsBoolean()) {
this.form.putAdditionalValueMapping(fieldNameSupplier.get());
}
return this;
}

public FormBuilder addFieldIf(
final BooleanSupplier condition,
final Supplier<FieldBuilder<?>> templateSupplier) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.Collection;
import java.util.Locale;

import org.apache.commons.lang3.StringUtils;
import org.joda.time.DateTime;

import ch.ethz.seb.sebserver.gbl.util.Utils;
Expand Down Expand Up @@ -61,7 +62,9 @@ public interface I18nSupport {
* @param date the DateTime instance
* @return date formatted date String to display */
default String formatDisplayDateWithTimeZone(final DateTime date) {
return formatDisplayDateTime(date) + " " + this.getUsersTimeZoneTitleSuffix();
return formatDisplayDateTime(date) + (date != null
? " " + this.getUsersTimeZoneTitleSuffix()
: StringUtils.EMPTY);
}

/** Format a time-stamp (milliseconds) to a text format to display.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public Result<Exam> initAdditionalAttributes(final Exam exam) {
EntityType.EXAM,
examId,
Exam.ADDITIONAL_ATTR_SIGNATURE_KEY_SALT,
KeyGenerators.string().generateKey().toString());
KeyGenerators.string().generateKey());

return exam;
}).flatMap(this::initAdditionalAttributesForMoodleExams);
Expand Down Expand Up @@ -204,7 +204,7 @@ public Result<Boolean> isRestricted(final Exam exam) {
return this.lmsAPIService
.getLmsAPITemplate(exam.lmsSetupId)
.map(lmsAPI -> lmsAPI.hasSEBClientRestriction(exam))
.onError(error -> log.error("Failed to check SEB restriction: ", error));
.onError(error -> log.warn("Failed to check SEB restriction: {}", error.getMessage()));
}

@Override
Expand Down Expand Up @@ -301,7 +301,7 @@ public Result<Exam> resetProctoringSettings(final Exam exam) {

return getProctoringServiceSettings(exam.id)
.map(settings -> {
ProctoringServiceSettings resetSettings;
final ProctoringServiceSettings resetSettings;
if (exam.examTemplateId != null) {
// get settings from origin template
resetSettings = this.proctoringAdminService
Expand Down Expand Up @@ -401,7 +401,7 @@ public Result<Exam> archiveExam(final Exam exam) {
.stream()
.forEach(configNodeId -> {
if (this.examConfigurationMapDAO.checkNoActiveExamReferences(configNodeId).getOr(false)) {
log.debug("Also set exam configuration to archived: ", configNodeId);
log.debug("Also set exam configuration to archived: {}", configNodeId);
this.configurationNodeDAO.save(
new ConfigurationNode(
configNodeId, null, null, null, null, null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,26 @@ public interface SEBRestrictionAPI {
* @return {@link LmsSetupTestResult } instance with the test result report */
LmsSetupTestResult testCourseRestrictionAPI();

/** Get SEB restriction data from LMS within a {@link SEBRestrictionData } instance. The available restriction
/** Get SEB restriction data from LMS within a {@link SEBRestriction } instance. The available restriction
* details
* depends on the type of LMS but shall at least contains the config-key(s) and the browser-exam-key(s).
*
* @param exam the exam to get the SEB restriction data for
* @return Result refer to the {@link SEBRestrictionData } instance or to an ResourceNotFoundException if the
* @return Result refer to the {@link SEBRestriction } instance or to an ResourceNotFoundException if the
* restriction is
* missing or to another exception on unexpected error case */
Result<SEBRestriction> getSEBClientRestriction(Exam exam);

/** Use this to check if there is a SEB restriction available on the LMS for the specified exam.
*
* <p>
* A SEB Restriction is available if it can get from LMS and if there is either a Config-Key
* or a BrowserExam-Key set or both. If none of this keys is set, the SEB Restriction is been
* considered to not set on the LMS.
*
* @param exam exam the exam to get the SEB restriction data for
* @return true if there is a SEB restriction set on the LMS for the exam or false otherwise */
default boolean hasSEBClientRestriction(final Exam exam) {
final Result<SEBRestriction> sebClientRestriction = getSEBClientRestriction(exam);
if (sebClientRestriction.hasError()) {
return false;
}

return hasSEBClientRestriction(sebClientRestriction.get());
return hasSEBClientRestriction(getSEBClientRestriction(exam).getOrThrow());
}

default boolean hasSEBClientRestriction(final SEBRestriction sebRestriction) {
Expand All @@ -58,7 +53,7 @@ default boolean hasSEBClientRestriction(final SEBRestriction sebRestriction) {
*
* @param exam The exam to apply the restriction for
* @param sebRestrictionData containing all data for SEB Client restriction to apply to the LMS
* @return Result refer to the given {@link SEBRestrictionData } if restriction was successful or to an error if
* @return Result refer to the given {@link SEBRestriction } if restriction was successful or to an error if
* not */
Result<SEBRestriction> applySEBClientRestriction(
Exam exam,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,11 @@ public LmsSetupTestResult test(final LmsAPITemplate template) {
}

if (template.lmsSetup().getLmsType().features.contains(LmsSetup.Features.SEB_RESTRICTION)) {
this.cache.remove(new CacheKey(template.lmsSetup().getModelId(), 0));
return template.testCourseRestrictionAPI();
final LmsSetupTestResult lmsSetupTestResult = template.testCourseRestrictionAPI();
if (!lmsSetupTestResult.isOk()) {
this.cache.remove(new CacheKey(template.lmsSetup().getModelId(), 0));
}
return lmsSetupTestResult;
}

return LmsSetupTestResult.ofOkay(template.lmsSetup().getLmsType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,18 +386,12 @@ public Result<SEBRestriction> getSEBClientRestriction(final Exam exam) {

return this.restrictionRequest.protectedRun(() -> this.sebRestrictionAPI
.getSEBClientRestriction(exam)
.onError(error -> log.error("Failed to get SEB restrictions: {}", error.getMessage()))
.getOrThrow());
}

@Override
public boolean hasSEBClientRestriction(final Exam exam) {
final Result<SEBRestriction> sebClientRestriction = getSEBClientRestriction(exam);
if (sebClientRestriction.hasError()) {
return false;
}

return this.sebRestrictionAPI.hasSEBClientRestriction(sebClientRestriction.get());
return this.sebRestrictionAPI.hasSEBClientRestriction(getSEBClientRestriction(exam).getOrThrow());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.Set;
import java.util.stream.Collectors;

import ch.ethz.seb.sebserver.webservice.datalayer.batis.model.AdditionalAttributeRecord;
import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -102,7 +103,7 @@ public Result<SEBRestriction> getSEBRestrictionFromExam(final Exam exam) {
.getOrThrow();

configKeys.addAll(generatedKeys);
if (generatedKeys != null && !generatedKeys.isEmpty()) {
if (!generatedKeys.isEmpty()) {
configKeys.addAll(this.lmsAPIService
.getLmsAPITemplate(exam.lmsSetupId)
.flatMap(lmsTemplate -> lmsTemplate.getSEBClientRestriction(exam))
Expand Down Expand Up @@ -130,8 +131,8 @@ public Result<SEBRestriction> getSEBRestrictionFromExam(final Exam exam) {
.collect(Collectors.toMap(
attr -> attr.getName().replace(
SEB_RESTRICTION_ADDITIONAL_PROPERTY_NAME_PREFIX,
""),
attr -> attr.getValue())));
StringUtils.EMPTY),
AdditionalAttributeRecord::getValue)));
} catch (final Exception e) {
log.error(
"Failed to load additional SEB restriction properties from AdditionalAttributes of the Exam: {}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

package ch.ethz.seb.sebserver.webservice.servicelayer.lms.impl.mockup;

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -23,6 +27,8 @@ public class MockSEBRestrictionAPI implements SEBRestrictionAPI {

private static final Logger log = LoggerFactory.getLogger(MockSEBRestrictionAPI.class);

//private Map<Long, Boolean> restrictionDB = new ConcurrentHashMap<>();

@Override
public LmsSetupTestResult testCourseRestrictionAPI() {
return LmsSetupTestResult.ofOkay(LmsType.MOCKUP);
Expand All @@ -31,15 +37,6 @@ public LmsSetupTestResult testCourseRestrictionAPI() {
@Override
public Result<SEBRestriction> getSEBClientRestriction(final Exam exam) {
log.info("Get SEB Client restriction for Exam: {}", exam);
// if (BooleanUtils.toBoolean(exam.sebRestriction)) {
// return Result.of(new SEBRestriction(
// exam.id,
// Stream.of("configKey").collect(Collectors.toList()),
// Collections.emptyList(),
// Collections.emptyMap()));
// } else {
// return Result.ofError(new NoSEBRestrictionException());
// }
return Result.ofError(new NoSEBRestrictionException());
}

Expand Down
Loading

0 comments on commit fc31059

Please sign in to comment.