Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Iris: Enable Iris for exercises based on their categories #9461

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
+475 −29
Open

Iris: Enable Iris for exercises based on their categories #9461

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
171b684
Add course settings option to enable exercise features by the categor…
Hialus Oct 13, 2024
9a2dc2c
Add client test
Hialus Oct 13, 2024
6812ddb
Feedback from Johannes
Hialus Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executors;
Expand All @@ -25,6 +26,7 @@
import de.tum.cit.aet.artemis.exam.repository.StudentExamRepository;
import de.tum.cit.aet.artemis.exercise.domain.Exercise;
import de.tum.cit.aet.artemis.exercise.repository.ExerciseRepository;
import de.tum.cit.aet.artemis.iris.service.settings.IrisSettingsService;
import de.tum.cit.aet.artemis.lecture.domain.ExerciseUnit;
import de.tum.cit.aet.artemis.lecture.repository.ExerciseUnitRepository;
import de.tum.cit.aet.artemis.lecture.service.LectureUnitService;
Expand Down Expand Up @@ -78,11 +80,14 @@ public class ExerciseDeletionService {

private final CompetencyProgressService competencyProgressService;

private final Optional<IrisSettingsService> irisSettingsService;

public ExerciseDeletionService(ExerciseRepository exerciseRepository, ExerciseUnitRepository exerciseUnitRepository, ParticipationService participationService,
ProgrammingExerciseService programmingExerciseService, ModelingExerciseService modelingExerciseService, QuizExerciseService quizExerciseService,
TutorParticipationRepository tutorParticipationRepository, ExampleSubmissionService exampleSubmissionService, StudentExamRepository studentExamRepository,
LectureUnitService lectureUnitService, PlagiarismResultRepository plagiarismResultRepository, TextExerciseService textExerciseService,
ChannelRepository channelRepository, ChannelService channelService, CompetencyProgressService competencyProgressService) {
ChannelRepository channelRepository, ChannelService channelService, CompetencyProgressService competencyProgressService,
Optional<IrisSettingsService> irisSettingsService) {
this.exerciseRepository = exerciseRepository;
this.participationService = participationService;
this.programmingExerciseService = programmingExerciseService;
Expand All @@ -98,6 +103,7 @@ public ExerciseDeletionService(ExerciseRepository exerciseRepository, ExerciseUn
this.channelRepository = channelRepository;
this.channelService = channelService;
this.competencyProgressService = competencyProgressService;
this.irisSettingsService = irisSettingsService;
}

/**
Expand Down Expand Up @@ -169,6 +175,10 @@ public void delete(long exerciseId, boolean deleteStudentReposBuildPlans, boolea
lectureUnitService.removeLectureUnit(exerciseUnit);
}

if (irisSettingsService.isPresent()) {
irisSettingsService.get().deleteSettingsFor(exercise);
}

Hialus marked this conversation as resolved.
Show resolved Hide resolved
// delete all plagiarism results belonging to this exercise
plagiarismResultRepository.deletePlagiarismResultsByExerciseId(exerciseId);

Expand Down
17 changes: 17 additions & 0 deletions src/main/java/de/tum/cit/aet/artemis/iris/domain/settings/IrisChatSubSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package de.tum.cit.aet.artemis.iris.domain.settings;

import java.util.SortedSet;
import java.util.TreeSet;

import jakarta.annotation.Nullable;
import jakarta.persistence.Column;
import jakarta.persistence.Convert;
import jakarta.persistence.DiscriminatorValue;
import jakarta.persistence.Entity;

Expand All @@ -24,6 +28,11 @@ public class IrisChatSubSettings extends IrisSubSettings {
@Column(name = "rate_limit_timeframe_hours")
private Integer rateLimitTimeframeHours;

@Nullable
@Column(name = "enabled_for_categories")
@Convert(converter = IrisListConverter.class)
private SortedSet<String> enabledForCategories = new TreeSet<>();
Hialus marked this conversation as resolved.
Show resolved Hide resolved

@Nullable
public Integer getRateLimit() {
return rateLimit;
Expand All @@ -41,4 +50,12 @@ public Integer getRateLimitTimeframeHours() {
public void setRateLimitTimeframeHours(@Nullable Integer rateLimitTimeframeHours) {
this.rateLimitTimeframeHours = rateLimitTimeframeHours;
}

public SortedSet<String> getEnabledForCategories() {
return enabledForCategories;
}
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved

public void setEnabledForCategories(SortedSet<String> enabledForCategories) {
this.enabledForCategories = enabledForCategories;
}
Hialus marked this conversation as resolved.
Show resolved Hide resolved
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
}
17 changes: 17 additions & 0 deletions ...ain/java/de/tum/cit/aet/artemis/iris/domain/settings/IrisTextExerciseChatSubSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package de.tum.cit.aet.artemis.iris.domain.settings;

import java.util.SortedSet;
import java.util.TreeSet;
Comment on lines +3 to +4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider organizing imports.

The new imports for SortedSet and TreeSet are added at the beginning of the import block. While this doesn't affect functionality, it's generally a good practice to group imports by their packages.

Consider reorganizing the imports as follows:

import java.util.SortedSet;
import java.util.TreeSet;

import jakarta.annotation.Nullable;
import jakarta.persistence.Column;
import jakarta.persistence.Convert;
import jakarta.persistence.DiscriminatorValue;
import jakarta.persistence.Entity;

import com.fasterxml.jackson.annotation.JsonInclude;


import jakarta.annotation.Nullable;
import jakarta.persistence.Column;
import jakarta.persistence.Convert;
import jakarta.persistence.DiscriminatorValue;
import jakarta.persistence.Entity;

Expand All @@ -23,6 +27,11 @@ public class IrisTextExerciseChatSubSettings extends IrisSubSettings {
@Column(name = "rate_limit_timeframe_hours")
private Integer rateLimitTimeframeHours;

@Nullable
@Column(name = "enabled_for_categories")
@Convert(converter = IrisListConverter.class)
private SortedSet<String> enabledForCategories = new TreeSet<>();
Comment on lines +30 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider removing default initialization or @Nullable annotation.

The enabledForCategories field is annotated with @Nullable, but it's also initialized with a new TreeSet. This could lead to confusion as the field will never be null unless explicitly set to null.

Consider one of the following options:

  1. Remove the @Nullable annotation if the field should never be null.
  2. Remove the default initialization if null is a valid state for this field.

If you choose option 1, update the field declaration as follows:

@Column(name = "enabled_for_categories")
@Convert(converter = IrisListConverter.class)
private SortedSet<String> enabledForCategories = new TreeSet<>();

If you choose option 2, update the field declaration as follows:

@Nullable
@Column(name = "enabled_for_categories")
@Convert(converter = IrisListConverter.class)
private SortedSet<String> enabledForCategories;


@Nullable
public Integer getRateLimit() {
return rateLimit;
Expand All @@ -41,4 +50,12 @@ public void setRateLimitTimeframeHours(@Nullable Integer rateLimitTimeframeHours
this.rateLimitTimeframeHours = rateLimitTimeframeHours;
}

@Nullable
public SortedSet<String> getEnabledForCategories() {
return enabledForCategories;
}
Comment on lines +53 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider returning an unmodifiable view of the set.

The current implementation returns the enabledForCategories set directly, which could allow external code to modify the internal state of the object. To maintain encapsulation and prevent unintended modifications, consider returning an unmodifiable view of the set.

Update the getter method as follows:

@Nullable
public SortedSet<String> getEnabledForCategories() {
    return enabledForCategories != null ? Collections.unmodifiableSortedSet(enabledForCategories) : null;
}

Don't forget to add the following import:

import java.util.Collections;


public void setEnabledForCategories(@Nullable SortedSet<String> enabledForCategories) {
this.enabledForCategories = enabledForCategories;
}
Comment on lines +58 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Create a defensive copy in the setter method.

The current implementation directly assigns the input set to the field. This could lead to unexpected behavior if the caller retains a reference to the set and modifies it after calling this method. To prevent this, consider creating a defensive copy of the input set.

Update the setter method as follows:

public void setEnabledForCategories(@Nullable SortedSet<String> enabledForCategories) {
    this.enabledForCategories = enabledForCategories != null ? new TreeSet<>(enabledForCategories) : null;
}

}
2 changes: 1 addition & 1 deletion src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisCombinedChatSubSettingsDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@

@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record IrisCombinedChatSubSettingsDTO(boolean enabled, Integer rateLimit, Integer rateLimitTimeframeHours, @Nullable Set<String> allowedVariants,
@Nullable String selectedVariant) {
@Nullable String selectedVariant, @Nullable Set<String> enabledForCategories) {
Hialus marked this conversation as resolved.
Show resolved Hide resolved

}
2 changes: 1 addition & 1 deletion ...main/java/de/tum/cit/aet/artemis/iris/dto/IrisCombinedTextExerciseChatSubSettingsDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@

@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record IrisCombinedTextExerciseChatSubSettingsDTO(boolean enabled, Integer rateLimit, Integer rateLimitTimeframeHours, @Nullable Set<String> allowedVariants,
@Nullable String selectedVariant) {
@Nullable String selectedVariant, @Nullable Set<String> enabledForCategories) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Consider adding a brief comment for the new field.

The addition of the enabledForCategories field is well-implemented and consistent with the existing code structure. It adheres to the coding guidelines, including proper naming conventions and the use of Java records for DTOs.

Consider adding a brief comment above the record declaration to explain the purpose of the enabledForCategories field. This would enhance code readability and maintainability. For example:

/**
 * DTO for combined text exercise chat sub-settings.
 * @param enabledForCategories Set of category identifiers for which this setting is enabled
 */
public record IrisCombinedTextExerciseChatSubSettingsDTO(...)


}
152 changes: 151 additions & 1 deletion src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.function.Supplier;

Expand All @@ -18,6 +20,9 @@
import org.springframework.context.event.EventListener;
import org.springframework.stereotype.Service;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

import de.tum.cit.aet.artemis.core.domain.Course;
import de.tum.cit.aet.artemis.core.domain.User;
import de.tum.cit.aet.artemis.core.exception.AccessForbiddenAlertException;
Expand All @@ -37,6 +42,10 @@
import de.tum.cit.aet.artemis.iris.domain.settings.IrisTextExerciseChatSubSettings;
import de.tum.cit.aet.artemis.iris.dto.IrisCombinedSettingsDTO;
import de.tum.cit.aet.artemis.iris.repository.IrisSettingsRepository;
import de.tum.cit.aet.artemis.programming.domain.ProgrammingExercise;
import de.tum.cit.aet.artemis.programming.repository.ProgrammingExerciseRepository;
import de.tum.cit.aet.artemis.text.domain.TextExercise;
import de.tum.cit.aet.artemis.text.repository.TextExerciseRepository;

/**
* Service for managing {@link IrisSettings}.
Expand All @@ -55,10 +64,20 @@ public class IrisSettingsService {

private final AuthorizationCheckService authCheckService;

public IrisSettingsService(IrisSettingsRepository irisSettingsRepository, IrisSubSettingsService irisSubSettingsService, AuthorizationCheckService authCheckService) {
private final ProgrammingExerciseRepository programmingExerciseRepository;

private final ObjectMapper objectMapper;

private final TextExerciseRepository textExerciseRepository;

public IrisSettingsService(IrisSettingsRepository irisSettingsRepository, IrisSubSettingsService irisSubSettingsService, AuthorizationCheckService authCheckService,
ProgrammingExerciseRepository programmingExerciseRepository, ObjectMapper objectMapper, TextExerciseRepository textExerciseRepository) {
this.irisSettingsRepository = irisSettingsRepository;
this.irisSubSettingsService = irisSubSettingsService;
this.authCheckService = authCheckService;
this.programmingExerciseRepository = programmingExerciseRepository;
this.objectMapper = objectMapper;
this.textExerciseRepository = textExerciseRepository;
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -248,6 +267,11 @@ private IrisGlobalSettings updateGlobalSettings(IrisGlobalSettings existingSetti
* @return The updated course Iris settings
*/
private IrisCourseSettings updateCourseSettings(IrisCourseSettings existingSettings, IrisCourseSettings settingsUpdate) {
var oldEnabledForCategoriesExerciseChat = existingSettings.getIrisChatSettings() == null ? new TreeSet<String>()
: existingSettings.getIrisChatSettings().getEnabledForCategories();
var oldEnabledForCategoriesTextExerciseChat = existingSettings.getIrisTextExerciseChatSettings() == null ? new TreeSet<String>()
: existingSettings.getIrisTextExerciseChatSettings().getEnabledForCategories();

coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
var parentSettings = getCombinedIrisGlobalSettings();
// @formatter:off
existingSettings.setIrisChatSettings(irisSubSettingsService.update(
Expand Down Expand Up @@ -276,9 +300,135 @@ private IrisCourseSettings updateCourseSettings(IrisCourseSettings existingSetti
));
// @formatter:on

// Automatically update the exercise settings when the enabledForCategories is changed
var newEnabledForCategoriesExerciseChat = existingSettings.getIrisChatSettings() == null ? new TreeSet<String>()
: existingSettings.getIrisChatSettings().getEnabledForCategories();
if (!Objects.equals(oldEnabledForCategoriesExerciseChat, newEnabledForCategoriesExerciseChat)) {
Hialus marked this conversation as resolved.
Show resolved Hide resolved
programmingExerciseRepository.findAllWithCategoriesByCourseId(existingSettings.getCourse().getId())
.forEach(exercise -> setEnabledForExerciseByCategories(exercise, oldEnabledForCategoriesExerciseChat, newEnabledForCategoriesExerciseChat));
Hialus marked this conversation as resolved.
Show resolved Hide resolved
}

var newEnabledForCategoriesTextExerciseChat = existingSettings.getIrisTextExerciseChatSettings() == null ? new TreeSet<String>()
: existingSettings.getIrisTextExerciseChatSettings().getEnabledForCategories();
if (!Objects.equals(oldEnabledForCategoriesTextExerciseChat, newEnabledForCategoriesTextExerciseChat)) {
textExerciseRepository.findAllWithCategoriesByCourseId(existingSettings.getCourse().getId())
.forEach(exercise -> setEnabledForExerciseByCategories(exercise, oldEnabledForCategoriesTextExerciseChat, newEnabledForCategoriesTextExerciseChat));
}

coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
return irisSettingsRepository.save(existingSettings);
}

/**
* Set the enabled status for an exercise based on it's categories.
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
* Compares the old and new enabled categories, reads the exercise categories,
* and updates the Iris chat settings accordingly if the new enabled categories match any of the exercise categories.
* This method is used when the enabled categories of the course settings are updated.
*
* @param exercise The exercise to update the enabled status for
* @param oldEnabledForCategories The old enabled categories
* @param newEnabledForCategories The new enabled categories
*/
public void setEnabledForExerciseByCategories(Exercise exercise, SortedSet<String> oldEnabledForCategories, SortedSet<String> newEnabledForCategories) {
var removedCategories = new TreeSet<>(oldEnabledForCategories == null ? Set.of() : oldEnabledForCategories);
removedCategories.removeAll(newEnabledForCategories);
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
Hialus marked this conversation as resolved.
Show resolved Hide resolved
var categories = getCategoryNames(exercise.getCategories());

if (newEnabledForCategories != null && categories.stream().anyMatch(newEnabledForCategories::contains)) {
var exerciseSettings = getRawIrisSettingsFor(exercise);
if (exercise instanceof ProgrammingExercise) {
exerciseSettings.getIrisChatSettings().setEnabled(true);
}
else if (exercise instanceof TextExercise) {
exerciseSettings.getIrisTextExerciseChatSettings().setEnabled(true);
}
irisSettingsRepository.save(exerciseSettings);
}
else if (categories.stream().anyMatch(removedCategories::contains)) {
var exerciseSettings = getRawIrisSettingsFor(exercise);
if (exercise instanceof ProgrammingExercise) {
exerciseSettings.getIrisChatSettings().setEnabled(false);
}
else if (exercise instanceof TextExercise) {
exerciseSettings.getIrisTextExerciseChatSettings().setEnabled(false);
}
irisSettingsRepository.save(exerciseSettings);
}
Hialus marked this conversation as resolved.
Show resolved Hide resolved
}
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved

/**
* Set the enabled status for an exercise based on its categories.
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
* Reads the exercise categories and updates the Iris chat settings accordingly if the enabled categories match any of the exercise categories.
* This method is used when the categories of an exercise are updated.
*
* @param exercise The exercise to update the enabled status for
* @param oldExerciseCategories The old exercise categories
*/
public void setEnabledForExerciseByCategories(Exercise exercise, Set<String> oldExerciseCategories) {
var oldCategories = getCategoryNames(oldExerciseCategories);
var newCategories = getCategoryNames(exercise.getCategories());
if (oldCategories.isEmpty() && newCategories.isEmpty()) {
return;
}

var course = exercise.getCourseViaExerciseGroupOrCourseMember();
var courseSettings = getRawIrisSettingsFor(course);

Set<String> enabledForCategories;
if (exercise instanceof ProgrammingExercise) {
enabledForCategories = courseSettings.getIrisChatSettings().getEnabledForCategories();
}
else if (exercise instanceof TextExercise) {
enabledForCategories = courseSettings.getIrisTextExerciseChatSettings().getEnabledForCategories();
}
else {
return;
}
if (enabledForCategories == null) {
return;
}

if (newCategories.stream().anyMatch(enabledForCategories::contains)) {
var exerciseSettings = getRawIrisSettingsFor(exercise);
if (exercise instanceof ProgrammingExercise) {
exerciseSettings.getIrisChatSettings().setEnabled(true);
}
else if (exercise instanceof TextExercise) {
exerciseSettings.getIrisTextExerciseChatSettings().setEnabled(true);
}
irisSettingsRepository.save(exerciseSettings);
}
else if (oldCategories.stream().anyMatch(enabledForCategories::contains)) {
var exerciseSettings = getRawIrisSettingsFor(exercise);
if (exercise instanceof ProgrammingExercise) {
exerciseSettings.getIrisChatSettings().setEnabled(false);
}
else if (exercise instanceof TextExercise) {
exerciseSettings.getIrisTextExerciseChatSettings().setEnabled(false);
}
irisSettingsRepository.save(exerciseSettings);
}
}
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved

/**
* Convert the category JSON strings of an exercise to a set of category names.
*
* @param exerciseCategories The set of category JSON strings
* @return The set of category names
*/
private Set<String> getCategoryNames(Set<String> exerciseCategories) {
var categories = new HashSet<String>();
for (var categoryJson : exerciseCategories) {
try {
var category = objectMapper.readTree(categoryJson);
categories.add(category.get("category").asText());
}
catch (JsonProcessingException e) {
return new HashSet<>();
}
}
return categories;
}
coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved

/**
* Helper method to update exercise Iris settings.
*
Expand Down
Loading
Loading