Communication: Fix reply button message editing issue in exercise view
#9815
Conversation
asliayk
added
tests
client
WalkthroughThe changes in this pull request primarily involve modifications to how the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.tsOops! Something went wrong! :( ESLint: 9.14.0 TypeError: Error while loading rule '@typescript-eslint/no-unused-expressions': Cannot read properties of undefined (reading 'allowShortCircuit') 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: 2
🧹 Outside diff range and nitpick comments (15)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.scss (1)
16-16: LGTM! Consider grouping related flexbox properties.The addition of
display: fleximproves the layout consistency of reaction buttons. However, for better maintainability, consider grouping related flexbox properties together..reaction-button { background: var(--reaction-button-default-color); color: var(--metis-gray); border: none; border-radius: 1rem; + display: flex; align-items: center; height: 1.2rem; - display: flex;src/main/webapp/app/shared/metis/posting.directive.ts (2)
12-12: Consider adding runtime validation for moderation rightsWhile the transition to the new
input()function is good, consider adding runtime validation to ensure the permission flag cannot be manipulated:- hasChannelModerationRights = input<boolean>(false); + hasChannelModerationRights = input<boolean>(() => { + const value = false; + // Ensure the value is strictly boolean + return typeof value === 'boolean' ? value : false; + });
Line range hint
82-90: Add permission checks to edit and delete methodsGiven that this PR focuses on fixing edit/delete permissions, these methods should verify the user has appropriate rights before proceeding. Currently, a malicious user could bypass UI controls and invoke these methods directly.
editPosting() { + if (!this.hasChannelModerationRights()) { + console.warn('Unauthorized attempt to edit posting'); + return; + } this.reactionsBar.editPosting(); this.showDropdown = false; } deletePost() { + if (!this.hasChannelModerationRights()) { + console.warn('Unauthorized attempt to delete posting'); + return; + } this.reactionsBar.deletePosting(); this.showDropdown = false; }src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
74-76: Consider refactoring the permission check for better maintainability.While the permission logic is correct and aligns with the PR objectives, consider extracting the complex permission check into a dedicated method for better readability and maintainability.
+ private canEditOtherUsersAnswer(): boolean { + const isCourseWideChannel = getAsChannelDTO(this.posting.post?.conversation)?.isCourseWide ?? false; + const isAtLeastInstructorInCourse = this.metisService.metisUserIsAtLeastInstructorInCourse(); + return (isCourseWideChannel && isAtLeastInstructorInCourse) || (this.hasChannelModerationRights() ?? false); + } + setMayEditOrDelete(): void { this.isAuthorOfOriginalPost = this.metisService.metisUserIsAuthorOfPosting(this.posting.post!); this.isAnswerOfAnnouncement = getAsChannelDTO(this.posting.post?.conversation)?.isAnnouncementChannel ?? false; - const isCourseWideChannel = getAsChannelDTO(this.posting.post?.conversation)?.isCourseWide ?? false; - const isAtLeastInstructorInCourse = this.metisService.metisUserIsAtLeastInstructorInCourse(); - const mayEditOrDeleteOtherUsersAnswer = (isCourseWideChannel && isAtLeastInstructorInCourse) || (this.hasChannelModerationRights() ?? false); + const mayEditOrDeleteOtherUsersAnswer = this.canEditOtherUsersAnswer(); this.mayEditOrDelete = !this.isReadOnlyMode && (this.isAuthorOfPosting || mayEditOrDeleteOtherUsersAnswer); this.mayEditOrDeleteOutput.emit(this.mayEditOrDelete); }src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
Line range hint
1-142: Consider implementing OnDestroy to prevent memory leaks.The component maintains a static reference
activeDropdownPostand uses document listeners, but doesn't clean them up properly.Consider implementing the following changes:
- export class AnswerPostComponent extends PostingDirective<AnswerPost> { + export class AnswerPostComponent extends PostingDirective<AnswerPost> implements OnDestroy { // ... existing code ... + ngOnDestroy() { + // Clean up static reference if this was the active dropdown + if (AnswerPostComponent.activeDropdownPost === this) { + AnswerPostComponent.activeDropdownPost = null; + } + this.enableBodyScroll(); + }
Line range hint
89-120: Consider extracting dropdown positioning logic to a service.The dropdown positioning logic in
onRightClickandadjustDropdownPositioncould be reused across other components. Consider extracting it to a shared service for better code reuse as per the coding guidelines.Example structure:
@Injectable({ providedIn: 'root' }) export class DropdownPositionService { adjustPosition(event: MouseEvent, dropdownWidth: number): { x: number; y: number } { const x = event.clientX; const y = event.clientY; const screenWidth = window.innerWidth; return { x: x + dropdownWidth > screenWidth ? screenWidth - dropdownWidth - 10 : x, y }; } }src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (1)
Line range hint
97-122: Consider adding error handling for permission denied scenarios.While the permission check through
mayEditOrDeleteis correct, consider adding user feedback for permission denied scenarios. This would improve user experience by clearly communicating why certain actions are not available.<button class="reaction-button clickable px-2 fs-small edit" [disabled]="readOnlyMode" (click)="editPosting()" + [ngbTooltip]="mayEditOrDelete ? ('artemisApp.metis.editPosting' | artemisTranslate) : ('artemisApp.metis.noPermission' | artemisTranslate)" - [ngbTooltip]="'artemisApp.metis.editPosting' | artemisTranslate" >src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
155-162: Test implementation looks good, consider adding edge cases.The test follows good practices with proper setup, specific assertions, and clear naming. However, consider adding test cases for:
- Error scenarios when modal closure fails
- Component state verification after modal closure
- Multiple successive open/close operations
it('should handle modal close errors gracefully', () => { component.posting = metisPostExerciseUser1; component.ngOnInit(); fixture.detectChanges(); const createAnswerPostModalClose = jest.spyOn(component.createAnswerPostModalComponent, 'close') .mockImplementation(() => { throw new Error('Modal close failed'); }); expect(() => component.closeCreateAnswerPostModal()).not.toThrow(); expect(createAnswerPostModalClose).toHaveBeenCalledOnce(); }); it('should maintain consistent state after modal operations', () => { component.posting = metisPostExerciseUser1; component.ngOnInit(); fixture.detectChanges(); component.openCreateAnswerPostModal(); component.closeCreateAnswerPostModal(); component.closeCreateAnswerPostModal(); // Double close expect(component.createAnswerPostModalComponent.close).toHaveBeenCalledTimes(2); });src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (2)
68-76: Consider adding error handling for event emissionsThe new methods for managing answer view state are well-structured, but could benefit from error handling for edge cases.
Consider wrapping the event emissions in try-catch blocks:
openAnswerView() { + try { this.showAnswersChange.emit(true); this.openPostingCreateEditModal.emit(); + } catch (error) { + console.error('Failed to open answer view:', error); + } } closeAnswerView() { + try { this.showAnswersChange.emit(false); this.closePostingCreateEditModal.emit(); + } catch (error) { + console.error('Failed to close answer view:', error); + } }
203-205: Improve readability of permission logicThe permission check is correct but could be more readable by extracting the complex condition into a well-named variable.
Consider this refactor for improved readability:
- const mayEditOrDeleteOtherUsersAnswer = (isCourseWideChannel && this.isAtLeastInstructorInCourse) || (this.hasChannelModerationRights() ?? false); + const hasInstructorPermissionsInCourseWideChannel = isCourseWideChannel && this.isAtLeastInstructorInCourse; + const hasChannelModeratorPermissions = this.hasChannelModerationRights() ?? false; + const mayEditOrDeleteOtherUsersAnswer = hasInstructorPermissionsInCourseWideChannel || hasChannelModeratorPermissions; this.mayEditOrDelete = !this.readOnlyMode && !this.previewMode && (this.isAuthorOfPosting || mayEditOrDeleteOtherUsersAnswer); this.mayEditOrDeleteOutput.emit(this.mayEditOrDelete);src/main/webapp/app/shared/metis/post/post.component.ts (2)
93-93: LGTM! Note on inheritance design.The change from private to protected enables inheritance-based code reuse, which aligns with the component's architecture and the PR's design consistency objectives.
Consider documenting the inheritance use case in the class-level JSDoc to help future maintainers understand the design decision.
221-226: Add error handling and enhance documentation.While the method implementation is clean and follows the single responsibility principle, it could benefit from defensive programming and better documentation.
Consider these improvements:
/** - * Close create answer modal + * Closes the answer post creation modal by delegating to the PostFooterComponent. + * This method is part of the reply functionality enhancement. + * @throws Error if postFooterComponent is not initialized */ closeCreateAnswerPostModal() { + if (!this.postFooterComponent) { + throw new Error('PostFooterComponent is not initialized'); + } this.postFooterComponent.closeCreateAnswerPostModal(); }src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
173-180: LGTM! Consider improving test readability.The test case correctly verifies the modal closing functionality and follows the established testing patterns. The spy assertion uses the recommended
toHaveBeenCalledOnce()matcher.Consider renaming the spy variable to better reflect its purpose:
- const postFooterOpenCreateAnswerPostModal = jest.spyOn(component.postFooterComponent, 'closeCreateAnswerPostModal'); + const postFooterCloseModalSpy = jest.spyOn(component.postFooterComponent, 'closeCreateAnswerPostModal');src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (2)
220-287: Consider using test.each for permission scenariosWhile the test suite is well-structured and comprehensive, consider using
test.eachto make the permission tests more maintainable and concise. This would reduce code duplication and make it easier to add new permission scenarios.Example refactor:
- it('should allow edit/delete when user is the author and not in read-only or preview mode', () => { - component.isAuthorOfPosting = true; - jest.spyOn(metisService, 'metisUserIsAtLeastInstructorInCourse').mockReturnValue(false); - jest.spyOn(component, 'hasChannelModerationRights').mockReturnValue(false); - - component.setMayEditOrDelete(); - - expect(component.mayEditOrDelete).toBeTrue(); - expect(component.mayEditOrDeleteOutput.emit).toHaveBeenCalledWith(true); - }); - - it('should allow edit/delete when user has channel moderation rights', () => { - // ... similar setup - }); + it.each` + scenario | isAuthor | hasModRights | readOnly | preview | expected + ${'author'} | ${true} | ${false} | ${false} | ${false} | ${true} + ${'moderator'} | ${false} | ${true} | ${false} | ${false} | ${true} + ${'read-only'} | ${true} | ${true} | ${true} | ${false} | ${false} + ${'preview'} | ${true} | ${true} | ${false} | ${true} | ${false} + ${'no permissions'} | ${false} | ${false} | ${false} | ${false} | ${false} + `('should $scenario scenario work correctly', + ({isAuthor, hasModRights, readOnly, preview, expected}) => { + component.isAuthorOfPosting = isAuthor; + component.readOnlyMode = readOnly; + component.previewMode = preview; + jest.spyOn(component, 'hasChannelModerationRights') + .mockReturnValue(hasModRights); + + component.setMayEditOrDelete(); + + expect(component.mayEditOrDelete).toBe(expected); + expect(component.mayEditOrDeleteOutput.emit) + .toHaveBeenCalledWith(expected); + });
375-393: Enhance test descriptions for modal interaction testsWhile the tests correctly verify the modal interactions, consider making the test descriptions more specific to better document the expected behavior.
Example improvement:
- it('should emit showAnswersChange and openPostingCreateEditModal when openAnswerView is called', () => { + it('should show answers and open create/edit modal when initiating a reply', () => {- it('should emit showAnswersChange and closePostingCreateEditModal when closeAnswerView is called', () => { + it('should hide answers and close create/edit modal when canceling a reply', () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(1 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(1 hunks)src/main/webapp/app/shared/metis/post/post.component.html(6 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(3 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html(0 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts(3 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts(5 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.scss(1 hunks)src/main/webapp/app/shared/metis/posting.directive.ts(2 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts(5 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html
- src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (13)
src/main/webapp/app/shared/metis/answer-post/answer-post.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/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/shared/metis/post/post.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/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.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/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.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/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (1)
src/main/webapp/app/shared/metis/posting.directive.ts (1)
src/test/javascript/spec/component/shared/metis/post/post.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}}
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.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}}
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.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}}
🔇 Additional comments (26)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html (1)
Line range hint 1-1: LGTM! Correct usage of new Angular syntax.
The template correctly uses the new @if and @for directives instead of the older *ngIf and *ngFor, following the latest Angular syntax guidelines.
Also applies to: 14-15
src/main/webapp/app/shared/metis/posting.directive.ts (3)
2-2: LGTM: Import changes follow Angular style guide
The addition of the input import from @angular/core is properly organized with other Angular core imports.
Line range hint 33-42: LGTM: Proper cleanup in ngOnDestroy
The implementation follows best practices for memory management by properly cleaning up timers in ngOnDestroy.
Line range hint 7-7: Verify permission checks in derived components
Since this is an abstract class, we should verify that all derived components properly handle the permission checks.
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
1-1: Verify Angular version compatibility for input signals.
The adoption of Angular's input signals API (input()) is a good modernization step. However, we should ensure the project's Angular version supports this feature (introduced in Angular 16).
Also applies to: 30-30
✅ Verification successful
✓ Input signals API usage is compatible with Angular version
The project uses Angular v18.2.12, which fully supports the input signals API introduced in Angular 16. The modernization change to use input() is safe to implement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Angular version in package.json
jq -r '.dependencies["@angular/core"]' package.json
Length of output: 61
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (4)
Line range hint 2-2: LGTM! Follows new Angular syntax.
The code correctly uses the new @if directive instead of *ngIf as per coding guidelines.
Line range hint 48-58: Design consistency maintained for reaction display.
Good separation of concerns - this instance of the reactions bar is correctly configured for displaying emoji counts only, without moderation controls.
Line range hint 69-79: Secure implementation of edit/delete options.
The dropdown menu correctly:
- Uses the new
@ifdirective syntax - Conditionally renders edit/delete options based on permissions
36-36: Permission check implementation looks good.
The change from property binding to method call hasChannelModerationRights() aligns with the PR objective of fixing edit/delete permissions for messages.
Let's verify the permission check implementation:
✅ Verification successful
Permission check implementation is correctly used across the codebase
The change from property binding to method call hasChannelModerationRights() in answer-post.component.html is consistent with the codebase's implementation:
- The permission is defined as an input property in answer-post.component.ts:
hasChannelModerationRights = input<boolean>(false) - It's used consistently across multiple components (post-footer, post-reactions-bar, etc.) for permission checks
- The implementation is properly tested in the test files
- The permission is properly integrated with the channel model (channel.model.ts) and permission utilities (conversation-permissions.utils.ts)
The change aligns with the codebase's permission handling patterns and supports the PR's goal of fixing edit/delete permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of hasChannelModerationRights to ensure consistent implementation
rg -t html "hasChannelModerationRights" --no-heading
# Look for the method definition in the component
ast-grep --pattern 'hasChannelModerationRights() {
$$$
}'
Length of output: 1751
Script:
#!/bin/bash
# Look for the method definition in TypeScript files
ast-grep --pattern 'hasChannelModerationRights() {
$$$
}'
# Search for class property definitions
ast-grep --pattern 'hasChannelModerationRights: boolean'
# Search for the method implementation in component files
rg -t typescript "hasChannelModerationRights" -A 3
Length of output: 330
Script:
#!/bin/bash
# Search for TypeScript/JavaScript files containing hasChannelModerationRights
rg "hasChannelModerationRights" --type-list
rg "hasChannelModerationRights" -t js -t ts -A 3
# Find all TypeScript/JavaScript files in the component directories
fd "\.(ts|js)$" src/main/webapp/app/shared/metis/answer-post/
Length of output: 32556
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
57-57: LGTM! Modern Angular input syntax used correctly.
The new input property hasChannelModerationRights is properly implemented using the modern Angular input() syntax with a default value of false, which is a good practice for boolean flags.
Line range hint 82-87: Verify the impact of body scroll manipulation.
The disableBodyScroll method manipulates the scroll behavior of .thread-answer-post. Ensure this doesn't affect other parts of the application when multiple answer posts are present.
✅ Verification successful
Let me analyze the scroll manipulation in the codebase further to verify its impact.
Let me try a different approach to search for scroll-related code.
Based on the analysis of the codebase, I can now provide a final response:
The scroll manipulation in answer-post.component.ts is safely scoped and won't affect other components.
The scroll manipulation is properly contained within the .thread-answer-post class, which is only used in the conversation thread sidebar component. The implementation follows the same pattern as other components in the codebase that manage scroll behavior:
- The class
.thread-answer-postis uniquely used inconversation-thread-sidebar.component.html - The scroll manipulation methods
disableBodyScroll()andenableBodyScroll()only affect elements with this specific class - The scroll state is properly restored when the dropdown is closed
- Unit tests verify the isolated behavior of these scroll manipulations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by scroll manipulation
rg -l "thread-answer-post" --type ts --type html
# Check for other scroll manipulation in the codebase
rg "overflow|scroll" --type ts --type html
Length of output: 90538
Script:
#!/bin/bash
# Search for other components that might be using the thread-answer-post class
ast-grep --pattern 'class="$$$thread-answer-post$$$"'
# Search for other components that might be manipulating scroll behavior
ast-grep --pattern 'disableBodyScroll() {
$$$
}'
# Search for other components that might be enabling scroll behavior
ast-grep --pattern 'enableBodyScroll() {
$$$
}'
Length of output: 284
Script:
#!/bin/bash
# Search for thread-answer-post class usage in HTML files
rg -l 'class="[^"]*thread-answer-post[^"]*"' --type html
# Search for scroll manipulation methods in TypeScript files
rg -A 5 'disableBodyScroll|enableBodyScroll' --type ts
# Search for components that might be affected by scroll manipulation
rg -A 5 'overflow.*hidden|overflow.*auto' --type ts --type html
Length of output: 42344
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (2)
14-14: LGTM: Import of signal-based input.
The addition of the input import from '@angular/core' aligns with Angular's modern practices for reactive input properties.
51-51: Verify template updates and permission checks.
The migration to signal-based input for hasChannelModerationRights looks good. However, since this property is crucial for edit/delete permissions (as per PR objectives), we should verify:
- All template bindings are updated to use the new syntax
- Permission checks are working as expected
✅ Verification successful
Let me verify the template updates by checking the actual template content.
Template updates and permission checks are correctly implemented
The verification shows that:
- All templates have been updated to use the signal-based input syntax with
hasChannelModerationRights()function call - Permission checks are consistently implemented across components:
- post-footer.component.html uses
[hasChannelModerationRights]="hasChannelModerationRights()" - posting-thread.component.html uses
[hasChannelModerationRights]="hasChannelModerationRights" - answer-post.component.html uses
[hasChannelModerationRights]="hasChannelModerationRights()"
- post-footer.component.html uses
The migration to signal-based input is complete and consistent with proper permission checks in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template updates and permission checks
# Check if all templates have been updated to use the new syntax
echo "Checking templates for old @Input syntax..."
rg -l "hasChannelModerationRights" --type html
# Check if permission checks are consistently implemented
echo "Checking permission implementations..."
rg -A 5 "hasChannelModerationRights" --type ts
Length of output: 46249
Script:
#!/bin/bash
# Check the actual content of post-footer.component.html to verify template bindings
echo "Checking post-footer.component.html..."
cat src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html
# Check other templates that use hasChannelModerationRights
echo -e "\nChecking other key templates..."
cat src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.html
cat src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
Length of output: 7235
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (4)
Line range hint 24-33: LGTM: Reply button implementation aligns with requirements.
The conditional rendering of the reply button (only showing when hovering and there are no answers) aligns with the PR objectives of improving design consistency. The reply option is now correctly placed in the reaction bar.
Line range hint 52-95: LGTM: Follows modern Angular syntax guidelines.
The implementation correctly uses the new Angular control flow syntax (@if and @for) as specified in the coding guidelines.
Line range hint 123-135: LGTM: Pin functionality implementation is robust.
The pin functionality implementation includes proper permission checks, clear visual feedback, and accessibility features through tooltips.
7-7: Verify architectural change from events to direct method calls.
The change from event emission (showAnswers.emit()) to direct method calls (closeAnswerView(), openAnswerView()) might affect component reusability and testing. While this works, it creates tighter coupling between the template and component.
Also applies to: 15-16
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
155-162: Verify component implementation matches test assumptions.
Let's verify that the closeCreateAnswerPostModal method exists in the component and that it's properly implemented.
✅ Verification successful
Based on the results, I can generate the final response as we have found the necessary information. The search results show that:
- The
closeCreateAnswerPostModalmethod exists inpost-footer.component.tsand callsthis.createAnswerPostModalComponent.close() - The
createAnswerPostModalComponentis properly declared as a ViewChild in the component - The implementation matches exactly what the test is verifying
The component implementation correctly matches the test assumptions
The test's expectations align perfectly with the actual implementation in src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts, where:
- The component has the required ViewChild decorator for
createAnswerPostModalComponent - The
closeCreateAnswerPostModalmethod is implemented and callsclose()on the modal component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of closeCreateAnswerPostModal in PostFooterComponent
# Search for the method implementation
echo "Searching for closeCreateAnswerPostModal implementation..."
ast-grep --pattern 'closeCreateAnswerPostModal() {
$$$
}'
# Search for related modal component references
echo "Searching for createAnswerPostModalComponent references..."
rg "createAnswerPostModalComponent" --type ts
Length of output: 1416
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (2)
1-1: LGTM: New event emitter for modal management
The addition of closePostingCreateEditModal event emitter aligns with the PR objectives for improving modal management and follows Angular's event handling patterns.
Also applies to: 44-44
55-55: Verify the impact of Signal-based input migration
The migration to Angular's Signal-based input API for hasChannelModerationRights is good, but we should verify its usage across templates.
✅ Verification successful
Migration to Signal-based input is correctly implemented and used
The verification shows that the hasChannelModerationRights input property is correctly migrated to Signal-based input and its usage is consistent across the codebase:
- The property is correctly defined using
input<boolean>(false)in the component - It's properly consumed in templates using both property binding
[hasChannelModerationRights]="..."and method invocationhasChannelModerationRights() - The property is used correctly in permission checks like
setMayEditOrDelete()andsetCanPin() - Test files have been updated to handle the Signal-based input properly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of hasChannelModerationRights in templates
# Look for both property binding and method invocation patterns
echo "Checking template usage patterns..."
rg -t html "hasChannelModerationRights" --context 2
echo "Checking component references..."
rg -t typescript "hasChannelModerationRights" --context 2
Length of output: 7022
Script:
#!/bin/bash
# Let's check TypeScript files specifically using ripgrep with the correct file extension pattern
echo "Checking TypeScript component references..."
rg -g "*.ts" "hasChannelModerationRights" --context 2
echo "Checking the specific component implementation..."
cat "src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts"
Length of output: 48493
src/main/webapp/app/shared/metis/post/post.component.html (4)
138-138: LGTM! Consistent permission check implementation.
The change maintains consistency with the reactions bar component in how moderation rights are evaluated.
Line range hint 70-86: LGTM! Verify permission checks across different user roles.
The change to invoke hasChannelModerationRights as a method allows for dynamic evaluation of moderation rights, which aligns with fixing the permission issues.
Let's verify the permission checks:
#!/bin/bash
# Search for all usages of hasChannelModerationRights to ensure consistent implementation
rg -t ts "hasChannelModerationRights" -A 5163-166: Verify component interaction for reply functionality.
The conditional logic for reply actions looks good, but let's verify:
- The
reactionsBarComponentreference is properly initialized - The
openAnswerViewmethod exists and is accessible
Let's check the component implementation:
#!/bin/bash
# Search for reactionsBarComponent definition and openAnswerView method
ast-grep --pattern 'class $_ {
$$$
reactionsBarComponent$_
$$$
}'
ast-grep --pattern 'openAnswerView() {
$$$
}'Line range hint 102-114: Verify modal state management.
The addition of closePostingCreateEditModal event improves the modal management. Ensure that the modal state is properly cleaned up when closed.
Let's check the modal handling implementation:
src/main/webapp/app/shared/metis/post/post.component.ts (1)
93-93: Verify permission checks for edit/delete operations.
Given that this PR addresses permission-related fixes, let's verify the implementation of permission checks throughout the codebase.
Also applies to: 221-226
✅ Verification successful
Let me analyze the results from the shell scripts. The search revealed comprehensive permission checks implementation across the codebase:
- The
PostReactionsBarComponentandAnswerPostReactionsBarComponentboth implement permission checks throughmayEditOrDeleteproperty andsetMayEditOrDelete()method - The permission checks consider multiple factors:
- User being the author (
isAuthorOfPosting) - Channel moderation rights (
hasChannelModerationRights) - Course-wide instructor status (
isAtLeastInstructorInCourse) - Read-only and preview modes
- User being the author (
- Extensive test coverage exists verifying different permission scenarios
- The permission system is consistently used across components with proper event emitters
Let me generate the final response:
Permission checks for edit/delete operations are properly implemented
The codebase shows a robust permission system that correctly handles edit/delete operations based on:
- Post authorship
- Channel moderation rights
- Course-wide instructor privileges
- Read-only and preview mode states
The implementation is well-tested with comprehensive test cases covering all permission scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify permission checks implementation
# Look for permission check usage and related components
# Check for permission-related methods and properties
echo "Searching for permission check implementations..."
rg -A 5 "mayEditOrDelete|hasChannelModerationRights|onMayEditOrDelete" --type ts
# Check for permission usage in templates
echo "Searching for permission checks in templates..."
rg -A 5 "\[mayEditOrDelete\]|\*ngIf.*mayEditOrDelete" --type html
# Look for test coverage of permission checks
echo "Searching for permission-related tests..."
rg -A 5 "describe.*permission|it.*permission|mayEditOrDelete" --type spec
Length of output: 55368
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (1)
156-156: LGTM: Proper mocking of channel moderation rights
The mock setup correctly tests the new permission check, aligning with the PR's objective of fixing edit/delete permissions.
| @@ -25,7 +25,7 @@ | |||
| (openPostingCreateEditModal)="createAnswerPostModal.open()" | |||
| (userReferenceClicked)="userReferenceClicked.emit($event)" | |||
| (channelReferenceClicked)="channelReferenceClicked.emit($event)" | |||
| [hasChannelModerationRights]="hasChannelModerationRights" | |||
| [hasChannelModerationRights]="hasChannelModerationRights()" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent permission check implementations found
The following files still use property binding instead of method invocation for hasChannelModerationRights:
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.htmlsrc/main/webapp/app/overview/course-conversations/layout/conversation-thread-sidebar/conversation-thread-sidebar.component.html
These components should be updated to use method invocation hasChannelModerationRights() for consistent permission evaluation across the application.
🔗 Analysis chain
Verify permission check consistency across components.
The change from property binding to method invocation for hasChannelModerationRights aligns with the PR objective of fixing edit/delete permissions. This ensures that permissions are always freshly evaluated.
Let's verify that this change is consistent across all components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of hasChannelModerationRights across components
# Expected: All components should use the method invocation style
# Search for any remaining direct property bindings
rg -t html "hasChannelModerationRights\]=\"hasChannelModerationRights\"" src/main/webapp/
# Search for the new method invocation style
rg -t html "hasChannelModerationRights\]=\"hasChannelModerationRights\(\)\"" src/main/webapp/
Length of output: 1144
| /** | ||
| * Close create answer modal | ||
| */ | ||
| closeCreateAnswerPostModal() { | ||
| this.createAnswerPostModalComponent.close(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for modal component.
While the method implementation is straightforward, it should handle cases where createAnswerPostModalComponent might be undefined.
Consider this safer implementation:
closeCreateAnswerPostModal() {
- this.createAnswerPostModalComponent.close();
+ if (this.createAnswerPostModalComponent) {
+ this.createAnswerPostModalComponent.close();
+ } else {
+ console.warn('Modal component is not initialized');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Close create answer modal | |
| */ | |
| closeCreateAnswerPostModal() { | |
| this.createAnswerPostModalComponent.close(); | |
| } | |
| /** | |
| * Close create answer modal | |
| */ | |
| closeCreateAnswerPostModal() { | |
| if (this.createAnswerPostModalComponent) { | |
| this.createAnswerPostModalComponent.close(); | |
| } else { | |
| console.warn('Modal component is not initialized'); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/shared/metis/post/post.component.scss (1)
23-23: Consider improving the hover menu's height handlingThe fixed
max-height: 2.2remmight cause issues:
- Content could get clipped if there are multiple action items
- It may not accommodate different font sizes or zoom levels
Consider this alternative approach:
.hover-actions { - max-height: 2.2rem; + max-height: fit-content; + min-height: 2.2rem; + overflow: visible; }Also, since this component contains interactive elements that appear/disappear, consider adding appropriate ARIA attributes and keyboard navigation support for better accessibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/shared/metis/post/post.component.scss(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.scss
🔇 Additional comments (1)
src/main/webapp/app/shared/metis/post/post.component.scss (1)
Line range hint 1-85: Well-structured stylesheet with good practices
The stylesheet demonstrates good practices:
- Consistent use of CSS variables for theming
- Clean organization of styles
- Proper transitions for smooth UX
- Good separation of concerns
asliayk
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (3)
15-15: Maintain consistent button styling.The collapse and expand buttons have inconsistent styling:
- Line 15: Uses
jhi-emojifor the close icon- Line 24: Uses
fa-iconfor the expand iconConsider using the same icon system for better maintainability and visual consistency.
- <button class="reaction-button clickable collapse-answers-btn" [class.reaction-button--reacted]="showAnswers" (click)="closeAnswerView()"> - <jhi-emoji [emoji]="closeCrossId" /> + <button class="reaction-button clickable collapse-answers-btn" [class.reaction-button--reacted]="showAnswers" (click)="closeAnswerView()"> + <fa-icon class="fa-xs align-self-center" [icon]="faTimes" />Also applies to: 23-24
Line range hint
48-93: Consider adding ARIA labels for better accessibility.While the reaction buttons implementation is functionally correct, consider adding ARIA labels to improve accessibility for screen readers.
<button [disabled]="readOnlyMode" class="reaction-button clickable" [class.reaction-button--reacted]="reactionMetaData.value.hasReacted" (click)="updateReaction(reactionMetaData.key)" [ngbTooltip]="reactionMetaData.value.reactingUsers | reactingUsersOnPosting | async" + [attr.aria-label]="'React with ' + reactionMetaData.key" + [attr.aria-pressed]="reactionMetaData.value.hasReacted" >
Line range hint
93-127: Enhance keyboard navigation for emoji selector.Consider adding keyboard navigation support for the emoji selector to improve accessibility and user experience.
<button class="reaction-button clickable px-2 fs-small" (click)="showReactionSelector = !showReactionSelector" + (keydown.escape)="showReactionSelector = false" + role="button" + aria-haspopup="true" + [attr.aria-expanded]="showReactionSelector" cdkOverlayOrigin #trigger="cdkOverlayOrigin" [title]="'artemisApp.metis.chooseReaction' | artemisTranslate" >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html(2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (4)
1-9: LGTM! Correctly implements new Angular syntax.
The reply button implementation correctly uses the new @if syntax and properly handles different behaviors for communication and non-communication pages.
Line range hint 32-48: LGTM! Thread expansion implementation looks good.
The code correctly implements the thread expansion functionality for the communication page, following the new Angular syntax guidelines.
Line range hint 127-135: Verify edit/delete permissions implementation.
The code uses mayEditOrDelete flag to control edit/delete permissions, which aligns with the PR objectives. Let's verify the implementation across the codebase.
✅ Verification successful
Based on the test cases and implementation found in the codebase, I can now provide a conclusive response about the edit/delete permissions implementation.
Edit/delete permissions implementation is correctly implemented.
The implementation properly handles edit/delete permissions through the mayEditOrDelete flag with the following conditions:
- Allows edit/delete when user is the author of the posting and not in read-only or preview mode
- Allows edit/delete when user has channel moderation rights
- Allows edit/delete for instructors in course-wide channels
- Correctly blocks edit/delete in read-only mode or preview mode
- Properly emits permission changes through
mayEditOrDeleteOutput
The test suite comprehensively verifies all these scenarios, giving confidence in the implementation's correctness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mayEditOrDelete usage and its implementation
rg -A 5 "mayEditOrDelete" --type ts
Length of output: 202
Script:
#!/bin/bash
# Let's search for the component file to understand the permission implementation
fd "post-reactions-bar.component.ts" --type f
# After finding the file, let's examine its content
rg -A 10 "canPin|mayEditOrDelete" --type ts
# Let's also check for any permission-related interfaces or types
rg -A 5 "interface.*Permission|type.*Permission" --type ts
Length of output: 30421
136-139: Verify auto-hide functionality for new message indicator.
The hideAfter5Seconds class suggests an auto-hide functionality. Let's verify its implementation.
✅ Verification successful
Auto-hide functionality is correctly implemented
The auto-hide functionality for the new message indicator is properly implemented:
-
The CSS class
hideAfter5Secondsis defined insrc/main/webapp/app/shared/metis/metis.component.scsswith:- A 5-second animation using
cssAnimation - Proper opacity transitions (1 → 0)
- Both webkit and standard animation properties for cross-browser support
- A 5-second animation using
-
The class is correctly applied to the new message indicator elements in multiple components:
- Post header component
- Answer post header component
- Post reactions bar component
The implementation will make the new message indicator automatically fade out after 5 seconds, providing a good user experience without requiring manual dismissal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hideAfter5Seconds implementation
rg "hideAfter5Seconds" --type css
Length of output: 202
Script:
#!/bin/bash
# Search for hideAfter5Seconds implementation with context
rg -A 5 "hideAfter5Seconds" --type scss
Length of output: 68
Script:
#!/bin/bash
# Search for any JavaScript/TypeScript implementation related to hiding functionality
ast-grep --pattern 'class $_ {
$$$
hideAfter5Seconds($$$) {
$$$
}
$$$
}'
Length of output: 210
Script:
#!/bin/bash
# Search for any timer-related code in the component
rg "setTimeout.*hideAfter" -A 5
Length of output: 33
Script:
#!/bin/bash
# Search for hideAfter5Seconds in all files
rg "hideAfter5Seconds" -A 5
# Search for CSS files with correct extension
fd -e css -e scss | xargs cat
Length of output: 435936
Script:
#!/bin/bash
# Search for hideAfter5Seconds animation implementation
rg "hideAfter5Seconds|cssAnimation" -A 5 --type scss --type css
# Search for any JavaScript implementation related to hiding functionality
rg "hideAfter5Seconds" -A 5 --type ts --type js
Length of output: 142
Script:
#!/bin/bash
# Search for hideAfter5Seconds animation implementation in all files
rg "hideAfter5Seconds|cssAnimation" -A 5
# Search for any animation-related CSS/SCSS files
fd -e css -e scss | xargs grep -l "animation"
Length of output: 4307
asliayk
changed the title
Communication: Fix edit/delete post permissions and reply functionality in exercise communication tabCommunication: Fix reply button message editing issue in exercise view
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (1)
338-356: LGTM: Comprehensive tests for answer view interactionsThe new test cases thoroughly verify the behavior of
openAnswerViewandcloseAnswerViewmethods, ensuring proper event emissions. This aligns well with the PR objectives of fixing reply functionality issues.Consider grouping these related tests under a describe block for better organization:
+ describe('Answer View Interactions', () => { it('should emit showAnswersChange and openPostingCreateEditModal when openAnswerView is called', () => { // ... existing test code ... }); it('should emit showAnswersChange and closePostingCreateEditModal when closeAnswerView is called', () => { // ... existing test code ... }); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(0 hunks)src/main/webapp/app/shared/metis/post/post.component.html(4 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/app/shared/metis/post/post.component.html
- src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts
- src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (1)
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts (1)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.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}}
🔇 Additional comments (7)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (3)
5-5: LGTM! Clean import statement.
The removal of unused faCog from imports helps maintain a cleaner codebase.
22-23: LGTM! Good use of readonly for icon properties.
Making icon properties readonly is a good practice as it:
- Ensures immutability of icon references
- Prevents accidental modifications
- Follows Angular best practices for constant values
Line range hint 13-20: Consider enhancing permission checks for edit functionality.
Given that this PR focuses on fixing message editing permissions, consider adding explicit permission checks in this component. While the component correctly handles edit mode through isReadOnlyMode, adding a dedicated method to check edit permissions would make the authorization logic more explicit and maintainable.
Let's verify how edit permissions are currently handled:
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts (3)
6-6: LGTM! Clean import statement.
The removal of the unused faCog icon import helps maintain a cleaner codebase.
25-26: LGTM! Improved immutability with readonly modifiers.
Making the icon properties readonly is a good practice as it:
- Prevents accidental modifications
- Makes the code more predictable
- Follows Angular's immutability principles
Line range hint 41-45: Verify authority checks implementation.
Given that this PR addresses message editing permissions, please ensure that setUserProperties() and setUserAuthorityIconAndTooltip() properly implement the required permission checks.
Let's verify the implementation:
✅ Verification successful
Let me generate additional verification scripts to check the implementation of setUserProperties() since it wasn't found in the previous results:
The previous results show proper authority checks in the directive and related components, but we need to verify the specific implementation of setUserProperties() to ensure it's correctly handling user properties in the context of message editing permissions.
Let me generate one final verification to check the MetisService implementation since it's crucial for the authority checks:
This will help us verify the underlying service methods that are used in the component's authority checks before making a final determination.
Authority checks are properly implemented
The verification shows that the component's authority checks are correctly implemented through a comprehensive hierarchy:
setUserProperties()in the directive properly sets up user permissions by:- Checking post authorship via
metisUserIsAuthorOfPosting - Verifying tutor status via
metisUserIsAtLeastTutorInCourse - Setting appropriate authority icons and tooltips based on user roles (USER, INSTRUCTOR, TUTOR)
- Checking post authorship via
- The underlying MetisService provides robust role verification through:
metisUserIsAuthorOfPosting: Validates post ownershipmetisUserIsAtLeastTutorInCourse: Checks tutor privilegesmetisUserIsAtLeastInstructorInCourse: Verifies instructor status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of authority-related methods
# Look for setUserProperties implementation in parent class
ast-grep --pattern 'setUserProperties() {
$$$
}'
# Check for any authority-related conditions
rg -A 5 'hasChannelModerationRights|isAtLeastInstructor|userAuthority' src/main/webapp/app/shared/metis/
Length of output: 26356
Script:
#!/bin/bash
# Look for setUserProperties implementation in the component and its parent classes
ast-grep --pattern 'setUserProperties() {
$$$
}'
# Check for any other method that might be setting user properties
rg -A 5 'setUserProperties|currentUser|posting\.author' src/main/webapp/app/shared/metis/posting-header/
Length of output: 11965
Script:
#!/bin/bash
# Check MetisService implementation for user authority checks
rg -A 5 'metisUserIsAuthorOfPosting|metisUserIsAtLeastTutorInCourse|metisUserIsAtLeastInstructorInCourse' src/main/webapp/app/shared/metis/metis.service.ts
Length of output: 775
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (1)
242-242: LGTM: Added emoji count verification
The addition of isEmojiCount property improves test coverage by verifying the component's behavior when emoji counting is enabled.
|
github-actions
bot
added
the
deployment-error
zagemello
added
deploy:artemis-test2
and removed
deployment-error
zagemello
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
zagemello
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
feature/communication/fix-post-reaction-bar-bugs-on-exercise-view
branch
Checklist
General
Client
Motivation and Context
There are some minor bugs/issues on the communication overview window of the exercise view:
Description
post-headerandanswer-post-header.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
Code Review
Manual Tests
Test Coverage
Screenshots
updated reply button on communication window in exercise view
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests