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.

Sign up for GitHub

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

Jump to bottom

Add pull requests component #92

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Add pull requests component #92

wants to merge 8 commits into from
+194 −7

Conversation

iam-flo
Copy link

@iam-flo iam-flo commented Sep 16, 2024 •
edited
Loading

Motivation

Adds a pull request widget component and the necessary data to display pull requests of a user

Description

  • extended Pull request model and api
  • add widget for pull request

Screenshots (if applicable)

Bildschirmfoto 2024-09-16 um 18 06 58

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
  • Screenshots have been attached (if applicable)
  • Documentation has been updated (if applicable)

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)

@iam-flo iam-flo self-assigned this Sep 16, 2024
@iam-flo iam-flo linked an issue Sep 16, 2024 that may be closed by this pull request
Pull request component for collaboration dashboard #82
Open
@github-actions github-actions bot added client size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 16, 2024
@github-actions github-actions bot added application-server 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 16, 2024
@iam-flo iam-flo changed the title DRAFT: create component for pull requests(#82) Create component for pull requests(#82) Sep 16, 2024
@FelixTJDietrich FelixTJDietrich changed the title Create component for pull requests(#82) Add pull requests component Sep 16, 2024
FelixTJDietrich
FelixTJDietrich requested changes Sep 16, 2024
View reviewed changes
webapp/src/app/core/modules/openapi/model/pull-request-label.ts


export interface PullRequestLabel {
name?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to make it not optional? I think if it exists it is always defined with name and color

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a general issue with JPA as OpenAPI Java-Variables can always be null implicitly. It applies to all our other model interfaces too.

...-server/src/main/java/de/tum/in/www1/hephaestus/codereview/pullrequest/PullRequestLabel.java
import lombok.NoArgsConstructor;
import lombok.Setter;

@Embeddable
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this means that there is no separate table for it am I right?

Copy link
Author

Choose a reason for hiding this comment

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

yes, but we could also create an own table for labels if we think that it is necessary. but then we have to handle the labels more

webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html Outdated Show resolved Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html Outdated Show resolved Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html Outdated Show resolved Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html Outdated Show resolved Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.ts Outdated Show resolved Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.stories.ts Outdated Show resolved Hide resolved
@FelixTJDietrich
Copy link
Collaborator

We also have colors defined for border and card that you can make use off

GODrums
GODrums requested changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@GODrums GODrums left a comment

Choose a reason for hiding this comment

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

I wasn't able to run storybook in the currently version due to a naming conflict of the number/prNumber-variable.

I would recommend that you rebase the branch to develop as it already contains the number-attribute.

...ation-server/src/main/java/de/tum/in/www1/hephaestus/codereview/pullrequest/PullRequest.java
@@ -36,6 +30,9 @@ public class PullRequest extends BaseGitServiceEntity {
@NonNull
private String url;

@NonNull
private int prNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

I already included the number-variable this in another PR . When you bring this branch up-to-date with develop you can just use that one instead.

webapp/src/app/core/modules/openapi/model/pull-request-label.ts


export interface PullRequestLabel {
name?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a general issue with JPA as OpenAPI Java-Variables can always be null implicitly. It applies to all our other model interfaces too.

webapp/src/app/ui/app-issue-card/app-issue-card.component.html
<div class="border border-gray-300 rounded-lg p-4 w-72">
<div class="flex justify-between items-center mb-2 text-xs text-gray-500">
<span class="font-bold flex justify-center items-center space-x-1">
<ng-icon [svg]="octGitPullRequest" size="16" class="mr-1"></ng-icon> {{ pullRequest().repository.name }} #{{ pullRequest().number }} on {{ pullRequest().createdAt }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to handle the null-case for the repository here to avoid a compiler error.

I'd be fine with simple optional chaining:
{{ pullRequest().repository?.name }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GODrums GODrums GODrums requested changes

@FelixTJDietrich FelixTJDietrich Awaiting requested review from FelixTJDietrich

@milesha milesha Awaiting requested review from milesha

Requested changes must be addressed to merge this pull request.

Assignees

@iam-flo iam-flo

Labels
application-server client size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pull request component for collaboration dashboard
3 participants
@iam-flo @FelixTJDietrich @GODrums