Programming exercises: Use server time for exercise details dates
#9755
Conversation
github-actions
bot
added
the
client
rabeatwork
changed the title
Programming Exercises: Use servertime for exercise details datesProgramming exercises: Use servertime for exercise details dates
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 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: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/utils/date.utils.ts (1)
78-80: Add JSDoc documentation for the new parameter and clarify the function name.The function implementation is correct, but there are a few improvements that could make it more maintainable:
- The function name
isDateLessThanAWeekInTheFutureis slightly misleading as it checks if the date is between now and a week from now, not just if it's less than a week in the future.- The new
compareTimeparameter needs documentation.Consider applying these changes:
+/** + * Checks if a date falls between now (or a specified comparison time) and one week from that time + * @param date The date to check + * @param compareTime Optional reference time for comparison (defaults to current time) + * @returns true if the date is between the comparison time and one week from it + */ -export function isDateLessThanAWeekInTheFuture(date: dayjs.Dayjs, compareTime?: dayjs.Dayjs): boolean { +export function isDateWithinNextWeek(date: dayjs.Dayjs, compareTime?: dayjs.Dayjs): boolean { const now = compareTime ?? dayjs(); - return date.isBetween(now.add(1, 'week'), now); + return date.isBetween(now, now.add(1, 'week')); }The changes:
- Add comprehensive JSDoc documentation
- Rename function to better reflect its behavior
- Swap the order of arguments in
isBetweento be more intuitive (from earlier to later date)src/main/webapp/app/exercises/shared/exercise-headers/exercise-headers-information/exercise-headers-information.component.ts (3)
45-45: Consider implementing periodic server time updatesWhile the server time initialization is correct, the
nowproperty might become stale during long user sessions. Consider implementing a periodic refresh mechanism to keep the server time synchronized.Example implementation:
+ private destroy$ = new Subject<void>(); ngOnInit() { this.dueDate = getExerciseDueDate(this.exercise, this.studentParticipation); - this.now = this.serverDateService.now(); + // Initial server time + this.updateServerTime(); + // Refresh server time every minute + interval(60000).pipe( + takeUntil(this.destroy$) + ).subscribe(() => this.updateServerTime()); // ... rest of the code } + private updateServerTime(): void { + this.now = this.serverDateService.now(); + } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Also applies to: 57-57
Line range hint
120-145: Consider breaking down complex date-related logicThe method handles multiple date-related conditions and UI elements. Consider extracting these into separate methods for better maintainability.
Example refactoring:
- addDueDateItems() { + addDueDateItems() { + this.addBasicDueDateItem(); + this.addAssessmentDueDateItem(); + this.addComplaintDueDateItem(); + } + + private addBasicDueDateItem(): void { + const dueDateItem = this.getDueDateItem(); + if (dueDateItem) { + this.informationBoxItems.push(dueDateItem); + } + } + + private addAssessmentDueDateItem(): void { if (this.dueDate?.isBefore(this.now) && this.exercise.assessmentDueDate?.isAfter(this.now)) { const assessmentDueItem: InformationBox = { title: 'artemisApp.courseOverview.exerciseDetails.assessmentDue', content: { type: 'dateTime', value: this.exercise.assessmentDueDate, }, isContentComponent: true, tooltip: 'artemisApp.courseOverview.exerciseDetails.assessmentDueTooltip', tooltipParams: { date: this.exercise.assessmentDueDate.format('lll') }, }; this.informationBoxItems.push(assessmentDueItem); } + } + + private addComplaintDueDateItem(): void { if (this.exercise.assessmentDueDate?.isBefore(this.now) && this.individualComplaintDueDate?.isAfter(this.now)) { const complaintDueItem: InformationBox = { title: 'artemisApp.courseOverview.exerciseDetails.complaintDue', content: { type: 'dateTime', value: this.individualComplaintDueDate, }, isContentComponent: true, tooltip: 'artemisApp.courseOverview.exerciseDetails.complaintDueTooltip', tooltipParams: { date: this.individualComplaintDueDate.format('lll') }, }; this.informationBoxItems.push(complaintDueItem); } }
Other components using direct
Dateusage found.
src/main/resources/templates/typescript/test/src/behavior.test.tssrc/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.service.tssrc/main/webapp/app/shared/server-date.service.tssrc/test/javascript/spec/service/course-exercise.service.spec.tssrc/main/webapp/app/admin/audits/audits.component.ts- ... (additional file paths as identified)
🔗 Analysis chain
Line range hint
1-299: Verify time manipulation scenariosWhile the implementation looks correct, please ensure comprehensive testing of the following scenarios:
- Time zone changes
- System time manipulation
- Browser time manipulation
- Long running sessions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other time-sensitive components that might need similar updates # Search for dayjs usage in components echo "Searching for other components using dayjs..." rg -l "dayjs\(\)" --type ts # Search for potential time manipulation vulnerabilities echo "Searching for direct Date usage..." rg "new Date\(\)" --type ts # Search for time-related methods echo "Searching for time-related methods..." rg "\.(isBefore|isAfter|isBetween)\(" --type tsLength of output: 67759
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/exercises/shared/exercise-headers/exercise-headers-information/exercise-headers-information.component.ts(7 hunks)src/main/webapp/app/utils/date.utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/exercises/shared/exercise-headers/exercise-headers-information/exercise-headers-information.component.ts (1)
src/main/webapp/app/utils/date.utils.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/utils/date.utils.ts (1)
78-78: Verify the impact of function rename.
Let's check where this function is used to understand the scope of the rename change.
✅ Verification successful
Function rename impact verified.
All usages are accounted for in test specs and ExerciseHeadersInformationComponent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of the function to assess the impact of renaming
rg "isDateLessThanAWeekInTheFuture" --type ts
Length of output: 1403
src/main/webapp/app/exercises/shared/exercise-headers/exercise-headers-information/exercise-headers-information.component.ts (2)
20-20: LGTM: Clean dependency injection implementation
The ArtemisServerDateService integration follows Angular best practices and style guidelines.
Also applies to: 50-53
151-155: LGTM: Consistent server time usage
The date comparison logic has been consistently updated to use server time across all methods, which effectively addresses the PR's objective of preventing time manipulation issues.
Also applies to: 183-185, 226-226
rabeatwork
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test3
and removed
deployment-error
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
|
flbrgit
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
JerroyTan
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
[Tested on TS5]
For me the time only automatically switched according to the changes on my computer system when I changed the time zone. However, when I changed the time manually (moved 5 minutes before, moved 2 hours before) it did not work as expected. The time on the exercise was shown according to the last time zone I set myself to.
Tested on Windows 11
That's the intended behavior. Since we do not want the students to manipulate the time on their computer and, by doing so, trick the system (e.g., by setting their computer time to before the due date of an exercise). |
rabeatwork
added
ready to merge
maintainer-approved
krusche
changed the title
Programming exercises: Use servertime for exercise details datesProgramming exercises: Use server time for exercise details dates
krusche
deleted the
bugfix/programming-exercises/use-servertime-for-exercise-details-dates
branch
Checklist
General
Motivation and Context
If a student manually changes the time on their computer (without changing the time zone), the time until an exercise starts, is due, etc., was displayed incorrectly.
Description
To avoid this, the server time is fetched and used instead of using dayjs().
Steps for Testing
Prerequisites:
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
Manual Tests
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Refactor