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 component and homepage #78

Merged
merged 33 commits into from
Sep 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c5b2c2c
Add Reviews to PRs
GODrums Sep 4, 2024
b2d5b26
Rename owner to author
GODrums Sep 4, 2024
1806b17
Concretize variable names
GODrums Sep 4, 2024
6f7424a
Prettify into functional approach
GODrums Sep 4, 2024
199278e
fix: owner rename in methods
GODrums Sep 4, 2024
72850f8
chore: update API specs and client
github-actions[bot] Sep 4, 2024
6fbea2b
Add Reviews to PRs
GODrums Sep 4, 2024
fe4c6ea
Rename owner to author
GODrums Sep 4, 2024
b9ab354
Concretize variable names
GODrums Sep 4, 2024
8f09e20
Prettify into functional approach
GODrums Sep 4, 2024
b681b82
fix: owner rename in methods
GODrums Sep 4, 2024
80d092a
chore: update API specs and client
github-actions[bot] Sep 4, 2024
7ff900b
Merge branch 'feature/schema-update-reviews' of https://github.com/ls…
GODrums Sep 5, 2024
fb5d518
Add ReviewComments and refactor IssueComments
GODrums Sep 6, 2024
f680230
Table Example Component
GODrums Sep 8, 2024
f44820d
Add Leaderboard page
GODrums Sep 8, 2024
81a8973
Change allowed node version range
GODrums Sep 10, 2024
eee3cd7
Rework leaderboard standalone
GODrums Sep 13, 2024
3937532
Merge branch 'develop' into feature/leaderboard-page
GODrums Sep 13, 2024
97ce98e
fix: merge corrections
GODrums Sep 13, 2024
d207561
Extract icons into files
GODrums Sep 13, 2024
1916d90
Integrate leaderboard service
GODrums Sep 13, 2024
6369b5d
fix: delete leftovers from merge
GODrums Sep 13, 2024
2775552
deps: remove @Tanstack/table
GODrums Sep 13, 2024
70e1c65
Add user type property
GODrums Sep 14, 2024
407dd8e
Score calculation for leaderboard
GODrums Sep 14, 2024
06a5206
Add Github link to leaderboard entry
GODrums Sep 14, 2024
e82dc6a
chore: update API specs and client
github-actions[bot] Sep 14, 2024
aa6a2e7
Only show activity when present
GODrums Sep 14, 2024
7472b20
Add rank to leaderboard
GODrums Sep 14, 2024
555e7a7
Remove table footer
GODrums Sep 14, 2024
5745b82
chore: update API specs and client
github-actions[bot] Sep 15, 2024
8045967
Merge branch 'develop' into feature/leaderboard-page
FelixTJDietrich Sep 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion server/application-server/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,18 @@ components:
enum:
- CLOSED
- OPEN
additions:
type: integer
format: int32
deletions:
type: integer
format: int32
commits:
type: integer
format: int32
changedFiles:
type: integer
format: int32
mergedAt:
type: string
format: date-time
Expand Down Expand Up @@ -402,6 +414,7 @@ components:
User:
required:
- login
- type
- url
type: object
properties:
Expand Down Expand Up @@ -433,6 +446,12 @@ components:
URL to the user's avatar.
If unavailable, a fallback can be generated from the login, e.g. on Github:
https://github.com/{login}.png
type:
type: string
description: Type of the user. Used to distinguish between users and bots.
enum:
- USER
- BOT
pullRequests:
uniqueItems: true
type: array
Expand All @@ -458,12 +477,19 @@ components:
properties:
githubName:
type: string
avatarUrl:
type: string
name:
type: string
type:
type: string
enum:
- USER
- BOT
score:
type: integer
format: int32
total:
rank:
type: integer
format: int32
changesRequested:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ public class PullRequest extends BaseGitServiceEntity {
@NonNull
private IssueState state;

private int additions;

private int deletions;

private int commits;

private int changedFiles;

private OffsetDateTime mergedAt;

@ManyToOne(fetch = FetchType.EAGER)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package de.tum.in.www1.hephaestus.codereview.pullrequest;

import java.io.IOException;

import org.kohsuke.github.GHPullRequest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.lang.NonNull;
import org.springframework.stereotype.Component;

Expand All @@ -9,6 +13,8 @@
@Component
public class PullRequestConverter extends BaseGitServiceEntityConverter<GHPullRequest, PullRequest> {

protected static final Logger logger = LoggerFactory.getLogger(PullRequestConverter.class);

@Override
public PullRequest convert(@NonNull GHPullRequest source) {
IssueState state = convertState(source.getState());
Expand All @@ -17,6 +23,30 @@ public PullRequest convert(@NonNull GHPullRequest source) {
pullRequest.setTitle(source.getTitle());
pullRequest.setUrl(source.getHtmlUrl().toString());
pullRequest.setState(state);
try {
pullRequest.setAdditions(source.getAdditions());
} catch (IOException e) {
logger.error("Failed to convert additions field for source {}: {}", source.getId(), e.getMessage());
pullRequest.setAdditions(0);
}
try {
pullRequest.setDeletions(source.getDeletions());
} catch (IOException e) {
logger.error("Failed to convert deletions field for source {}: {}", source.getId(), e.getMessage());
pullRequest.setDeletions(0);
}
try {
pullRequest.setCommits(source.getCommits());
} catch (IOException e) {
logger.error("Failed to convert commits field for source {}: {}", source.getId(), e.getMessage());
pullRequest.setCommits(0);
}
try {
pullRequest.setChangedFiles(source.getChangedFiles());
} catch (IOException e) {
logger.error("Failed to convert changedFiles field for source {}: {}", source.getId(), e.getMessage());
pullRequest.setChangedFiles(0);
}
if (source.getMergedAt() != null) {
pullRequest.setMergedAt(convertToOffsetDateTime(source.getMergedAt()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ public class User extends BaseGitServiceEntity {
*/
private String avatarUrl;

/**
* Type of the user. Used to distinguish between users and bots.
*/
@NonNull
private UserType type;

@OneToMany(cascade = CascadeType.ALL, mappedBy = "author")
private Set<PullRequest> pullRequests = new HashSet<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,23 @@ public User convert(@NonNull org.kohsuke.github.GHUser source) {
} catch (IOException e) {
logger.error("Failed to convert user email field for source {}: {}", source.getId(), e.getMessage());
}
try {
user.setType(convertUserType(source.getType()));
} catch (IOException e) {
logger.error("Failed to convert user type field for source {}: {}", source.getId(), e.getMessage());
}
return user;
}

private UserType convertUserType(String type) {
switch (type) {
case "User":
return UserType.USER;
case "Bot":
return UserType.BOT;
default:
return UserType.USER;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package de.tum.in.www1.hephaestus.codereview.user;

public enum UserType {
FelixTJDietrich marked this conversation as resolved.
Show resolved Hide resolved
USER, BOT
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,25 @@
package de.tum.in.www1.hephaestus.leaderboard;

import com.fasterxml.jackson.annotation.JsonInclude;
import de.tum.in.www1.hephaestus.codereview.user.UserType;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.Setter;
import lombok.ToString;

@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record LeaderboardEntry(String githubName, String name, int score, int total, int changesRequested,
int approvals, int comments) {
@AllArgsConstructor
@Getter
@Setter
@ToString
public class LeaderboardEntry {
private String githubName;
private String avatarUrl;
private String name;
private UserType type;
private int score;
private int rank;
private int changesRequested;
private int approvals;
private int comments;
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
package de.tum.in.www1.hephaestus.leaderboard;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;

import de.tum.in.www1.hephaestus.codereview.pullrequest.PullRequest;
import de.tum.in.www1.hephaestus.codereview.user.User;
import de.tum.in.www1.hephaestus.codereview.user.UserService;
import de.tum.in.www1.hephaestus.codereview.user.UserType;

@Service
public class LeaderboardService {
Expand All @@ -27,10 +33,13 @@ public List<LeaderboardEntry> createLeaderboard() {
logger.info("Found " + users.size() + " users");
FelixTJDietrich marked this conversation as resolved.
Show resolved Hide resolved

List<LeaderboardEntry> leaderboard = users.stream().map(user -> {
logger.info("Creating leaderboard entry for user: " + user.getLogin());
if (user.getType() != UserType.USER) {
return null;
}
int comments = user.getIssueComments().size();
AtomicInteger changesRequested = new AtomicInteger(0);
AtomicInteger changesApproved = new AtomicInteger(0);
AtomicInteger score = new AtomicInteger(0);
user.getReviews().stream().forEach(review -> {
switch (review.getState()) {
case CHANGES_REQUESTED:
Expand All @@ -42,11 +51,46 @@ public List<LeaderboardEntry> createLeaderboard() {
default:
break;
}
score.addAndGet(calculateScore(review.getPullRequest()));
});
return new LeaderboardEntry(user.getLogin(), user.getName(), 0, 0, changesRequested.get(),
return new LeaderboardEntry(user.getLogin(), user.getAvatarUrl(), user.getName(), user.getType(),
score.get(),
0, // preliminary rank
changesRequested.get(),
changesApproved.get(), comments);
}).toList();
}).filter(Objects::nonNull).collect(Collectors.toCollection(ArrayList::new));

// update ranks by score
leaderboard.sort(Comparator.comparingInt(LeaderboardEntry::getScore).reversed());
AtomicInteger rank = new AtomicInteger(1);
leaderboard.stream().forEach(entry -> {
entry.setRank(rank.get());
rank.incrementAndGet();
});

return leaderboard;
}

/**
* Calculates the score for a given pull request.
* Possible values: 1, 3, 7, 17, 33.
* Taken from the original leaderboard implementation script.
*
* @param pullRequest
* @return score
*/
private int calculateScore(PullRequest pullRequest) {
Double complexityScore = (pullRequest.getChangedFiles() * 3) + (pullRequest.getCommits() * 0.5)
+ pullRequest.getAdditions() + pullRequest.getDeletions();
if (complexityScore < 10) {
return 1; // Simple
} else if (complexityScore < 50) {
return 3; // Medium
} else if (complexityScore < 100) {
return 7; // Large
} else if (complexityScore < 500) {
return 17; // Huge
}
return 33; // Overly complex
}
}
6 changes: 6 additions & 0 deletions webapp/.vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"tailwindCSS.experimental.classRegex": ["cva\\(([^)]*)\\)", "[\"'`]([^\"'`]*).*?[\"'`]", "cn\\(([^)]*)\\)", "[\"'`]([^\"'`]*).*?[\"'`]"],
"editor.codeActionsOnSave": {
"source.fixAll": "explicit"
}
}
2 changes: 1 addition & 1 deletion webapp/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion webapp/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
},
"private": true,
"engines": {
"node": "22"
"node": ">=20.16 <23"
},
"dependencies": {
"@angular/animations": "18.2.1",
Expand Down
57 changes: 57 additions & 0 deletions webapp/src/app/components/leaderboard/leaderboard.component.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<div class="flex flex-col gap-4 py-8">
<h1 class="text-3xl font-bold">Artemis Leaderboard</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just have a leaderboard page component that is the smart component and separate out the table, I think this is the way.

We can skip having a story for the smart components for now, there we would need to mock the services in the stories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the leaderboard is already basically just a wrapper for the table-component with the required logic it feels like.
I guess we could also just move the heading to the main-component to make the separation more clear?
Is there an advantage to separate components I'm missing here?

Copy link
Collaborator

@FelixTJDietrich FelixTJDietrich Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still thinking about it 🤔 Okay maybe taking a step back this page would be called the home page or the dashboard page which fetches the data from the service(s) using a leaderboard component. But ideally the leaderboard component is dumb and does not require a dependency to a service, because this always makes testing or showing it in the storybook harder.

If we were to add for example some user actions for filtering the timeframe etc. the leaderboard component would have a callback/output/binding to do that but does not smartly also set the filter for the service. Does this make sense?

In the design system /ui the components should be rather generic and reusable. Having a more specialized component building the leaderboard using the code from the design system which also has a story also makes sense to me. I would like to show the leaderboard component in the storybook for documentation but we do not necessarily have to show the whole home page / dashboard page smart component in there too.

Copy link
Collaborator Author

@GODrums GODrums Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand where you are going with that idea now. I personally didn't think about that separation right away because we don't really plan on creating multiple leaderboards rather than one for all repos.

With your explanation I do see the point for it, although I feel like it creates an (unnecessary?) level of abstraction in this use case. Let's further discuss this in our next meeting maybe and rework in a follow-up PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in a follow-up. In my mind the leaderboard is just one component on the page. We will add more gamification elements on the same page but you are also right that we might create a leaderboard route per repo in the near future 🤔

@if (query.isPending()) {
<span class="text-muted-foreground">Data is loading...</span>
} @else if (query.error()) {
<span class="text-destructive">An error has occurred</span>
}

@if (leaderboard(); as leaderboard) {
<div class="px-4">
<app-table>
<thead appTableHeader>
<tr appTableRow>
<th appTableHead>Rank</th>
<th appTableHead>Contributor</th>
GODrums marked this conversation as resolved.
Show resolved Hide resolved
<th appTableHead>Score</th>
<th appTableHead>Activity</th>
</tr>
</thead>
<tbody appTableBody>
@for (entry of leaderboard; track entry.githubName) {
<tr appTableRow>
<td appTableCell>{{ entry.rank }}</td>
<td appTableCell>
<a href="https://github.com/{{ entry.githubName }}" target="_blank" rel="noopener noreferrer" class="flex items-center gap-2 font-medium">
<img src="{{ entry.avatarUrl }}" width="24" height="24" />
<span class="text-muted-foreground">{{ entry.name }}</span>
</a>
</td>
<td appTableCell>{{ entry.score }}</td>
<td appTableCell class="flex items-center gap-4" title="Changes Requested">
@if (entry.changesRequested && entry.changesRequested > 0) {
<div class="flex items-center gap-2">
<app-icon-pull-request-changes-requested />
{{ entry.changesRequested }}
</div>
}
@if (entry.approvals && entry.approvals > 0) {
<div class="flex items-center gap-2" title="Approvals">
<app-icon-pull-request-approved />
{{ entry.approvals }}
</div>
}
@if (entry.comments && entry.comments > 0) {
<div class="flex items-center gap-2" title="Comments">
<app-icon-pull-request-comment />
{{ entry.comments }}
</div>
}
</td>
</tr>
}
</tbody>
</app-table>
</div>
}
</div>
Loading