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.

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

Iris: Improve event handling for predictable scenarios #10025

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

kaancayli
Copy link
Contributor

@kaancayli kaancayli commented Dec 15, 2024

Checklist

General

Server

Motivation and Context

Currently, when an event is triggered for a disabled feature (e.g., Iris disabled for an exercise), the event system throws an exception. However, since disabled features represent a predictable condition, we should handle such cases more gracefully. This PR introduces proper validation and logging for disabled events, providing better visibility of these occurrences without raising exceptions.

Resolves ARTEMIS-241

Description

Refactors the event system to utilize the Spring's ApplicationEventPublisher and ApplicationListener pattern. Also replaces the exceptions with logging for disabled events

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  • A test server with Iris enabled
  • 1 Programming Exercise with Iris enabled
  • Iris terms and conditions are already accepted

First scenario

Additional prerequisites:

  • Monitor student submission build failures under Iris Chat Settings Disabled
  • Monitor exercise performance progress under Iris Chat Settings Disabled
  1. Log in to Artemis
  2. Navigate to the programming exercise with Iris enabled
  3. Testing events:
    1. Build failed event:

      • Make a programming exercise submission with a failing build
      • Verify that no exception is thrown. You can monitor the logs on Grafana.
    2. Progress stalled event:

      • Make 3 consequent programming exercise submissions
      • Make sure the scores after each submission are not increasing (e.g., you can submit the initial repository 3 times)
      • Verify that no exception is thrown. You can monitor the logs on Grafana.

Second scenario

Additional prerequisites:

  • Monitor student submission build failures under Iris Chat Settings Enabled
  • Monitor exercise performance progress under Iris Chat Settings Enabled
  1. Log in to Artemis
  2. Navigate to the programming exercise with Iris enabled
  3. Testing events:
    1. Build failed event:

      • Make a programming exercise submission with a failing build
      • Verify that Iris reacted to the event and responded with a proactive message
    2. Progress stalled event:

      • Make 3 consequent programming exercise submissions
      • Make sure the scores after each submission are not increasing (e.g., you can submit the initial repository 3 times)
      • Verify that Iris reacted to the event and responded with a proactive message

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a new service for publishing Pyris events, enhancing event handling within the application.
    • Updated event handling logic for build failures and new results, improving clarity and efficiency.
  • Bug Fixes

    • Streamlined error handling for event activation checks, reducing reliance on exceptions.
  • Refactor

    • Removed outdated methods and consolidated functionality for better maintainability.
    • Renamed and updated references to the event service for consistency across the application.
    • Adjusted event processing methods to enhance clarity and separation of concerns.
  • Documentation

    • Enhanced logging capabilities for better monitoring of feature activation states and event processing.

@kaancayli kaancayli requested a review from a team as a code owner December 15, 2024 08:35
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) iris Pull requests that affect the corresponding module labels Dec 15, 2024
Copy link

coderabbitai bot commented Dec 15, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 pmd (7.8.0)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java

The following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.

Walkthrough

The pull request introduces significant changes to the event handling system in the Artemis application, focusing on the Iris and Pyris services. The modifications include replacing the existing PyrisEventService with a new PyrisEventPublisherService, updating event classes to extend ApplicationEvent, and refactoring event processing methods. The changes aim to streamline event management, improve logging, and provide more flexible event handling across various services like course chat sessions, programming submissions, and competency judgement of learning events.

Changes

File Change Summary
.../IrisExerciseChatSessionService.java Simplified event handling logic, removed exception-based checks for event activation.
.../IrisSettingsService.java Removed isActivatedForElseThrow methods, added logging for event activation states.
.../PyrisEventSystemIntegrationTest.java Updated event triggering mechanism, removed exception-based assertions.
.../CompetencyJolService.java Replaced PyrisEventService with PyrisEventPublisherService.
.../PyrisEventService.java Removed generic trigger method, added specific event handler methods.
.../CompetencyJolSetEvent.java Updated event class structure, added user retrieval method.
.../NewResultEvent.java Simplified event class, added result and user retrieval methods.
.../PyrisEvent.java Removed generic parameters, changed to extend ApplicationEvent.
.../IrisCourseChatSessionService.java Modified session handling and feature activation checks.
.../ProgrammingMessagingService.java Updated event service references.
.../PyrisEventPublisherService.java New service for publishing events with conditional activation.

Possibly related PRs

  • Iris: Allow team repository access for Iris #9975: This PR modifies the IrisExerciseChatSessionService class, which is directly related to the main PR's changes in the same class, enhancing the logic for retrieving student participations in team exercises.

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 097e2ed and 73048c8.

📒 Files selected for processing (1)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 15, 2024
@kaancayli kaancayli changed the title Iris: Imprive error handling when triggering events Iris: Improve error handling when triggering events Dec 15, 2024
@kaancayli kaancayli changed the title Iris: Improve error handling when triggering events Iris: Replace exceptions with logging for disabled event checks Dec 15, 2024
@kaancayli kaancayli requested review from krusche and bassner December 15, 2024 08:40
@github-actions github-actions bot added atlas Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Dec 15, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (9)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (1)

Line range hint 87-94: Consider enhancing error logging and event creation.

The changes correctly implement the new event publishing approach and align with the PR's objective of replacing exceptions with logging. However, there are opportunities for improvement:

  1. Make the log message more specific by including context:
-                log.warn("Something went wrong while sending the judgement of learning to Iris", e);
+                log.warn("Failed to publish CompetencyJolSetEvent for competency {} and user {}", competencyId, userId, e);
  1. Consider extracting event creation to a separate method for better testability:
+    private CompetencyJolSetEvent createJolSetEvent(CompetencyJol jol) {
+        return new CompetencyJolSetEvent(this, jol);
+    }
+
     public void setJudgementOfLearning(long competencyId, long userId, short jolValue) {
         // ... existing code ...
         pyrisEventPublisher.ifPresent(service -> {
             try {
-                service.publishEvent(new CompetencyJolSetEvent(this, jol));
+                service.publishEvent(createJolSetEvent(jol));
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/CompetencyJolSetEvent.java (2)

10-18: Consider adding Javadoc for constructor parameters.

While the implementation is correct, adding parameter documentation would improve code clarity.

+    /**
+     * Creates a new CompetencyJolSetEvent.
+     * @param source The object on which the event initially occurred
+     * @param competencyJol The competency judgment of learning to be processed
+     * @throws IllegalArgumentException if competencyJol is null
+     */
     public CompetencyJolSetEvent(Object source, CompetencyJol competencyJol) {

20-26: LGTM! Consider adding @nullable annotation for clarity.

The implementation correctly uses Optional for null safety. Consider adding @Nullable annotation to the competencyJol.getUser() call to make the potential null value explicit.

     @Override
     public Optional<User> getUser() {
-        return Optional.ofNullable(competencyJol.getUser());
+        return Optional.ofNullable(competencyJol.getUser()); // @Nullable
     }
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisher.java (3)

42-55: Improve exception handling in publishEvent method

Catching the generic Exception and rethrowing it may obscure the original context. Consider catching specific exceptions or wrapping them in a custom exception to provide clearer error information.


77-105: Refactor switch expression for scalability

As new event types are introduced, the current switch expression may become unwieldy. Consider implementing a strategy pattern or an event handler registry to enhance maintainability and scalability.


45-45: Adjust logging level for skipped events

Using log.debug may hide important information in production. If skipping event publication is significant, consider using log.info to increase visibility.

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)

52-64: Consider handling cases where result.getSubmission() returns null

If result.getSubmission() returns null, the current code does not process the event and lacks logging or notification for this case. Consider adding a null check for submission and handling it appropriately, perhaps by logging a warning.

Apply this diff to add a null check:

 public void handleNewResultEvent(NewResultEvent event) {
     log.debug("Processing NewResultEvent");
     try {
         var result = event.getResult();
         var submission = result.getSubmission();

+        if (submission == null) {
+            log.warn("Result {} has null submission", result.getId());
+            return;
+        }

         if (submission instanceof ProgrammingSubmission programmingSubmission) {
             if (programmingSubmission.isBuildFailed()) {
                 irisExerciseChatSessionService.onBuildFailure(result);
             }
             else {
                 irisExerciseChatSessionService.onNewResult(result);
             }
         }
         log.debug("Successfully processed NewResultEvent");
     }
     catch (Exception e) {
         log.error("Failed to process NewResultEvent: {}", event, e);
         throw e;
     }
 }
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)

197-202: Rename lambda parameter for clarity

In the lambda expression, the parameter is named service, but it refers to a PyrisEventPublisher. Rename it to publisher for better readability.

Apply this diff:

- pyrisEventPublisher.ifPresent(service -> {
+ pyrisEventPublisher.ifPresent(publisher -> {
     try {
-        service.publishEvent(new NewResultEvent(this, result));
+        publisher.publishEvent(new NewResultEvent(this, result));
     }
     catch (Exception e) {
         log.error("Could not publish event for result {}", result.getId(), e);
     }
 });
src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (1)

743-745: Consider refactoring repetitive event checking logic

The current implementation has repetitive code blocks for each event type. Consider refactoring to reduce duplication and improve maintainability.

Here's a suggested refactoring:

    private boolean isEventEnabledInSettings(IrisCombinedSettingsDTO settings, IrisEventType type) {
+       if (settings.irisChatSettings().disabledProactiveEvents() == null) {
+           return true;
+       }
+       var isEventEnabled = !settings.irisChatSettings().disabledProactiveEvents()
+           .contains(type.name().toLowerCase());
+       log.debug("Event {} enabled: {}", type.name(), isEventEnabled);
+       return isEventEnabled;
-        return switch (type) {
-            case PROGRESS_STALLED -> {
-                if (settings.irisChatSettings().disabledProactiveEvents() != null) {
-                    var isEventEnabled = !settings.irisChatSettings().disabledProactiveEvents().contains(IrisEventType.PROGRESS_STALLED.name().toLowerCase());
-                    log.debug("Event PROGRESS_STALLED enabled: {}", isEventEnabled);
-                    yield isEventEnabled;
-                }
-                else {
-                    yield true;
-                }
-            }
-            case BUILD_FAILED -> {
-                // ... similar code
-            }
-            case JOL -> {
-                // ... similar code
-            }
-        };
    }

This refactoring:

  1. Eliminates code duplication
  2. Makes the code more maintainable
  3. Reduces the chance of bugs when adding new event types
  4. Makes the logging consistent across all event types

Also applies to: 753-760, 761-765

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c48f343 and c4021ed.

📒 Files selected for processing (11)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (3 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisher.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/CompetencyJolSetEvent.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/PyrisEvent.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (0 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (0 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (5 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (4 hunks)
  • src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (10 hunks)
💤 Files with no reviewable changes (2)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
  • src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java
🧰 Additional context used
📓 Path-based instructions (9)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/PyrisEvent.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/CompetencyJolSetEvent.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisher.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (16)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (3)

26-26: LGTM: Import change aligns with the new event publishing approach.

The import change correctly reflects the transition from PyrisEventService to PyrisEventPublisher.


48-56: LGTM: Dependency injection changes follow best practices.

The changes maintain proper dependency injection patterns:

  • Uses constructor injection (following guidelines)
  • Keeps the Optional wrapper for the event publisher
  • Maintains clean initialization in the constructor

Line range hint 87-94: Verify consistent event publishing implementation across the codebase.

Let's ensure the new event publishing approach is consistently implemented across related components.

✅ Verification successful

Based on the results, I can generate the final response as we have sufficient information to verify the consistency of event publishing implementation:

Event publishing implementation is consistent across the codebase

The verification shows:

  • PyrisEventService is properly contained in its service class and test file
  • Consistent error handling pattern is used across services (CompetencyJolService and ProgrammingMessagingService)
  • Both implementations follow the same pattern:
    • Using pyrisEventPublisher.ifPresent
    • Wrapping event publishing in try-catch blocks
    • Proper error logging
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent implementation of PyrisEventPublisher across the codebase

# Check for any remaining PyrisEventService usages that might need updating
echo "Checking for remaining PyrisEventService usages..."
rg "PyrisEventService" -l

# Check for consistent event publishing pattern
echo "Checking event publishing patterns..."
ast-grep --pattern 'publishEvent(new CompetencyJolSetEvent($$$))'

# Check for proper error handling pattern
echo "Checking error handling patterns..."
rg "pyrisEventPublisher.ifPresent" -A 5

Length of output: 2357

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/CompetencyJolSetEvent.java (2)

3-8: LGTM! Clean class structure with minimal imports.

The class structure follows best practices with specific imports and clear inheritance hierarchy.


8-26: Verify integration with PyrisEventPublisher.

Let's ensure this event class is properly integrated with the new event publishing system mentioned in the PR.

✅ Verification successful

Integration with PyrisEventPublisher is properly implemented

The event class is correctly integrated with the new event publishing system:

  • PyrisEventPublisher handles CompetencyJolSetEvent in its switch expression, checking if JOL events are activated for the course
  • PyrisEventService processes the event with proper logging (debug/error) instead of throwing exceptions
  • CompetencyJolService publishes the event through PyrisEventPublisher with error logging
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CompetencyJolSetEvent usage in PyrisEventPublisher
ast-grep --pattern 'class PyrisEventPublisher {
  $$$
  publishEvent($_, CompetencyJolSetEvent $_) {
    $$$
  }
  $$$
}'

# Check if there are any remaining exception throws for disabled events
rg -A 5 "CompetencyJolSetEvent.*throw.*Exception" --type java

Length of output: 311


Script:

#!/bin/bash
# Let's try a broader search to understand the event publishing system
# First, find all references to CompetencyJolSetEvent
rg "CompetencyJolSetEvent" --type java -C 3

# Then, look for the general event publishing pattern
ast-grep --pattern 'publishEvent($event)'

# Also search for PyrisEventPublisher class definition
rg "class PyrisEventPublisher" --type java -C 5

Length of output: 9261

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/PyrisEvent.java (2)

12-16: Class refactoring aligns with event handling architecture

Extending ApplicationEvent and updating the constructor enhances integration with Spring's event system.


23-23: Ensure all subclasses implement the getUser() method

As getUser() is now an abstract method, verify that all subclasses of PyrisEvent implement it appropriately to return the associated User.

Run the following script to confirm that all subclasses of PyrisEvent implement the getUser() method:

✅ Verification successful

All subclasses correctly implement the getUser() method

Both identified subclasses of PyrisEvent have properly implemented the abstract getUser() method:

  • CompetencyJolSetEvent returns the user from the associated CompetencyJol
  • NewResultEvent returns the student from the programming exercise participation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all subclasses of PyrisEvent implement getUser()

# Find all subclasses of PyrisEvent
fd --extension java --type f | xargs grep -l 'extends PyrisEvent' > subclasses.txt

# Check if each subclass implements getUser()
while IFS= read -r file; do
  if ! grep -q 'public Optional<User> getUser()' "$file"; then
    echo "Class in $file does not implement getUser() method."
  fi
done < subclasses.txt

Length of output: 392


Script:

#!/bin/bash
# Let's try a different approach using ast-grep to find all classes extending PyrisEvent
# and their implementation of getUser()

# First, find all classes extending PyrisEvent
ast-grep --pattern 'class $_ extends PyrisEvent'

# Then, let's examine the content of these files
echo "=== Content of CompetencyJolSetEvent.java ==="
cat src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/CompetencyJolSetEvent.java

echo -e "\n=== Content of NewResultEvent.java ==="
cat src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java

Length of output: 2288

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisher.java (1)

32-35: Constructor uses proper dependency injection

Dependencies are correctly injected via constructor, adhering to best practices.

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (2)

13-19: Constructor logic is correct

The constructor properly initializes the result field and validates that it is not null.


21-23: Getter method for result is correctly implemented

The getResult() method correctly returns the result field.

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)

35-46: Event handling for CompetencyJolSetEvent is correctly implemented

The handleCompetencyJolSetEvent method processes the event with appropriate logging and error handling.

src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)

254-257: Tests correctly updated to reflect new event handling logic

The test testShouldNotFireProgressStalledEventWithEventDisabled properly verifies that the event is not processed when disabled, and method calls are correctly adjusted.


271-274: Tests correctly updated to reflect new event handling logic

The test testShouldNotFireBuildFailedEventWhenEventSettingDisabled accurately checks that no methods are called when the event is disabled, aligning with the new implementation.

src/main/java/de/tum/cit/aet/artemis/iris/service/settings/IrisSettingsService.java (3)

18-19: LGTM: Logger initialization follows best practices

The logger is correctly initialized using SLF4J's LoggerFactory with proper access modifiers and naming conventions.

Also applies to: 65-66


515-518: LGTM: Successfully replaced exception with logging for course chat check

The implementation aligns with the PR objectives by replacing exception throwing with logging and early return. The log message includes the course ID for better traceability.


531-534: LGTM: Successfully replaced exception with logging for exercise chat check

The implementation aligns with the PR objectives by replacing exception throwing with logging and early return. The log message includes the exercise ID for better traceability.

ole-ve
ole-ve previously approved these changes Dec 16, 2024
Copy link
Contributor

@ole-ve ole-ve left a comment

Choose a reason for hiding this comment

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

code looks good, just added two smaller suggestions

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (2)

49-52: Consider catching specific exceptions instead of generic Exception

Catching the generic Exception can mask other exceptions and make error handling less precise. Consider catching more specific exceptions that eventPublisher.publishEvent(event) might throw, such as IllegalArgumentException or IllegalStateException.


55-83: Refactor isEventEnabled method for improved readability

The isEventEnabled method contains complex logic within its switch cases. To enhance readability and adhere to the single responsibility principle, consider extracting the logic for each event type into separate private methods.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c269687 and 77a10be.

📒 Files selected for processing (4)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (3 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (4 hunks)
  • src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java
🧰 Additional context used
📓 Path-based instructions (2)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (9)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (9)

42-42: Import statement correctly adds the required class

The import of PyrisEventPublisherService ensures that the new event publisher service is available in the test class.


65-67: Usage of pyrisEventService needs verification

Please verify if pyrisEventService is still needed in this class. If it's no longer used after transitioning to PyrisEventPublisherService, consider removing it to clean up the code.


78-78: Autowired pyrisEventPublisherService is properly added

The new PyrisEventPublisherService is correctly autowired, enabling the test methods to use the updated event publishing mechanism.


196-196: Invocation of publishEvent method is correct

The publishEvent method is appropriately used to publish the NewResultEvent, aligning with the new event handling approach.


254-257: Test correctly verifies no event is published when disabled

The test ensures that when the event is disabled, onNewResult and executeExerciseChatPipeline are not called, which is the expected behavior.


271-274: Test accurately checks handling of disabled build failed events

Verifications confirm that no actions are taken when the build failed event is disabled, which aligns with the updated logic.


288-294: Consistent use of publishEvent in multiple submissions

The publishEvent method is consistently used in the test for multiple submissions, ensuring the event handling is thoroughly tested.


309-309: Event publishing correctly implemented for less than three submissions

The test case accurately reflects the scenario where the progress stalled event should not be fired due to insufficient submissions.


324-324: Proper handling of increasing scores in submissions

The test verifies that no progress stalled event is fired when submission scores are increasing, which is the intended behavior.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)

248-254: Enhance test coverage for unsupported events

While the test verifies basic exception handling, consider:

  1. Testing multiple types of unsupported events
  2. Verifying the complete exception message
  3. Adding test cases for edge cases (null event source, null user)
-        assertThatExceptionOfType(UnsupportedPyrisEventException.class).isThrownBy(() -> pyrisEventPublisherService.publishEvent(new PyrisEvent(this) {
-            @Override
-            public Optional<User> getUser() {
-                return Optional.empty();
-            }
-        })).withMessageStartingWith("Unsupported Pyris event");
+        // Test basic unsupported event
+        var basicEvent = new PyrisEvent(this) {
+            @Override
+            public Optional<User> getUser() {
+                return Optional.empty();
+            }
+        };
+        assertThatExceptionOfType(UnsupportedPyrisEventException.class)
+            .isThrownBy(() -> pyrisEventPublisherService.publishEvent(basicEvent))
+            .withMessage("Unsupported Pyris event: " + basicEvent.getClass().getSimpleName());
+
+        // Test null source
+        var nullSourceEvent = new PyrisEvent(null) {
+            @Override
+            public Optional<User> getUser() {
+                return Optional.empty();
+            }
+        };
+        assertThatExceptionOfType(UnsupportedPyrisEventException.class)
+            .isThrownBy(() -> pyrisEventPublisherService.publishEvent(nullSourceEvent));

Line range hint 1-371: Improve test class documentation and structure

While the test implementation is solid, consider these improvements:

  1. Add class-level JavaDoc explaining the test scope and requirements
  2. Extract common test setup into helper methods to reduce duplication
  3. Group related tests using @nested classes
  4. Add @DisplayName annotations for better test reporting

Example for test grouping:

@DisplayName("PyrisEventSystem Integration Tests")
class PyrisEventSystemIntegrationTest extends AbstractIrisIntegrationTest {
    @Nested
    @DisplayName("Progress Stalled Events")
    class ProgressStalledEventTests {
        // Group all progress stalled related tests
    }

    @Nested
    @DisplayName("Build Failed Events")
    class BuildFailedEventTests {
        // Group all build failed related tests
    }
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77a10be and bc5d3db.

📒 Files selected for processing (2)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)

198-198: LGTM: Consistent migration to ApplicationEvent pattern

The changes consistently replace direct event triggering with Spring's ApplicationEvent publishing mechanism, properly including the event source.

Also applies to: 218-218, 269-269, 286-286, 303-303, 309-309, 324-324, 339-339


67-69: Clarify the need for both PyrisEventService and PyrisEventPublisherService

The presence of both services suggests an incomplete migration to the publisher-subscriber pattern. Consider:

  1. If PyrisEventService is being phased out, mark it as @deprecated
  2. If both are needed, add documentation explaining their distinct responsibilities

Also applies to: 80-80

@kaancayli kaancayli requested a review from ole-ve December 16, 2024 18:39
ole-ve
ole-ve previously approved these changes Dec 16, 2024
Copy link
Contributor

@ole-ve ole-ve left a comment

Choose a reason for hiding this comment

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

thanks for applying my suggestions, code looks good

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1)

62-109: Refactor 'isEventEnabled' method to enhance readability and maintainability

The isEventEnabled method contains multiple nested conditions and repetitive null checks, which can make the code harder to read and maintain. Consider refactoring to simplify the logic and reduce nesting.

You could extract the null checks and event-specific logic into separate private methods or utilize helper functions to improve clarity. This refactoring would adhere to the Single Responsibility Principle and enhance the overall code quality.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc5d3db and 097e2ed.

📒 Files selected for processing (4)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (11 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventPublisherService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/event/NewResultEvent.java (2)

13-19: Constructor null check for 'result' parameter is well-implemented

Good job adding a null check for the result parameter in the constructor to prevent potential NullPointerExceptions. This ensures that the NewResultEvent is always initialized with a valid Result.


26-31: Null checks in 'getUser()' method prevent potential issues

The null checks in the getUser() method effectively handle cases where result.getSubmission() or result.getSubmission().getParticipation() might be null. This prevents potential NullPointerExceptions and ensures robustness.

src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)

44-44: Consider logging additional event details for better debugging

Currently, the log message only indicates the processing of CompetencyJolSetEvent. Including more details about the event, such as related identifiers or relevant data, could aid in debugging and monitoring.

src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)

198-198: Verify that event publishing is correctly tested

Ensure that the replacement of pyrisEventService.trigger with pyrisEventPublisherService.publishEvent in the test maintains the intended test coverage and behavior.


269-272: Confirm that methods are not invoked when events are disabled

The test correctly verifies that no methods are called when the event is disabled. This aligns with the new event handling logic and ensures that the system behaves as expected.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 16, 2024
@kaancayli kaancayli changed the title Iris: Replace exceptions with logging for disabled event checks Iris: Improve event handling for predictable scenarios Dec 16, 2024
@kaancayli kaancayli dismissed stale reviews from coderabbitai[bot] and ole-ve via 73048c8 December 17, 2024 09:16
@kaancayli kaancayli requested a review from ole-ve December 17, 2024 09:17
Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

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

Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atlas Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

4 participants