From fa11c5be1657049eaf02ff0811fe228ddf873bdb Mon Sep 17 00:00:00 2001 From: Florian Ehrenstorfer Date: Fri, 31 Jan 2025 17:59:02 +0100 Subject: [PATCH 01/18] adjust changelog(#223) --- .../gitprovider/pullrequest/PullRequestRepository.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java index 44549b36..56700bdf 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java @@ -21,6 +21,7 @@ SELECT MIN(p.createdAt) ) Optional firstContributionByAuthorLogin(@Param("authorLogin") String authorLogin); + @Transactional @Query( """ SELECT p From 93fb071c0a5cfc70f51bec0d09f71b6a4309c1c5 Mon Sep 17 00:00:00 2001 From: Florian Ehrenstorfer Date: Fri, 7 Feb 2025 15:01:11 +0100 Subject: [PATCH 02/18] create individual endpoints for detection and integrate existing bad practices #239 --- .../hephaestus/activity/ActivityController.java | 6 ++++++ .../hephaestus/activity/ActivityService.java | 16 ++++++++++++++++ .../app/detector/bad_practice_detector.py | 10 ++-------- .../prompts/pullrequest_badpractice_detector.py | 9 +++++++-- .../intelligence-service/app/routers/detector.py | 10 +++------- 5 files changed, 34 insertions(+), 17 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java index 5cf5acf8..d8ece9f1 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java @@ -25,4 +25,10 @@ public ResponseEntity> detectBadPracticesByUser( List badPractices = activityService.detectBadPractices(login); return ResponseEntity.ok(badPractices); } + + @PostMapping("{prId}/badpractices/") + public ResponseEntity> detectBadPracticesByUserAndPr(@PathVariable Long prId) { + List badPractice = activityService.detectBadPractices(prId); + return ResponseEntity.ok(badPractice); + } } diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java index 38ad50ee..e9e7cb7b 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java @@ -91,4 +91,20 @@ public List detectBadPractices(String login) { } return detectedBadPractices.stream().map(PullRequestBadPracticeDTO::fromPullRequestBadPractice).toList(); } + + public List detectBadPractices(Long prId) { + logger.info("Detecting bad practices for PR: {}", prId); + + PullRequest pullRequest = pullRequestRepository.findById(prId).orElse(null); + if (pullRequest == null) { + throw new IllegalArgumentException("Pull request " + prId + " not found"); + } + + List existingBadPractices = pullRequestBadPracticeRepository.findByPullRequestId(prId); + existingBadPractices.forEach(existingBadPractice -> existingBadPractice.setResolved(true)); + pullRequestBadPracticeRepository.saveAll(existingBadPractices); + + List detectedBadPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest); + return detectedBadPractices.stream().map(PullRequestBadPracticeDTO::fromPullRequestBadPractice).toList(); + } } diff --git a/server/intelligence-service/app/detector/bad_practice_detector.py b/server/intelligence-service/app/detector/bad_practice_detector.py index 446633ed..d0fa8f56 100644 --- a/server/intelligence-service/app/detector/bad_practice_detector.py +++ b/server/intelligence-service/app/detector/bad_practice_detector.py @@ -7,18 +7,12 @@ from .prompts.pullrequest_badpractice_detector import BAD_PRACTICE_PROMPT_TEST from ..model import model - -class PullRequest(BaseModel): - id: str - title: str - description: str - - class BadPractice(BaseModel): """A detected bad practice in a pull request.""" title: str = Field(description="The title of the bad practice.") description: str = Field(description="The description of the bad practice.") + resolved: bool = Field(description="Whether the bad practice has been resolved.") class BadPracticeList(BaseModel): @@ -32,7 +26,7 @@ class BadPracticeList(BaseModel): def detect_bad_practices(title, description) -> BadPracticeList: prompt_text = BAD_PRACTICE_PROMPT_TEST prompt_template = ChatPromptTemplate.from_template(prompt_text) - prompt = prompt_template.invoke({"title": title, "description": description}) + prompt = prompt_template.invoke({"title": title, "description": description, "bad_practices": bad_practices}) structured_llm = model.with_structured_output(BadPracticeList) response = structured_llm.invoke(prompt) return response diff --git a/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py b/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py index cc8de25f..5bb0ab33 100644 --- a/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py +++ b/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py @@ -7,7 +7,11 @@ Detect and identify any bad practices in the provided pull request title and description. - Review the title and description for any issues or violations of the guidelines. - For each detected bad practice, provide a title and a brief description of the issue. -- Return a list of all detected bad practices in the pull request. +- Check the list of existing bad practices and add any new bad practices to the list. +- If the same bad practice is detected multiple times, return it consistently in the same way. +- If a bad practice was resolved, meaning it is no longer present in the title or description, mark it as resolved. +- If a bad practice was marked as resolved, but is detected again, mark it as detected again. +- Return a list of all detected and all resolved bad practices in the pull request. GUIDELINES: 1. The title and description should be clear, concise, and descriptive. @@ -30,4 +34,5 @@ 6. Multiple runs on the same title and description should return the same results if nothing has changed. Pull Request Title: {title} -Pull Request Description: {description}""" +Pull Request Description: {description} +Existing Bad Practices: {bad_practices}""" diff --git a/server/intelligence-service/app/routers/detector.py b/server/intelligence-service/app/routers/detector.py index fa7856c7..7638c48f 100644 --- a/server/intelligence-service/app/routers/detector.py +++ b/server/intelligence-service/app/routers/detector.py @@ -3,12 +3,7 @@ from fastapi import APIRouter from openai import BaseModel -from ..detector.bad_practice_detector import ( - PullRequest, - detect_bad_practices, - BadPracticeList, - BadPractice, -) +from ..detector.bad_practice_detector import detectbadpractices, BadPractice router = APIRouter(prefix="/detector", tags=["detector"]) @@ -16,6 +11,7 @@ class DetectorRequest(BaseModel): title: str description: str + bad_practices: List[BadPractice] class DetectorResponse(BaseModel): @@ -28,4 +24,4 @@ class DetectorResponse(BaseModel): summary="Detect bad practices for given pull request.", ) def detect(request: DetectorRequest): - return detectbadpractices(request.title, request.description) + return detectbadpractices(request.title, request.description, request.bad_practices) From 20dce0f7f3a5ad761f82018565cf418770b6779e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 7 Feb 2025 14:11:08 +0000 Subject: [PATCH 03/18] chore: update API specs and client --- .../intelligenceservice/api/DetectorApi.java | 4 +- .../model/BadPractice.java | 33 +++++++++++- .../model/DetectorRequest.java | 53 +++++++++++++++++-- .../model/DetectorResponse.java | 8 +-- server/intelligence-service/openapi.yaml | 13 ++++- 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/api/DetectorApi.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/api/DetectorApi.java index 7a6b081f..ddf83d6c 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/api/DetectorApi.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/api/DetectorApi.java @@ -40,7 +40,7 @@ public DetectorApi(ApiClient apiClient) { } /** - * Detect bad practices given rules. + * Detect bad practices for given pull request. * *

200 - Successful Response *

422 - Validation Error @@ -53,7 +53,7 @@ public DetectorResponse detectDetectorPost(DetectorRequest detectorRequest) thro } /** - * Detect bad practices given rules. + * Detect bad practices for given pull request. * *

200 - Successful Response *

422 - Validation Error diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/BadPractice.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/BadPractice.java index d251392e..02f5eaf1 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/BadPractice.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/BadPractice.java @@ -29,6 +29,7 @@ */ @JsonPropertyOrder({ BadPractice.JSON_PROPERTY_DESCRIPTION, + BadPractice.JSON_PROPERTY_RESOLVED, BadPractice.JSON_PROPERTY_TITLE }) @jakarta.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", comments = "Generator version: 7.7.0") @@ -36,6 +37,9 @@ public class BadPractice { public static final String JSON_PROPERTY_DESCRIPTION = "description"; private String description; + public static final String JSON_PROPERTY_RESOLVED = "resolved"; + private Boolean resolved; + public static final String JSON_PROPERTY_TITLE = "title"; private String title; @@ -67,6 +71,31 @@ public void setDescription(String description) { this.description = description; } + public BadPractice resolved(Boolean resolved) { + + this.resolved = resolved; + return this; + } + + /** + * Whether the bad practice has been resolved. + * @return resolved + */ + @jakarta.annotation.Nonnull + @JsonProperty(JSON_PROPERTY_RESOLVED) + @JsonInclude(value = JsonInclude.Include.ALWAYS) + + public Boolean getResolved() { + return resolved; + } + + + @JsonProperty(JSON_PROPERTY_RESOLVED) + @JsonInclude(value = JsonInclude.Include.ALWAYS) + public void setResolved(Boolean resolved) { + this.resolved = resolved; + } + public BadPractice title(String title) { this.title = title; @@ -102,12 +131,13 @@ public boolean equals(Object o) { } BadPractice badPractice = (BadPractice) o; return Objects.equals(this.description, badPractice.description) && + Objects.equals(this.resolved, badPractice.resolved) && Objects.equals(this.title, badPractice.title); } @Override public int hashCode() { - return Objects.hash(description, title); + return Objects.hash(description, resolved, title); } @Override @@ -115,6 +145,7 @@ public String toString() { StringBuilder sb = new StringBuilder(); sb.append("class BadPractice {\n"); sb.append(" description: ").append(toIndentedString(description)).append("\n"); + sb.append(" resolved: ").append(toIndentedString(resolved)).append("\n"); sb.append(" title: ").append(toIndentedString(title)).append("\n"); sb.append("}"); return sb.toString(); diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java index d859f9f4..f5864d43 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java @@ -20,7 +20,11 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonTypeName; import com.fasterxml.jackson.annotation.JsonValue; +import de.tum.in.www1.hephaestus.intelligenceservice.model.BadPractice; +import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import com.fasterxml.jackson.annotation.JsonPropertyOrder; import com.fasterxml.jackson.annotation.JsonTypeName; @@ -30,11 +34,15 @@ * DetectorRequest */ @JsonPropertyOrder({ + DetectorRequest.JSON_PROPERTY_BAD_PRACTICES, DetectorRequest.JSON_PROPERTY_DESCRIPTION, DetectorRequest.JSON_PROPERTY_TITLE }) @jakarta.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", comments = "Generator version: 7.7.0") -public class DetectorRequest { +public class DetectorRequest extends HashMap { + public static final String JSON_PROPERTY_BAD_PRACTICES = "bad_practices"; + private List badPractices = new ArrayList<>(); + public static final String JSON_PROPERTY_DESCRIPTION = "description"; private String description; @@ -45,6 +53,39 @@ public DetectorRequest() { } + public DetectorRequest badPractices(List badPractices) { + + this.badPractices = badPractices; + return this; + } + + public DetectorRequest addBadPracticesItem(BadPractice badPracticesItem) { + if (this.badPractices == null) { + this.badPractices = new ArrayList<>(); + } + this.badPractices.add(badPracticesItem); + return this; + } + + /** + * Get badPractices + * @return badPractices + */ + @jakarta.annotation.Nonnull + @JsonProperty(JSON_PROPERTY_BAD_PRACTICES) + @JsonInclude(value = JsonInclude.Include.ALWAYS) + + public List getBadPractices() { + return badPractices; + } + + + @JsonProperty(JSON_PROPERTY_BAD_PRACTICES) + @JsonInclude(value = JsonInclude.Include.ALWAYS) + public void setBadPractices(List badPractices) { + this.badPractices = badPractices; + } + public DetectorRequest description(String description) { this.description = description; @@ -104,19 +145,23 @@ public boolean equals(Object o) { return false; } DetectorRequest detectorRequest = (DetectorRequest) o; - return Objects.equals(this.description, detectorRequest.description) && - Objects.equals(this.title, detectorRequest.title); + return Objects.equals(this.badPractices, detectorRequest.badPractices) && + Objects.equals(this.description, detectorRequest.description) && + Objects.equals(this.title, detectorRequest.title) && + super.equals(o); } @Override public int hashCode() { - return Objects.hash(description, title); + return Objects.hash(badPractices, description, title, super.hashCode()); } @Override public String toString() { StringBuilder sb = new StringBuilder(); sb.append("class DetectorRequest {\n"); + sb.append(" ").append(toIndentedString(super.toString())).append("\n"); + sb.append(" badPractices: ").append(toIndentedString(badPractices)).append("\n"); sb.append(" description: ").append(toIndentedString(description)).append("\n"); sb.append(" title: ").append(toIndentedString(title)).append("\n"); sb.append("}"); diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java index adbd004a..1187bbcc 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java @@ -37,7 +37,7 @@ DetectorResponse.JSON_PROPERTY_BAD_PRACTICES }) @jakarta.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", comments = "Generator version: 7.7.0") -public class DetectorResponse { +public class DetectorResponse extends HashMap { public static final String JSON_PROPERTY_BAD_PRACTICES = "bad_practices"; private List badPractices = new ArrayList<>(); @@ -87,18 +87,20 @@ public boolean equals(Object o) { return false; } DetectorResponse detectorResponse = (DetectorResponse) o; - return Objects.equals(this.badPractices, detectorResponse.badPractices); + return Objects.equals(this.badPractices, detectorResponse.badPractices) && + super.equals(o); } @Override public int hashCode() { - return Objects.hash(badPractices); + return Objects.hash(badPractices, super.hashCode()); } @Override public String toString() { StringBuilder sb = new StringBuilder(); sb.append("class DetectorResponse {\n"); + sb.append(" ").append(toIndentedString(super.toString())).append("\n"); sb.append(" badPractices: ").append(toIndentedString(badPractices)).append("\n"); sb.append("}"); return sb.toString(); diff --git a/server/intelligence-service/openapi.yaml b/server/intelligence-service/openapi.yaml index ac10733a..fe973d22 100644 --- a/server/intelligence-service/openapi.yaml +++ b/server/intelligence-service/openapi.yaml @@ -7,6 +7,10 @@ components: description: The description of the bad practice. title: Description type: string + resolved: + description: Whether the bad practice has been resolved. + title: Resolved + type: boolean title: description: The title of the bad practice. title: Title @@ -14,11 +18,17 @@ components: required: - title - description + - resolved title: BadPractice type: object DetectorRequest: additionalProperties: true properties: + bad_practices: + items: + $ref: '#/components/schemas/BadPractice' + title: Bad Practices + type: array description: title: Description type: string @@ -28,6 +38,7 @@ components: required: - title - description + - bad_practices title: DetectorRequest type: object DetectorResponse: @@ -152,7 +163,7 @@ paths: schema: $ref: '#/components/schemas/HTTPValidationError' description: Validation Error - summary: Detect bad practices given rules. + summary: Detect bad practices for given pull request. tags: - detector /health: From d6cd748d2069ba18728fdcbcbd0a4eee5edf44e6 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 7 Feb 2025 17:00:23 +0000 Subject: [PATCH 04/18] chore: update API specs and client --- server/application-server/openapi.yaml | 21 +++++++ .../modules/openapi/api/activity.service.ts | 63 +++++++++++++++++++ .../openapi/api/activity.serviceInterface.ts | 7 +++ 3 files changed, 91 insertions(+) diff --git a/server/application-server/openapi.yaml b/server/application-server/openapi.yaml index 16454d0c..02fc0347 100644 --- a/server/application-server/openapi.yaml +++ b/server/application-server/openapi.yaml @@ -288,6 +288,27 @@ paths: application/json: schema: $ref: "#/components/schemas/Message" + /activity/{prId}/badpractices/: + post: + tags: + - activity + operationId: detectBadPracticesByUserAndPr + parameters: + - name: prId + in: path + required: true + schema: + type: integer + format: int64 + responses: + "200": + description: OK + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/PullRequestBadPractice" /activity/{login}/badpractices: post: tags: diff --git a/webapp/src/app/core/modules/openapi/api/activity.service.ts b/webapp/src/app/core/modules/openapi/api/activity.service.ts index 3d7fc27e..d418c5b2 100644 --- a/webapp/src/app/core/modules/openapi/api/activity.service.ts +++ b/webapp/src/app/core/modules/openapi/api/activity.service.ts @@ -160,6 +160,69 @@ export class ActivityService implements ActivityServiceInterface { ); } + /** + * @param prId + * @param observe set whether or not to return the data Observable as the body, response or events. defaults to returning the body. + * @param reportProgress flag to report request and response progress. + */ + public detectBadPracticesByUserAndPr(prId: number, observe?: 'body', reportProgress?: boolean, options?: {httpHeaderAccept?: 'application/json', context?: HttpContext, transferCache?: boolean}): Observable>; + public detectBadPracticesByUserAndPr(prId: number, observe?: 'response', reportProgress?: boolean, options?: {httpHeaderAccept?: 'application/json', context?: HttpContext, transferCache?: boolean}): Observable>>; + public detectBadPracticesByUserAndPr(prId: number, observe?: 'events', reportProgress?: boolean, options?: {httpHeaderAccept?: 'application/json', context?: HttpContext, transferCache?: boolean}): Observable>>; + public detectBadPracticesByUserAndPr(prId: number, observe: any = 'body', reportProgress: boolean = false, options?: {httpHeaderAccept?: 'application/json', context?: HttpContext, transferCache?: boolean}): Observable { + if (prId === null || prId === undefined) { + throw new Error('Required parameter prId was null or undefined when calling detectBadPracticesByUserAndPr.'); + } + + let localVarHeaders = this.defaultHeaders; + + let localVarHttpHeaderAcceptSelected: string | undefined = options && options.httpHeaderAccept; + if (localVarHttpHeaderAcceptSelected === undefined) { + // to determine the Accept header + const httpHeaderAccepts: string[] = [ + 'application/json' + ]; + localVarHttpHeaderAcceptSelected = this.configuration.selectHeaderAccept(httpHeaderAccepts); + } + if (localVarHttpHeaderAcceptSelected !== undefined) { + localVarHeaders = localVarHeaders.set('Accept', localVarHttpHeaderAcceptSelected); + } + + let localVarHttpContext: HttpContext | undefined = options && options.context; + if (localVarHttpContext === undefined) { + localVarHttpContext = new HttpContext(); + } + + let localVarTransferCache: boolean | undefined = options && options.transferCache; + if (localVarTransferCache === undefined) { + localVarTransferCache = true; + } + + + let responseType_: 'text' | 'json' | 'blob' = 'json'; + if (localVarHttpHeaderAcceptSelected) { + if (localVarHttpHeaderAcceptSelected.startsWith('text')) { + responseType_ = 'text'; + } else if (this.configuration.isJsonMime(localVarHttpHeaderAcceptSelected)) { + responseType_ = 'json'; + } else { + responseType_ = 'blob'; + } + } + + let localVarPath = `/activity/${this.configuration.encodeParam({name: "prId", value: prId, in: "path", style: "simple", explode: false, dataType: "number", dataFormat: "int64"})}/badpractices/`; + return this.httpClient.request>('post', `${this.configuration.basePath}${localVarPath}`, + { + context: localVarHttpContext, + responseType: responseType_, + withCredentials: this.configuration.withCredentials, + headers: localVarHeaders, + observe: observe, + transferCache: localVarTransferCache, + reportProgress: reportProgress + } + ); + } + /** * @param login * @param observe set whether or not to return the data Observable as the body, response or events. defaults to returning the body. diff --git a/webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts b/webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts index 1c33bdd3..488faa3f 100644 --- a/webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts +++ b/webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts @@ -32,6 +32,13 @@ export interface ActivityServiceInterface { */ detectBadPracticesByUser(login: string, extraHttpRequestParams?: any): Observable>; + /** + * + * + * @param prId + */ + detectBadPracticesByUserAndPr(prId: number, extraHttpRequestParams?: any): Observable>; + /** * * From ab8e9abecc6f41aea75cd334c315a221fbf18a67 Mon Sep 17 00:00:00 2001 From: Florian Ehrenstorfer Date: Fri, 7 Feb 2025 18:25:57 +0100 Subject: [PATCH 05/18] add detect to individual pr #243 --- .../activity/ActivityController.java | 6 ++-- .../hephaestus/activity/ActivityService.java | 17 +++------- .../PullRequestBadPracticeDetector.java | 32 +++++++++++++++++-- .../pullrequest/PullRequestRepository.java | 1 - .../model/DetectorRequest.java | 8 ++--- .../model/DetectorResponse.java | 7 ++-- .../pullrequest_badpractice_detector.py | 2 +- .../activity-dashboard.component.html | 1 + ...l-request-bad-practice-card.component.html | 6 +++- ...ull-request-bad-practice-card.component.ts | 26 +++++++++++++-- 10 files changed, 72 insertions(+), 34 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java index d8ece9f1..24c301c8 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java @@ -22,13 +22,13 @@ public ResponseEntity getActivityByUser(@PathVariable String login) @PostMapping("/{login}/badpractices") public ResponseEntity> detectBadPracticesByUser(@PathVariable String login) { - List badPractices = activityService.detectBadPractices(login); + List badPractices = activityService.detectBadPracticesForUser(login); return ResponseEntity.ok(badPractices); } @PostMapping("{prId}/badpractices/") - public ResponseEntity> detectBadPracticesByUserAndPr(@PathVariable Long prId) { - List badPractice = activityService.detectBadPractices(prId); + public ResponseEntity> detectBadPracticesByPr(@PathVariable Long prId) { + List badPractice = activityService.detectBadPracticesForPr(prId); return ResponseEntity.ok(badPractice); } } diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java index e9e7cb7b..97f31771 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java @@ -48,7 +48,6 @@ public ActivityDTO getActivity(String login) { Map> pullRequestBadPracticesMap = openPulLRequestBadPractices .stream() - .filter(pullRequestBadPractice -> !pullRequestBadPractice.isResolved()) .collect( Collectors.groupingBy( PullRequestBadPractice::getPullrequest, @@ -71,7 +70,8 @@ public ActivityDTO getActivity(String login) { return new ActivityDTO(openPullRequestsWithBadPractices); } - public List detectBadPractices(String login) { + @Transactional + public List detectBadPracticesForUser(String login) { logger.info("Detecting bad practices for user with login: {}", login); List pullRequests = pullRequestRepository.findAssignedByLoginAndStates( @@ -79,12 +79,6 @@ public List detectBadPractices(String login) { Set.of(Issue.State.OPEN) ); - List existingBadPractices = pullRequestBadPracticeRepository.findAssignedByLoginAndOpen( - login - ); - existingBadPractices.forEach(existingBadPractice -> existingBadPractice.setResolved(true)); - pullRequestBadPracticeRepository.saveAll(existingBadPractices); - List detectedBadPractices = new ArrayList<>(); for (PullRequest pullRequest : pullRequests) { detectedBadPractices.addAll(pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest)); @@ -92,7 +86,8 @@ public List detectBadPractices(String login) { return detectedBadPractices.stream().map(PullRequestBadPracticeDTO::fromPullRequestBadPractice).toList(); } - public List detectBadPractices(Long prId) { + @Transactional + public List detectBadPracticesForPr(Long prId) { logger.info("Detecting bad practices for PR: {}", prId); PullRequest pullRequest = pullRequestRepository.findById(prId).orElse(null); @@ -100,10 +95,6 @@ public List detectBadPractices(Long prId) { throw new IllegalArgumentException("Pull request " + prId + " not found"); } - List existingBadPractices = pullRequestBadPracticeRepository.findByPullRequestId(prId); - existingBadPractices.forEach(existingBadPractice -> existingBadPractice.setResolved(true)); - pullRequestBadPracticeRepository.saveAll(existingBadPractices); - List detectedBadPractices = pullRequestBadPracticeDetector.detectAndSyncBadPractices(pullRequest); return detectedBadPractices.stream().map(PullRequestBadPracticeDTO::fromPullRequestBadPractice).toList(); } diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java index ee11e27f..88a79a7c 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java @@ -33,27 +33,53 @@ public class PullRequestBadPracticeDetector { */ public List detectAndSyncBadPractices(PullRequest pullRequest) { logger.info("Detecting bad practices for pull request: {}", pullRequest.getId()); + + List existingBadPractices = pullRequestBadPracticeRepository.findByPullRequestId(pullRequest.getId()); + DetectorRequest detectorRequest = new DetectorRequest(); detectorRequest.setDescription(pullRequest.getBody()); detectorRequest.setTitle(pullRequest.getTitle()); + detectorRequest.setBadPractices(existingBadPractices.stream().map(this::convertToIntelligenceBadPractice).toList()); DetectorResponse detectorResponse = detectorApi.detectDetectorPost(detectorRequest); List detectedBadPractices = new LinkedList<>(); + // Check if there are returned bad practices in the response with the same title as an existing bad practice for (BadPractice badPractice : detectorResponse.getBadPractices()) { - detectedBadPractices.add(handleDetectedBadPractices(pullRequest, badPractice)); + boolean exists = false; + for (PullRequestBadPractice existingBadPractice : existingBadPractices) { + if (existingBadPractice.getTitle().equals(badPractice.getTitle())) { + existingBadPractice.setDescription(badPractice.getDescription()); + existingBadPractice.setResolved(badPractice.getResolved()); + detectedBadPractices.add(pullRequestBadPracticeRepository.save(existingBadPractice)); + exists = true; + break; + } + } + if (!exists) { + detectedBadPractices.add(saveDetectedBadPractices(pullRequest, badPractice)); + } } logger.info("Detected {} bad practices for pull request: {}", detectedBadPractices.size(), pullRequest.getId()); return detectedBadPractices; } - protected PullRequestBadPractice handleDetectedBadPractices(PullRequest pullRequest, BadPractice badPractice) { + protected PullRequestBadPractice saveDetectedBadPractices(PullRequest pullRequest, BadPractice badPractice) { + PullRequestBadPractice pullRequestBadPractice = new PullRequestBadPractice(); pullRequestBadPractice.setTitle(badPractice.getTitle()); pullRequestBadPractice.setDescription(badPractice.getDescription()); pullRequestBadPractice.setPullrequest(pullRequest); - pullRequestBadPractice.setResolved(false); + pullRequestBadPractice.setResolved(badPractice.getResolved()); return pullRequestBadPracticeRepository.save(pullRequestBadPractice); } + + private BadPractice convertToIntelligenceBadPractice(PullRequestBadPractice pullRequestBadPractice) { + BadPractice badPractice = new BadPractice(); + badPractice.setTitle(pullRequestBadPractice.getTitle()); + badPractice.setDescription(pullRequestBadPractice.getDescription()); + badPractice.setResolved(pullRequestBadPractice.isResolved()); + return badPractice; + } } diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java index 56700bdf..44549b36 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/PullRequestRepository.java @@ -21,7 +21,6 @@ SELECT MIN(p.createdAt) ) Optional firstContributionByAuthorLogin(@Param("authorLogin") String authorLogin); - @Transactional @Query( """ SELECT p diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java index f5864d43..9c32957b 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java @@ -39,7 +39,7 @@ DetectorRequest.JSON_PROPERTY_TITLE }) @jakarta.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", comments = "Generator version: 7.7.0") -public class DetectorRequest extends HashMap { +public class DetectorRequest { public static final String JSON_PROPERTY_BAD_PRACTICES = "bad_practices"; private List badPractices = new ArrayList<>(); @@ -147,20 +147,18 @@ public boolean equals(Object o) { DetectorRequest detectorRequest = (DetectorRequest) o; return Objects.equals(this.badPractices, detectorRequest.badPractices) && Objects.equals(this.description, detectorRequest.description) && - Objects.equals(this.title, detectorRequest.title) && - super.equals(o); + Objects.equals(this.title, detectorRequest.title); } @Override public int hashCode() { - return Objects.hash(badPractices, description, title, super.hashCode()); + return Objects.hash(badPractices, description, title); } @Override public String toString() { StringBuilder sb = new StringBuilder(); sb.append("class DetectorRequest {\n"); - sb.append(" ").append(toIndentedString(super.toString())).append("\n"); sb.append(" badPractices: ").append(toIndentedString(badPractices)).append("\n"); sb.append(" description: ").append(toIndentedString(description)).append("\n"); sb.append(" title: ").append(toIndentedString(title)).append("\n"); diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java index 1187bbcc..54c84ad8 100644 --- a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorResponse.java @@ -37,7 +37,7 @@ DetectorResponse.JSON_PROPERTY_BAD_PRACTICES }) @jakarta.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", comments = "Generator version: 7.7.0") -public class DetectorResponse extends HashMap { +public class DetectorResponse { public static final String JSON_PROPERTY_BAD_PRACTICES = "bad_practices"; private List badPractices = new ArrayList<>(); @@ -87,13 +87,12 @@ public boolean equals(Object o) { return false; } DetectorResponse detectorResponse = (DetectorResponse) o; - return Objects.equals(this.badPractices, detectorResponse.badPractices) && - super.equals(o); + return Objects.equals(this.badPractices, detectorResponse.badPractices); } @Override public int hashCode() { - return Objects.hash(badPractices, super.hashCode()); + return Objects.hash(badPractices); } @Override diff --git a/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py b/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py index 5bb0ab33..443fe11d 100644 --- a/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py +++ b/server/intelligence-service/app/detector/prompts/pullrequest_badpractice_detector.py @@ -9,7 +9,7 @@ - For each detected bad practice, provide a title and a brief description of the issue. - Check the list of existing bad practices and add any new bad practices to the list. - If the same bad practice is detected multiple times, return it consistently in the same way. -- If a bad practice was resolved, meaning it is no longer present in the title or description, mark it as resolved. +- Check each existing bad practice. If a bad practice was resolved, meaning it is no longer present in the title or description, mark it as resolved. - If a bad practice was marked as resolved, but is detected again, mark it as detected again. - Return a list of all detected and all resolved bad practices in the pull request. diff --git a/webapp/src/app/home/activity/activity-dashboard.component.html b/webapp/src/app/home/activity/activity-dashboard.component.html index 32ea949b..6687121c 100644 --- a/webapp/src/app/home/activity/activity-dashboard.component.html +++ b/webapp/src/app/home/activity/activity-dashboard.component.html @@ -22,6 +22,7 @@

Your open pull requests

@if (query.data()?.pullRequests) { @for (pullRequest of query.data()?.pullRequests; track pullRequest.id) { - + @if (isLoading()) { @@ -21,6 +21,10 @@ } @else { {{ badPractices()?.length }} bad practices detected } + diff --git a/webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts b/webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts index fa6dbc24..46be808e 100644 --- a/webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts +++ b/webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts @@ -1,7 +1,18 @@ -import { Component, computed, input } from '@angular/core'; -import { PullRequestInfo, LabelInfo, PullRequestBadPractice } from '@app/core/modules/openapi'; +import { Component, computed, inject, input } from '@angular/core'; +import { PullRequestInfo, LabelInfo, PullRequestBadPractice, ActivityService } from '@app/core/modules/openapi'; import { NgIcon } from '@ng-icons/core'; -import { octCheck, octComment, octFileDiff, octGitPullRequest, octGitPullRequestClosed, octGitPullRequestDraft, octGitMerge, octX, octFold } from '@ng-icons/octicons'; +import { + octCheck, + octComment, + octFileDiff, + octGitPullRequest, + octGitPullRequestClosed, + octGitPullRequestDraft, + octGitMerge, + octX, + octFold, + octSync +} from '@ng-icons/octicons'; import { HlmCardModule } from '@spartan-ng/ui-card-helm'; import { HlmSkeletonComponent } from '@spartan-ng/ui-skeleton-helm'; @@ -33,6 +44,8 @@ import { formatTitle } from '@app/utils'; ] }) export class PullRequestBadPracticeCardComponent { + activityService = inject(ActivityService); + protected readonly octCheck = octCheck; protected readonly octX = octX; protected readonly octComment = octComment; @@ -41,6 +54,7 @@ export class PullRequestBadPracticeCardComponent { isLoading = input(false); class = input(''); + id = input.required(); title = input(); number = input(); additions = input(); @@ -82,4 +96,10 @@ export class PullRequestBadPracticeCardComponent { return { icon, color }; }); + protected readonly octSync = octSync; + + detectBadPractices = () => { + console.log('Detecting bad practices for PR ' + this.id()); + this.activityService.detectBadPracticesByUserAndPr(this.id()).subscribe(); + }; } From 4a6fdf105c2a593968c54e7709a86ac43b647167 Mon Sep 17 00:00:00 2001 From: Florian Ehrenstorfer Date: Sun, 9 Feb 2025 13:31:09 +0100 Subject: [PATCH 06/18] create task to detect bad practices after ready to review #243 --- .../BadPracticeDetectorScheduler.java | 25 +++++++++++++++++++ .../BadPracticeDetectorTask.java | 23 +++++++++++++++++ .../github/GitHubPullRequestSyncService.java | 5 ++++ 3 files changed, 53 insertions(+) create mode 100644 server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java create mode 100644 server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorTask.java diff --git a/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java new file mode 100644 index 00000000..f11ce1bd --- /dev/null +++ b/server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java @@ -0,0 +1,25 @@ +package de.tum.in.www1.hephaestus.activity.badpracticedetector; + +import de.tum.in.www1.hephaestus.gitprovider.label.Label; +import de.tum.in.www1.hephaestus.gitprovider.pullrequest.PullRequest; +import org.springframework.stereotype.Component; + +import java.util.Set; + +@Component +public class BadPracticeDetectorScheduler { + + private final String readyToMerge = "ready to merge"; + + + public void detectBadPracticeForPrIfReadyToMerge(PullRequest pullRequest, Set