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

Leaderboard: Custom Sliding Time Window #100

Merged
merged 16 commits into from
Sep 22, 2024

Conversation

GODrums
Copy link
Contributor

@GODrums GODrums commented Sep 20, 2024

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 and after timestamps, both usable standalone or combined
  • Server-side via @RequestParam
  • Client-side via URL query params

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 +1

In 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

  • PR title is clear and descriptive
  • PR description explains the purpose and changes
  • Code follows project coding standards
  • Self-review of the code has been done
  • Changes have been tested locally

Client (if applicable)

  • UI changes look good on all screen sizes and browsers
  • No console errors or warnings
  • User experience and accessibility have been tested
  • Added Storybook stories for new components
  • Components follow design system guidelines (if applicable)

Server (if applicable)

  • Code is performant and follows best practices
  • No security vulnerabilities introduced
  • Proper error handling has been implemented
  • Added tests for new functionality
  • Changes have been tested in different environments (if applicable)

@GODrums GODrums self-assigned this Sep 20, 2024
@github-actions github-actions bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 20, 2024
@github-actions github-actions bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed autocommit-openapi size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 20, 2024
webapp/src/app/home/home.component.ts Outdated Show resolved Hide resolved
webapp/src/app/home/home.component.ts Outdated Show resolved Hide resolved
webapp/src/app/home/home.component.ts Outdated Show resolved Hide resolved
webapp/src/app/home/home.component.ts Outdated Show resolved Hide resolved
@@ -55,8 +59,8 @@ public List<LeaderboardEntry> createLeaderboard() {
Set<PullRequestReviewDTO> commentSet = new HashSet<>();

user.getReviews().stream()
Copy link
Collaborator

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.

Copy link
Contributor Author

@GODrums GODrums Sep 20, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

webapp/src/app/home/home.component.ts Outdated Show resolved Hide resolved
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()
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

@GODrums GODrums Sep 21, 2024

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.

@github-actions github-actions bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 20, 2024
@github-actions github-actions bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed autocommit-openapi size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 20, 2024
@github-actions github-actions bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 21, 2024
Copy link
Collaborator

@FelixTJDietrich FelixTJDietrich left a 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! 😄

@FelixTJDietrich FelixTJDietrich merged commit 3d26821 into develop Sep 22, 2024
5 checks passed
@FelixTJDietrich FelixTJDietrich deleted the leaderboard-after-before branch September 22, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-server client enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants