Communication: Highlight solution answer in thread #9998
Conversation
github-actions
bot
added
tests
client
WalkthroughThe changes in this pull request introduce a new method and a conditional rendering in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (1)
32-35: Consider simplifying the boolean check and improving the comment.The implementation can be improved in terms of clarity and documentation:
- //Check if the post is a resolves post and if the attribute is not set return false - isResolvesPostSet(): boolean { - return this.posting.resolvesPost !== undefined && this.posting.resolvesPost; - } + /** + * Checks if the current post is marked as a solution. + * @returns true if this post resolves the original post, false otherwise + */ + isResolvesPostSet(): boolean { + return Boolean(this.posting.resolvesPost); + }The
Boolean()conversion handles both undefined check and truthiness in a cleaner way.src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
37-45: Enhance accessibility for the solution badge.While the implementation is functionally correct, we should improve accessibility for screen readers:
@if (isResolvesPostSet()) { <span class="post-header-solution"> <span class="badge bg-info ms-2" jhiTranslate="artemisApp.metis.post.resolvesPost" ngbTooltip="{{ 'artemisApp.metis.post.resolvesPost' | artemisTranslate }}" + role="status" + aria-label="{{ 'artemisApp.metis.post.resolvesPost' | artemisTranslate }}" ></span> </span> }Adding
role="status"andaria-labelimproves the experience for screen reader users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts(1 hunks)src/main/webapp/i18n/de/metis.json(1 hunks)src/main/webapp/i18n/en/metis.json(1 hunks)src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (1)
src/main/webapp/i18n/de/metis.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
📓 Learnings (1)
src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts (1)
Learnt from: rabeatwork
PR: ls1intum/Artemis#9103
File: src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts:94-94
Timestamp: 2024-11-12T12:52:03.805Z
Learning: The `#today-flag` ID is only present in the test files and not in the actual component's HTML or TypeScript files.
🔇 Additional comments (2)
src/main/webapp/i18n/en/metis.json (1)
138-138: LGTM! Translation key properly integrated
The new translation key "resolvesPost" with value "Solution" is well-placed in the post section and aligns perfectly with the PR objective of highlighting solution answers in threads.
src/main/webapp/i18n/de/metis.json (1)
138-138: LGTM! German translation properly implemented
The German translation key "resolvesPost" with value "Lösung" is:
- Correctly placed in the post section
- Uses appropriate German translation
- Maintains consistency with the informal language style (dutzen) used throughout the file
| it('should display solution when answerPost resolves post', () => { | ||
| const isResolvesPostSetSpy = jest.spyOn(component, 'isResolvesPostSet'); | ||
| component.posting = metisResolvingAnswerPostUser1; | ||
| fixture.detectChanges(); | ||
| const solution = debugElement.query(By.css('.post-header-solution')); | ||
| expect(isResolvesPostSetSpy).toHaveReturnedWith(true); | ||
| expect(solution).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for the solution badge feature.
While the current test verifies the positive case, we should add more test cases:
it('should display solution when answerPost resolves post', () => {
const isResolvesPostSetSpy = jest.spyOn(component, 'isResolvesPostSet');
component.posting = metisResolvingAnswerPostUser1;
fixture.detectChanges();
const solution = debugElement.query(By.css('.post-header-solution'));
expect(isResolvesPostSetSpy).toHaveReturnedWith(true);
expect(solution).not.toBeNull();
+ const badge = solution.query(By.css('.badge'));
+ expect(badge.nativeElement.getAttribute('aria-label')).toBeTruthy();
});
+
+ it('should not display solution badge when post does not resolve', () => {
+ component.posting = { ...metisResolvingAnswerPostUser1, resolvesPost: false };
+ fixture.detectChanges();
+ const solution = debugElement.query(By.css('.post-header-solution'));
+ expect(solution).toBeNull();
+ });
+
+ it('should display correct translation in solution badge', () => {
+ component.posting = metisResolvingAnswerPostUser1;
+ fixture.detectChanges();
+ const badge = debugElement.query(By.css('.badge'));
+ expect(badge.attributes['jhiTranslate']).toBe('artemisApp.metis.post.resolvesPost');
+ });These additional tests will:
- Verify the accessibility attributes
- Test the negative case
- Ensure correct translation key usage
Committable suggestion skipped: line range outside the PR's diff.
Hey @HawKhiem, |
Sorry for pressing the wrong button and acidentally closing this pr a minute ago. I reopen it. Anyway when I tried to deploy to a test server, I got something like this. Starting from the third check onwards somehow didn't run through
|
@HawKhiem I see, however all the errors seem to be CI related. It also does not make any sense that the server would not build as I didn't change anything in it. 
|
That is indeed quite odd. I reran again and the build still doesnt go through. I will try deploying and test locally later today
|
|
I have also encountered this problem. For the majority (maybe even all) of my PRs, the deployment on the test server does not work. For most of them the |
Yes I also have that option |
Malekos74
added
deploy:artemis-test3
and removed
deploy:artemis-test1
deploy:artemis-test3
labels
Malekos74
added
deploy:artemis-test3
deploy:artemis-test5
and removed
deploy:artemis-test3
labels
Just a quick update for others who may have the same problem of PRs not being deployed. If you try to create a PR with a branch from a forked Artemis repository, the test server will not deploy it. |
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Implements #9934
Description
Add a solution badge to the resolving message of a thread.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests