-
Notifications
You must be signed in to change notification settings - Fork 1
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
Leaderboard: Custom Sliding Time Window #100
Conversation
...plication-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
@@ -55,8 +59,8 @@ public List<LeaderboardEntry> createLeaderboard() { | |||
Set<PullRequestReviewDTO> commentSet = new HashSet<>(); | |||
|
|||
user.getReviews().stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user.getReviews
could already support querying by timeframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand your idea correctly, you would like to do the filtering in the SQL-Query already?
That's a great idea for performance improvements, I will take a look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried out a Query like this:
SELECT u
FROM User u
JOIN FETCH u.pullRequests
JOIN FETCH u.issueComments
JOIN FETCH u.reviewComments
JOIN FETCH u.reviews re
WHERE re.createdAt between :after and :before
OR re.updatedAt between :after and :before
but it doesn't seem to return any results, most likely I'm missing something regarding the format conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to find the correct SQL query to filter the dates.
With 63c434d, we now filter directly in the DB.
OffsetDateTime cutOffTime = new Date(System.currentTimeMillis() - 1000 * 60 * 60 * 24 * timeframe) | ||
.toInstant().atOffset(ZoneOffset.UTC); | ||
OffsetDateTime afterCutOff = after.isPresent() ? after.get() | ||
: new Date(System.currentTimeMillis() - 1000 * 60 * 60 * 24 * timeframe).toInstant() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should round to the beginning of the day?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or what happens with 2024-09-10
? I guess it will use midnight, then it might be important for the after
case. If you for example use 2024-09-15
as before
I think it should get all timestamps before the end of day. So actually we would need to round up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no timestamp is provided, it should default to midnight, yes.
While I personally find different default timestamps for before
and after
confusing, I do see why this would be expected behavior for users. Will change it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look through Java best practices and found this sheet: https://i.sstatic.net/WyAl2.png
Equivalently to your idea, after and before are now only dates (LocalDate
- without timestamp) with an interpretation as the start and end of the day on the server respectively. Also looks much cleaner in the URL (example: http://localhost:4200/?after=2024-09-21&before=2024-09-23).
Implemented this change in b76ed6d.
…m/Hephaestus into leaderboard-after-before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, thank you! 😄
Motivation
We want to be able to set custom timeframes for the leaderboard. This allows us to create sliding time windows with custom dates for the client.
Description
This PR adds support for custom sliding time windows in both the server- and client-side:
before
andafter
timestamps, both usable standalone or combined@RequestParam
Example URL: http://localhost:4200/?after=2024-09-18T10:15:30%2001:00&before=2024-09-20T10:15:30%2001:00
-> sets
after
to: 2024-09-19 10:15:30 UTC +1-> sets
before
to: 2024-09-20 10:15:30 UTC +1In the future, this should be combined with UI elements (e.g. button, range-slider) to allow the visitor of the website to filter the timeframe they would like to see.
Checklist
General
Client (if applicable)
Server (if applicable)