From 1082fa3f43e1d70308672d3016c3b2b25f566ecf Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Sun, 3 Dec 2023 16:48:36 +0100 Subject: [PATCH 1/8] make delete operation depend on text instead of id --- .../shared/code-editor/ace/code-editor-ace.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts index eab419a00bcd..9c5d4c307a91 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts @@ -637,7 +637,7 @@ export class CodeEditorAceComponent implements AfterViewInit, OnChanges, OnDestr * @param feedback Feedback to be removed */ deleteFeedback(feedback: Feedback) { - this.feedbacks = this.feedbacks.filter((f) => f.id !== feedback.id); + this.feedbacks = this.feedbacks.filter((f) => f.text !== feedback.text); this.removeLineWidget(Feedback.getReferenceLine(feedback)!); this.onUpdateFeedback.emit(this.feedbacks); } From cd02ad071283db96f5efeca20c50a1d3229c5534 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Sun, 3 Dec 2023 17:19:51 +0100 Subject: [PATCH 2/8] make equality dependent on multiple variables --- src/main/webapp/app/entities/feedback.model.ts | 10 ++++++++++ .../code-editor/ace/code-editor-ace.component.ts | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/main/webapp/app/entities/feedback.model.ts b/src/main/webapp/app/entities/feedback.model.ts index 8994193c7559..3c1d97cd8000 100644 --- a/src/main/webapp/app/entities/feedback.model.ts +++ b/src/main/webapp/app/entities/feedback.model.ts @@ -149,6 +149,16 @@ export class Feedback implements BaseEntity { return Feedback.hasDetailText(that) || !!that.gradingInstruction?.feedback; } + /** + * Checks for equality of two feedbacks. Only checking the ids is not enough because they are undefined for inline + * feedbacks before they are saved. + * @param f1 The feedback that is compared to f2 + * @param f2 The feedback that is compared to f1 + */ + public static areIdentical(f1: Feedback, f2: Feedback) { + return f1.id === f2.id && f1.text === f2.text && f1.detailText === f2.detailText; + } + /** * Get the referenced file path for referenced programming feedbacks, or undefined. * Typical reference format for programming feedback: `file:src/com/example/package/MyClass.java_line:13`. diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts index 9c5d4c307a91..d59c3ed6201a 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/ace/code-editor-ace.component.ts @@ -637,7 +637,7 @@ export class CodeEditorAceComponent implements AfterViewInit, OnChanges, OnDestr * @param feedback Feedback to be removed */ deleteFeedback(feedback: Feedback) { - this.feedbacks = this.feedbacks.filter((f) => f.text !== feedback.text); + this.feedbacks = this.feedbacks.filter((f) => !Feedback.areIdentical(f, feedback)); this.removeLineWidget(Feedback.getReferenceLine(feedback)!); this.onUpdateFeedback.emit(this.feedbacks); } From 8fe6e94d433a3c090b030e54820e098a0f832afd Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Sun, 3 Dec 2023 19:17:34 +0100 Subject: [PATCH 3/8] add tests for feedback deletion --- .../code-editor-ace.component.spec.ts | 122 ++++++++++++++++-- 1 file changed, 112 insertions(+), 10 deletions(-) diff --git a/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts index 73f73a14baa4..2e8e941ed662 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts @@ -129,7 +129,13 @@ describe('CodeEditorAceComponent', () => { await fixture.whenStable(); expect(comp.isLoading).toBeFalse(); - expect(comp.fileSession).toEqual({ dummy: { code: 'lorem ipsum', cursor: { column: 0, row: 0 }, loadingError: false } }); + expect(comp.fileSession).toEqual({ + dummy: { + code: 'lorem ipsum', + cursor: { column: 0, row: 0 }, + loadingError: false, + }, + }); expect(initEditorSpy).toHaveBeenCalledWith(); })); @@ -218,7 +224,10 @@ describe('CodeEditorAceComponent', () => { await comp.onFileChange(fileChange); - expect(comp.fileSession).toEqual({ anotherFile: fileSession.anotherFile, [fileChange.newFileName]: fileSession[selectedFile] }); + expect(comp.fileSession).toEqual({ + anotherFile: fileSession.anotherFile, + [fileChange.newFileName]: fileSession[selectedFile], + }); }); it('should init editor on newly created file if selected', async () => { @@ -232,7 +241,10 @@ describe('CodeEditorAceComponent', () => { await comp.onFileChange(fileChange); expect(initEditorSpy).toHaveBeenCalledWith(); - expect(comp.fileSession).toEqual({ anotherFile: fileSession.anotherFile, [fileChange.fileName]: { code: '', cursor: { row: 0, column: 0 }, loadingError: false } }); + expect(comp.fileSession).toEqual({ + anotherFile: fileSession.anotherFile, + [fileChange.fileName]: { code: '', cursor: { row: 0, column: 0 }, loadingError: false }, + }); }); it('should not do anything on file content change if the code has not changed', () => { @@ -267,7 +279,16 @@ describe('CodeEditorAceComponent', () => { await comp.onFileTextChanged(newFileContent); expect(onFileContentChangeSpy).toHaveBeenCalledWith({ file: selectedFile, fileContent: newFileContent }); - const newAnnotations = [{ fileName: selectedFile, text: 'error', type: 'error', timestamp: 0, row: 4, column: 4 }]; + const newAnnotations = [ + { + fileName: selectedFile, + text: 'error', + type: 'error', + timestamp: 0, + row: 4, + column: 4, + }, + ]; expect(comp.annotationsArray).toEqual(newAnnotations); }); @@ -359,16 +380,33 @@ describe('CodeEditorAceComponent', () => { it('should convert an accepted feedback suggestion to a marked manual feedback', async () => { await comp.initEditor(); - const suggestion = { text: 'FeedbackSuggestion:', detailText: 'test', reference: 'file:src/Test.java_line:16', type: FeedbackType.MANUAL }; + const suggestion = { + text: 'FeedbackSuggestion:', + detailText: 'test', + reference: 'file:src/Test.java_line:16', + type: FeedbackType.MANUAL, + }; comp.feedbackSuggestions = [suggestion]; await comp.acceptSuggestion(suggestion); expect(comp.feedbackSuggestions).toBeEmpty(); - expect(comp.feedbacks).toEqual([{ text: 'FeedbackSuggestion:accepted:', detailText: 'test', reference: 'file:src/Test.java_line:16', type: FeedbackType.MANUAL }]); + expect(comp.feedbacks).toEqual([ + { + text: 'FeedbackSuggestion:accepted:', + detailText: 'test', + reference: 'file:src/Test.java_line:16', + type: FeedbackType.MANUAL, + }, + ]); }); it('should remove discarded suggestions', async () => { await comp.initEditor(); - const suggestion = { text: 'FeedbackSuggestion:', detailText: 'test', reference: 'file:src/Test.java_line:16', type: FeedbackType.MANUAL }; + const suggestion = { + text: 'FeedbackSuggestion:', + detailText: 'test', + reference: 'file:src/Test.java_line:16', + type: FeedbackType.MANUAL, + }; comp.feedbackSuggestions = [suggestion]; comp.discardSuggestion(suggestion); expect(comp.feedbackSuggestions).toBeEmpty(); @@ -380,11 +418,25 @@ describe('CodeEditorAceComponent', () => { await comp.initEditor(); // Change of feedbacks from the outside - await comp.ngOnChanges({ feedbacks: { previousValue: [], currentValue: [new Feedback()], firstChange: true, isFirstChange: () => true } }); + await comp.ngOnChanges({ + feedbacks: { + previousValue: [], + currentValue: [new Feedback()], + firstChange: true, + isFirstChange: () => true, + }, + }); expect(updateLineWidgetHeightSpy).toHaveBeenCalled(); // Change of feedback suggestions from the outside - await comp.ngOnChanges({ feedbackSuggestions: { previousValue: [], currentValue: [new Feedback()], firstChange: true, isFirstChange: () => true } }); + await comp.ngOnChanges({ + feedbackSuggestions: { + previousValue: [], + currentValue: [new Feedback()], + firstChange: true, + isFirstChange: () => true, + }, + }); expect(updateLineWidgetHeightSpy).toHaveBeenCalled(); }); @@ -392,10 +444,60 @@ describe('CodeEditorAceComponent', () => { await comp.initEditor(); const addLineWidgetWithFeedbackSpy = jest.spyOn(comp, 'addLineWidgetWithFeedback'); - comp.feedbackSuggestions = [{ text: 'FeedbackSuggestion:', detailText: 'test', reference: 'file:src/Test.java_line:16', type: FeedbackType.MANUAL }]; + comp.feedbackSuggestions = [ + { + text: 'FeedbackSuggestion:', + detailText: 'test', + reference: 'file:src/Test.java_line:16', + type: FeedbackType.MANUAL, + }, + ]; comp.selectedFile = 'src/Test.java'; await comp.updateLineWidgets(); expect(addLineWidgetWithFeedbackSpy).toHaveBeenCalled(); }); + + describe('feedback deletion', () => { + let f1; + let f2; + beforeEach(() => { + f1 = new Feedback(); + f1.text = 'File src/abc/BubbleSort.java at line 6'; + f1.detailText = 'a'; + f1.reference = 'file:src/abc/BubbleSort.java_line:5'; + + f2 = new Feedback(); + f2.text = 'File src/abc/BubbleSort.java at line 7'; + f2.detailText = 'b'; + f2.reference = 'file:src/abc/BubbleSort.java_line:6'; + }); + + it('should delete correct inline feedback before saving for the first time', async () => { + await comp.initEditor(); + await comp.updateFeedback(f1); + await comp.updateFeedback(f2); + + comp.deleteFeedback(f1); + + expect(comp.feedbacks).not.toContain(f1); + expect(comp.feedbacks).toContain(f2); + }); + + it('should delete correct inline feedback after saving', async () => { + await comp.initEditor(); + await comp.updateFeedback(f1); + await comp.updateFeedback(f2); + + // saving is simulated here by giving the feedbacks an id, which is the only remaining untested factor for + // feedback deletion + f1.id = 1; + f2.id = 2; + + comp.deleteFeedback(f1); + + expect(comp.feedbacks).not.toContain(f1); + expect(comp.feedbacks).toContain(f2); + }); + }); }); From e10376919c2e55daac6553e5f70c0b51f9853ac2 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Sun, 3 Dec 2023 19:57:41 +0100 Subject: [PATCH 4/8] maybe fix tests --- .../component/code-editor/code-editor-ace.component.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts index 2e8e941ed662..0d6c2ae4313a 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-ace.component.spec.ts @@ -459,8 +459,8 @@ describe('CodeEditorAceComponent', () => { }); describe('feedback deletion', () => { - let f1; - let f2; + let f1: Feedback; + let f2: Feedback; beforeEach(() => { f1 = new Feedback(); f1.text = 'File src/abc/BubbleSort.java at line 6'; From 43238e3c19c6e129ce649dcf3892914c3f5fa9eb Mon Sep 17 00:00:00 2001 From: Patrick Bassner Date: Mon, 4 Dec 2023 18:43:55 +0100 Subject: [PATCH 5/8] add gpt code reviewer --- .github/workflows/crgpt.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .github/workflows/crgpt.yml diff --git a/.github/workflows/crgpt.yml b/.github/workflows/crgpt.yml new file mode 100644 index 000000000000..a08f2aedc941 --- /dev/null +++ b/.github/workflows/crgpt.yml @@ -0,0 +1,21 @@ +name: Code Review GPT + +on: + pull_request: + types: [labeled, synchronize] + +jobs: + run_code_review: + if: contains(github.event.pull_request.labels.*.name, 'ready for review') && !github.event.pull_request.draft + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + + - name: Code Review GPT + uses: mattzcarey/code-review-gpt@v0.1.5 + with: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + MODEL: 'gpt-3.5-turbo' + GITHUB_TOKEN: ${{ github.token }} \ No newline at end of file From 3caa2a791dd9511ed3b8959b35ea7d20eed12a70 Mon Sep 17 00:00:00 2001 From: Patrick Bassner Date: Mon, 4 Dec 2023 18:55:28 +0100 Subject: [PATCH 6/8] fix? --- .github/workflows/crgpt.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/crgpt.yml b/.github/workflows/crgpt.yml index a08f2aedc941..af83d5b3d7c0 100644 --- a/.github/workflows/crgpt.yml +++ b/.github/workflows/crgpt.yml @@ -2,20 +2,25 @@ name: Code Review GPT on: pull_request: - types: [labeled, synchronize] + types: [opened, synchronize] + pull_request_target: + types: [ready_for_review] jobs: run_code_review: - if: contains(github.event.pull_request.labels.*.name, 'ready for review') && !github.event.pull_request.draft + if: > + (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'ready for review') && !github.event.pull_request.draft) || + github.event_name == 'pull_request_target' runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 with: fetch-depth: 0 + ref: ${{ github.head_ref || github.event.pull_request.head.ref }} - name: Code Review GPT uses: mattzcarey/code-review-gpt@v0.1.5 with: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} MODEL: 'gpt-3.5-turbo' - GITHUB_TOKEN: ${{ github.token }} \ No newline at end of file + GITHUB_TOKEN: ${{ github.token }} From ed909601a020e4ac8273cdad84066cc8c8e30deb Mon Sep 17 00:00:00 2001 From: Patrick Bassner Date: Mon, 4 Dec 2023 20:14:24 +0100 Subject: [PATCH 7/8] hmm --- .github/workflows/crgpt.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/crgpt.yml b/.github/workflows/crgpt.yml index af83d5b3d7c0..c696189c6709 100644 --- a/.github/workflows/crgpt.yml +++ b/.github/workflows/crgpt.yml @@ -16,7 +16,6 @@ jobs: - uses: actions/checkout@v3 with: fetch-depth: 0 - ref: ${{ github.head_ref || github.event.pull_request.head.ref }} - name: Code Review GPT uses: mattzcarey/code-review-gpt@v0.1.5 From c8566dbda717aa16468f40270ef9a3a7d6f657ab Mon Sep 17 00:00:00 2001 From: Patrick Bassner Date: Sat, 9 Dec 2023 12:44:18 +0100 Subject: [PATCH 8/8] remove crgpt.yml --- .github/workflows/crgpt.yml | 25 ------------------------- 1 file changed, 25 deletions(-) delete mode 100644 .github/workflows/crgpt.yml diff --git a/.github/workflows/crgpt.yml b/.github/workflows/crgpt.yml deleted file mode 100644 index c696189c6709..000000000000 --- a/.github/workflows/crgpt.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: Code Review GPT - -on: - pull_request: - types: [opened, synchronize] - pull_request_target: - types: [ready_for_review] - -jobs: - run_code_review: - if: > - (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'ready for review') && !github.event.pull_request.draft) || - github.event_name == 'pull_request_target' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - with: - fetch-depth: 0 - - - name: Code Review GPT - uses: mattzcarey/code-review-gpt@v0.1.5 - with: - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} - MODEL: 'gpt-3.5-turbo' - GITHUB_TOKEN: ${{ github.token }}