Programming exercises: Calculate submission time difference with server time
#10123
Conversation
github-actions
bot
added
the
client
badkeyy
changed the title
Programming Exercises: Calculate submission time with server time
badkeyy
changed the title
Programming Exercises: Calculate submission time with server timeProgramming exercises: Calculate submission time with server time
|
github-actions
bot
added
the
deployment-error
badkeyy
added
deploy:artemis-test2
and removed
deployment-error
badkeyy
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
badkeyy
changed the title
Programming exercises: Calculate submission time with server timeProgramming exercises: Calculate submission time difference with server time
WalkthroughThe pull request introduces a modification to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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 comments (3)
src/test/javascript/spec/pipe/artemis-time-ago.pipe.spec.ts (1)
Line range hint
35-43: Make tests more deterministic by using fixed timestampsCurrent tests use dynamic time calculations with
dayjs(), which could lead to flaky tests. Consider using fixed timestamps and the mock service's capabilities for more reliable testing.- it.each([ - { value: dayjs().add(5, 'minutes'), expected: { en: 'in 5 minutes', de: 'in 5 Minuten' } }, - { value: dayjs().add(10, 'minutes'), expected: { en: 'in 10 minutes', de: 'in 10 Minuten' } }, + it.each([ + { minutes: 5, future: true, expected: { en: 'in 5 minutes', de: 'in 5 Minuten' } }, + { minutes: 10, future: true, expected: { en: 'in 10 minutes', de: 'in 10 Minuten' } }, + ])('returns the correct time and switches the language correctly', ({ minutes, future, expected }) => { + const baseTime = dayjs('2024-01-01T12:00:00Z'); + const testTime = future ? baseTime.add(minutes, 'minutes') : baseTime.subtract(minutes, 'minutes'); + (serverTimeService as MockArtemisServerDateService).setMockDate(baseTime);src/main/webapp/app/shared/pipes/artemis-time-ago.pipe.ts (2)
Line range hint
51-70: Enhance timer cleanup to prevent memory leaksThe timer cleanup in
createTimercould be more robust. Consider adding error handling and ensuring cleanup on all paths.private createTimer() { if (this.currentTimer) { + this.removeTimer(); // Ensure cleanup of existing timer return; } const dayjsInstance = dayjs(this.lastValue); const timeToUpdate = getSecondsUntilUpdate(dayjsInstance) * 1000; this.currentTimer = this.ngZone.runOutsideAngular(() => { if (typeof window !== 'undefined') { - return window.setTimeout(() => { + const timerId = window.setTimeout(() => { + if (this.currentTimer !== timerId) { + return; // Another timer was created + } this.lastText = this.formatFn(dayjs(this.lastValue)); this.currentTimer = null; this.ngZone.run(() => this.cdRef.markForCheck()); }, timeToUpdate); + return timerId; } else { return null; } }); }
Use ArtemisServerDateService for timer intervals
The suggestion to use server time is correct, but instead of passing server time as a parameter, use the already injected
ArtemisServerDateService:function getSecondsUntilUpdate(dayjsInstance: dayjs.Dayjs) { - const howOld = Math.abs(dayjs().diff(dayjsInstance, 'minute')); + const howOld = Math.abs(this.serverDateService.now().diff(dayjsInstance, 'minute'));🔗 Analysis chain
Line range hint
89-93: Verify timer intervals with server timeThe
getSecondsUntilUpdatefunction uses client-side time (dayjs()) to calculate update intervals. This should be updated to use server time for consistency.function getSecondsUntilUpdate(dayjsInstance: dayjs.Dayjs) { - const howOld = Math.abs(dayjs().diff(dayjsInstance, 'minute')); + // TODO: Pass serverTime as parameter + const howOld = Math.abs(serverTime.diff(dayjsInstance, 'minute'));Let's verify the usage of this function:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to getSecondsUntilUpdate to ensure we can safely modify its signature rg "getSecondsUntilUpdate" --type tsLength of output: 287
Script:
#!/bin/bash # Search for server time related patterns rg -i "serverTime|server[._]time" --type ts -A 2 # Search for time sync or server clock patterns rg -i "time[._]sync|clock[._]sync" --type ts -A 2Length of output: 2139
🧹 Nitpick comments (3)
src/test/javascript/spec/helpers/mocks/service/mock-artemis-server-date.service.ts (1)
3-7: Enhance mock service with configurable time controlThe current implementation always returns the actual current time, which might not be suitable for all test scenarios. Consider adding the ability to set a specific time for more deterministic tests.
export class MockArtemisServerDateService { + private mockDate?: dayjs.Dayjs; + + setMockDate(date: dayjs.Dayjs): void { + this.mockDate = date; + } + now(): dayjs.Dayjs { - return dayjs(); + return this.mockDate || dayjs(); } }src/test/javascript/spec/pipe/artemis-time-ago.pipe.spec.ts (1)
Line range hint
47-58: Add test coverage for edge casesThe test suite should include additional scenarios to ensure robust handling of various time differences.
Consider adding tests for:
- Different time zones
- Date transitions (day/month/year boundaries)
- Invalid date inputs
- Extreme time differences
src/main/webapp/app/shared/pipes/artemis-time-ago.pipe.ts (1)
28-30: Consider caching server time for performanceThe
formatmethod callsserverDateService.now()on every invocation, which could lead to unnecessary server calls. Consider caching the server time and updating it periodically.+ private lastServerTime: dayjs.Dayjs; + private readonly SERVER_TIME_UPDATE_INTERVAL = 60000; // 1 minute + + private updateServerTime(): void { + this.lastServerTime = this.serverDateService.now(); + } + format(date: dayjs.Dayjs) { - return date.locale(this.lastLocale).from(this.serverDateService.now(), this.lastOmitSuffix); + return date.locale(this.lastLocale).from(this.lastServerTime, this.lastOmitSuffix); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/webapp/app/shared/pipes/artemis-time-ago.pipe.ts(2 hunks)src/test/javascript/spec/helpers/mocks/service/mock-artemis-server-date.service.ts(1 hunks)src/test/javascript/spec/pipe/artemis-time-ago.pipe.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/test/javascript/spec/helpers/mocks/service/mock-artemis-server-date.service.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/pipe/artemis-time-ago.pipe.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/shared/pipes/artemis-time-ago.pipe.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
vinceclifford
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Currently the submission time difference ("Submitted X minutes ago") has been calculated with client time. With this PR it gets now calculated with server time.
Fixes #9940
Description
I changed the ArtemisTimeAgoPipe to use server time (ArtemisServerDateService) and not client time. I also adapted the tests for it.
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Tests
Refactor