Lectures: Integrate slide visibility scheduling into ScheduleService
#10705
Conversation
github-actions
bot
added
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
…fecycle' into feature/lectures/create-slide-lifecycle
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
WalkthroughThis change introduces a comprehensive scheduling framework for managing slide lifecycle events, particularly for handling the automatic unhiding of slides. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as AdminScheduleResource
participant Schedule as ScheduleService
participant Slide as Slide
participant SlideLife as SlideLifecycleService
participant UnhideSvc as SlideUnhideService
participant ExecSvc as SlideUnhideExecutionService
Admin->>+Schedule: getAllSlideSchedules(pageable)
Schedule-->>-Admin: Page<ScheduledSlideEvent>
UnhideSvc->>+Schedule: cancelScheduledTaskForSlideLifecycle(slideId, UNHIDE)
alt hidden date is null
UnhideSvc-->>UnhideSvc: Log and return
else hidden date in past
UnhideSvc->>+ExecSvc: unhideSlide(slideId)
ExecSvc-->>-UnhideSvc: (slide unhidden)
else hidden date in future
UnhideSvc->>+Schedule: scheduleSlideTask(slide, UNHIDE, task, "Slide Unhiding")
Schedule->>+SlideLife: scheduleTask(slide, UNHIDE, task)
SlideLife-->>-Schedule: ScheduledFuture
Schedule-->>-UnhideSvc: (task scheduled)
end
UnhideSvc->>+Schedule: cancelScheduledTaskForSlideLifecycle(slideId, UNHIDE)
UnhideSvc->>+ExecSvc: unhideSlide(slideId)
ExecSvc-->>-UnhideSvc: (slide unhidden)
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/domain/SlideLifecycle.java (1)
10-18: Consider clarifying the Javadoc comment for UNHIDE.The Javadoc comment states "The date when the slide becomes visible" but the implementation returns
slide.getHidden(), which is when the slide was hidden. This might be confusing for future developers. Consider updating the comment to clarify that this returns the date when the slide should be unhidden (which is stored in thehiddenfield).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/SlideLifecycle.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideLifecycleService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/lecture/domain/SlideLifecycle.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/SlideLifecycleService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.javasrc/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java
🧬 Code Graph Analysis (2)
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleServiceTest.java (1)
src/main/webapp/app/lecture/shared/entities/lecture-unit/slide.model.ts (1)
Slide(4-10)
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java (1)
src/main/webapp/app/lecture/shared/entities/lecture-unit/slide.model.ts (1)
Slide(4-10)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (41)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java (2)
45-45: LGTM! Renamed method improves clarity.The method renaming from
getAlltogetAllExerciseSchedulesenhances code readability by being more specific about what type of schedules are being retrieved.
51-62: LGTM! Well-implemented new endpoint.The new slide schedules endpoint is nicely implemented, following the same pattern as the existing exercise schedules endpoint with proper pagination, HTTP response generation, and clear documentation.
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideLifecycleService.java (3)
18-31: LGTM! Well-designed service with proper dependency injection.The service is correctly configured with appropriate annotations and follows best practices with constructor injection for the TaskScheduler dependency.
44-48: LGTM! Well-implemented scheduling with appropriate logging.The scheduleTask method correctly uses the TaskScheduler with proper error handling and appropriate debug logging. The implementation is clean and focused.
60-62: LGTM! Good method overloading pattern.This overloaded method provides a convenient way to derive the execution time directly from the slide and lifecycle event, improving API usability.
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java (1)
26-161: LGTM! Comprehensive test coverage with well-structured test cases.This test class provides excellent coverage of the SlideUnhideExecutionService functionality, testing both normal operation and various edge cases. The structure follows good testing practices with clear setup, execution, and verification phases in each test.
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleServiceTest.java (10)
1-25: Good structure with proper imports and test setup.The imports are well-organized, and the class appropriately extends AbstractSpringIntegrationIndependentTest. The test uses descriptive naming conventions and follows JUnit 5 best practices.
26-45: Well-structured test class with appropriate test repository usage.The class follows good testing practices by:
- Using a descriptive unique test prefix for user context
- Properly mocking external dependencies
- Using Spring's DI for test utilities and repositories
- Creating a clear instance of the service under test with controllable dependencies
This approach isolates the service for proper unit testing while using real repositories for slide data.
46-65: Good test data initialization with varied test cases.The setup method effectively initializes test data by:
- Setting up mock objects
- Instantiating the service under test with the right dependencies
- Creating test slides with different hidden timestamps
- Using both past and future dates to test different scenarios
This provides comprehensive coverage for scheduling logic based on different timestamp conditions.
67-80: Test effectively verifies scheduling of all hidden slides.The test properly checks that
scheduleAllHiddenSlides()triggers the slide unhide service for each hidden slide. This ensures the core functionality of finding and scheduling all hidden slides works correctly.
82-98: Test properly verifies scheduling by DTO with valid slide data.This test correctly validates that when scheduling with a valid slide DTO:
- The service successfully finds the slide by ID
- The slide unhide service is properly invoked with the slide object
This ensures correct DTO-to-entity handling in the scheduling workflow.
100-111: Test appropriately handles null hidden date case.The test correctly verifies that when a DTO has a null hidden date:
- The service properly handles this edge case
- No unhide operations are triggered
This ensures the service correctly handles optional or missing date values.
113-125: Test validates scheduling by valid slide ID.The test confirms that when scheduling with a valid slide ID:
- The slide is correctly retrieved from the repository
- The unhide service is called with the expected slide object
This ensures proper ID-based scheduling flow.
127-138: Test verifies system stability with invalid ID input.The test properly validates that when an invalid slide ID is provided:
- The service gracefully handles the non-existent ID
- No unhide operations are attempted
This prevents potential runtime errors from invalid inputs.
140-150: Test confirms correct cancellation of scheduled unhide tasks.The test verifies that the cancellation flow:
- Properly delegates to the schedule service
- Passes the correct slide ID to the cancellation method
This ensures scheduled tasks can be properly cancelled.
152-163: Test confirms proper initialization on application startup.Using a spy to verify method invocation is a good approach to test that the
onApplicationReadyevent properly triggers scheduling of all hidden slides, ensuring automatic initialization of slide unhiding.src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideService.java (7)
5-7: Appropriate imports for date handling and optional values.The imports of ZonedDateTime and Optional are necessary for the updated implementation and follow the coding guidelines for Java imports.
13-16: Good imports for scheduling service and lifecycle domain model.The ScheduleService import and SlideLifecycle enum import are appropriate for implementing the new scheduling infrastructure.
18-19: Improved service documentation.The updated JavaDoc clearly describes the service's responsibility for slide unhiding business logic.
24-33: Good service setup with proper dependency injection.The service now:
- Uses a proper logger for traceability
- Accepts an Optional to handle potential absence of the service
- Uses constructor injection for dependencies following best practices
This ensures clean dependency management and prevents NPEs when the schedule service isn't available.
35-42: Improved method documentation with comprehensive description.The updated JavaDoc clearly describes all three scenarios for slide hidden property updates:
- Future date: Schedule unhiding
- Past date: Unhide immediately
- Null: Cancel existing tasks
This makes the method's behavior clear and helps with maintenance.
43-64: Well-implemented slide unhiding logic with proper error handling.The handleSlideHiddenUpdate method:
- First cancels any existing scheduled tasks to prevent duplication
- Handles null hidden dates with early return
- Compares current time with hidden date to decide between immediate unhiding and scheduling
- Uses appropriate logging for each decision path
- Safely accesses the optional schedule service
The implementation is robust and handles all edge cases appropriately.
67-76: Enhanced unhide method with task cancellation.The method now cancels any scheduled unhide tasks before performing the immediate unhide operation, which prevents potential race conditions where a scheduled unhide might occur after a manual unhide.
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java (8)
4-11: Updated imports for proper mock testing.The additional imports for ArgumentCaptor, Mock, MockitoAnnotations, and SlideLifecycle support the updated testing approach that verifies service interactions rather than implementation details.
20-24: Appropriate service dependencies for testing.The imports now include ScheduleService and SlideLifecycle, which are necessary for testing the updated implementation.
32-45: Proper test setup with mocked dependencies.The test now:
- Mocks both ScheduleService and SlideUnhideExecutionService
- Maintains real repositories for slide data
- Sets up the service with explicit dependencies
This approach provides good isolation for testing while maintaining real data behavior.
48-55: Improved test initialization with proper mocking.The updated initialization:
- Properly initializes all mocks with MockitoAnnotations
- Creates the service with explicit mocked dependencies
- Uses Optional.of for the schedule service to match the implementation
This setup enables precise verification of service interactions.
71-97: Well-structured test for future hidden date scenario.The test effectively verifies that when a slide has a future hidden date:
- The scheduleSlideTask method is called with the correct arguments
- The slide, lifecycle event, and task name are correctly passed
- The service doesn't immediately unhide the slide
Using ArgumentCaptors is an effective approach to verify multiple arguments.
99-116: Good test coverage for past hidden date scenario.The test properly verifies that when a slide has a past hidden date:
- The slide is immediately unhidden via the execution service
- No scheduling is performed
This ensures the service correctly handles slides that should already be unhidden.
132-138: Updated verification for null hidden date scenario.The test now properly verifies that when a slide's hidden date is null:
- Any existing scheduled tasks are cancelled
- No new tasks are scheduled
- No immediate unhiding occurs
This ensures proper handling of slides that don't need unhiding.
151-155: Updated verification for manual unhide scenario.The test now correctly verifies that when manually unhiding a slide:
- Any existing scheduled tasks are cancelled first
- The slide is immediately unhidden via the execution service
This ensures proper coordination between scheduled and manual unhiding.
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideScheduleService.java (6)
14-17: Updated import for schedule service integration.The import of ScheduleService is appropriate for integrating with the centralized scheduling system.
18-21: Improved class documentation.The updated JavaDoc clearly explains the service's role in delegating to the integrated ScheduleService, which helps with understanding the design.
27-38: Simplified service with proper dependency injection.The service now:
- Has appropriate dependencies for its responsibilities
- Uses constructor injection following best practices
- Delegates scheduling to SlideUnhideService and cancellation to ScheduleService
This refactoring promotes single responsibility principle and improves maintainability.
72-73: Simplified scheduling logic with proper delegation.The method now properly delegates to SlideUnhideService for handling slide unhiding, removing duplication of scheduling logic and improving maintainability.
81-82: Consistent delegation pattern for slide unhiding.This method follows the same pattern of delegating to SlideUnhideService, maintaining consistency in the codebase and centralizing the unhide logic.
90-91: Proper delegation of cancellation to ScheduleService.The method delegates cancellation to ScheduleService and adds appropriate logging, centralizing the scheduling logic in the dedicated service.
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (4)
110-122: Inject a sharedTaskSchedulerbean instead of instantiating a new poolCreating a dedicated
ThreadPoolTaskSchedulerhere duplicates thread‑pool infrastructure that Spring already provides.
Drawbacks:
- Multiple schedulers = harder tracing & metrics, especially now that this service is the central hub for all lifecycle events (exercise + participation + slide).
- The pool size is hard‑coded to
1; with virtual threads enabled this partly defeats the purpose because a single carrier thread can still become a bottleneck for long‑running jobs.- No explicit shutdown (
scheduler.shutdown()) – the pool survives application context re‑starts in tests, leaking threads and producing flaky failures.Consider constructor‑injecting the primary
TaskScheduler(or a qualified bean) and removing the manual initialization:- private final TaskScheduler taskScheduler; + private final TaskScheduler taskScheduler; @@ - public ScheduleService(ExerciseLifecycleService exerciseLifecycleService, ParticipationLifecycleService participationLifecycleService, - SlideLifecycleService slideLifecycleService) { + public ScheduleService(ExerciseLifecycleService exerciseLifecycleService, + ParticipationLifecycleService participationLifecycleService, + SlideLifecycleService slideLifecycleService, + TaskScheduler taskScheduler) { // <-- inject shared bean @@ - // Initialize the TaskScheduler - ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); - scheduler.setPoolSize(1); - scheduler.setVirtualThreads(true); - scheduler.initialize(); - this.taskScheduler = scheduler; + this.taskScheduler = taskScheduler; }If a dedicated pool is still required, declare it as a separate
@Beanin a configuration class so Spring can manage its lifecycle.
[ suggest_essential_refactor ]
118-122: Ensure proper shutdown of the scheduler to prevent thread leaksWhen keeping the ad‑hoc
ThreadPoolTaskScheduler, add a@PreDestroy‑annotated method to calltaskScheduler.shutdown().
Without this, integration‑/unit‑tests that tear down and recreate the Spring context leave non‑daemon carrier threads running, producing “task already scheduled on shutdown executor” warnings.
[ flag_critical_issue ]
153-166: Guard against negative delays when computingscheduledTime
future.getDelay(TimeUnit.SECONDS)returns a negative value once the task is overdue, causingscheduledTimeto be in the past and breaking chronologic ordering on the admin UI.- var scheduledTime = ZonedDateTime.now().plusSeconds(task.future().getDelay(TimeUnit.SECONDS)); + long delaySeconds = Math.max(0, task.future().getDelay(TimeUnit.SECONDS)); + var scheduledTime = ZonedDateTime.now().plusSeconds(delaySeconds);(Optional) filter out already‑executed tasks altogether instead of showing a timestamp “now minus X”.
[ suggest_optional_refactor ]
341-346: Support multiple slide tasks per lifecycle (mirroring exercises)
scheduleSlideTaskcurrently accepts only a singleRunnable. For feature parity with exercises you may need overloads accepting:
Pair<ZonedDateTime, Runnable>– schedule at a specific point in time.Set<Pair<ZonedDateTime, Runnable>>– schedule several runs.This avoids downstream services having to loop and call
scheduleSlideTaskrepeatedly.
[ suggest_optional_refactor ]
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideExecutionServiceTest.java
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
eceeeren
force-pushed
the
feature/lectures/create-slide-lifecycle
branch
from
de9b452 to
3265e54
Compare
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
|
I am manually testing this. What do you mean by point no. 5 exactly? I have created an attachment unit as a PDF, then I hide it for 2 minutes. |
Hello, sorry for the confusion. When you hide it for 2 minutes, after 2 minutes the page should be available again. The idea here is to test if SlideUnhideService automatically makes the slides visible again. If you can't see the page again after 2 minutes (also as a student), something should be wrong. |
I can see it ,after the specified time. But I have to download it again to check it. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
|
Locally tested- Attached a PDF, hide it for a few minutes. On the student account, when I download the PDF file, the section is hidden for the time. |
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
Hello Khiem, thanks for your testing. Did you save the attachment unit after hiding the page? If not, the Slide Unhide Service is not triggered. |
There was a problem hiding this comment.
Locally tested- Attached a PDF, hide it for a few minutes. On the student account, when I download the PDF file, the section is hidden for the time.
Also when aligned with the due date of an exercise, it is released parallely. As mentioned, on student side pdf needs to be downloaded
👍
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Checklist
General
Server
Motivation and Context
This pull request assigns a distinct name to each scheduled task to improve clarity in debug logs, making it easier to identify and trace specific tasks during execution. It also exposes the scheduling information through an admin REST API via AdminScheduleResource, allowing for better visibility and management of scheduled tasks.
Description
To enable this implementation, the following changes are made:
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit