Communication: Fix visibility of the edit message option for non-authors
#9830
Conversation
asliayk
added
tests
client
WalkthroughThe changes in this pull request involve significant modifications to the handling of user permissions for editing and deleting posts across various components. The output event bindings for permissions have been separated into distinct outputs for editing and deleting, enhancing clarity. Conditional rendering logic has been refined to ensure that UI elements reflect the correct permissions based on user roles. These adjustments are made in the HTML and TypeScript files of components related to posts and reactions, improving the overall state management and maintainability of the code. Changes
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
error Couldn't find any versions for "@eslint/eslintrc" that matches "^3.2.0" 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: 0
🧹 Outside diff range and nitpick comments (15)
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
71-81: Consider refactoring complex permission logicThe delete permission logic handles multiple conditions and could benefit from being broken down into smaller, more focused methods.
Consider refactoring like this:
- setMayDelete(): void { - // determines if the current user is the author of the original post, that the answer belongs to - 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) || (getAsChannelDTO(this.metisService.getCurrentConversation())?.hasChannelModerationRights ?? false); - this.mayDelete = !this.isReadOnlyMode && (this.isAuthorOfPosting || mayEditOrDeleteOtherUsersAnswer); - this.mayDeleteOutput.emit(this.mayDelete); - } + private hasModeratorPermissions(): boolean { + const isCourseWideChannel = getAsChannelDTO(this.posting.post?.conversation)?.isCourseWide ?? false; + const isAtLeastInstructorInCourse = this.metisService.metisUserIsAtLeastInstructorInCourse(); + const hasChannelModRights = getAsChannelDTO(this.metisService.getCurrentConversation())?.hasChannelModerationRights ?? false; + + return (isCourseWideChannel && isAtLeastInstructorInCourse) || hasChannelModRights; + } + + private updatePostMetadata(): void { + this.isAuthorOfOriginalPost = this.metisService.metisUserIsAuthorOfPosting(this.posting.post!); + this.isAnswerOfAnnouncement = getAsChannelDTO(this.posting.post?.conversation)?.isAnnouncementChannel ?? false; + } + + setMayDelete(): void { + this.updatePostMetadata(); + const mayDeleteOtherUsersAnswer = this.hasModeratorPermissions(); + this.mayDelete = !this.isReadOnlyMode && (this.isAuthorOfPosting || mayDeleteOtherUsersAnswer); + this.mayDeleteOutput.emit(this.mayDelete); + }This refactoring:
- Separates concerns into focused methods
- Improves readability and maintainability
- Makes the permission logic easier to test
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (1)
68-73: LGTM! Consider improving template indentation.The implementation correctly uses the new
@ifsyntax and properly separates edit permissions. Consider adjusting the indentation for better readability:@if (mayEdit) { - <button class="dropdown-item d-flex" (click)="editPosting()"> - <fa-icon [icon]="faPencilAlt" class="item-icon"></fa-icon> - <span jhiTranslate="artemisApp.metis.post.editMessage"></span> - </button> + <button class="dropdown-item d-flex" (click)="editPosting()"> + <fa-icon [icon]="faPencilAlt" class="item-icon"></fa-icon> + <span jhiTranslate="artemisApp.metis.post.editMessage"></span> + </button> }src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
55-56: LGTM! Good separation of concerns.The separation of edit and delete permissions into distinct properties improves code clarity and follows the single responsibility principle. The default
falsevalues provide secure defaults.Consider documenting these properties with JSDoc comments to clarify their purpose and relationship to user permissions.
99-104: Consider triggering change detection after property updates.The methods correctly handle permission updates, but since the component uses
ChangeDetectionStrategy.OnPush, property changes might not be immediately reflected in the view.Consider updating the methods to trigger change detection:
onMayDelete(value: boolean) { this.mayDelete = value; + this.changeDetector.markForCheck(); } onMayEdit(value: boolean) { this.mayEdit = value; + this.changeDetector.markForCheck(); }src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html (1)
Line range hint
81-89: Add aria-label for better accessibility.The delete button implementation looks good and properly separates the delete permission. Consider adding an aria-label to improve accessibility.
- <button class="reaction-button clickable px-2 fs-small delete"> + <button class="reaction-button clickable px-2 fs-small delete" + aria-label="{{ 'artemisApp.metis.deleteAnswer' | artemisTranslate }}">src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (1)
Line range hint
122-130: Consider enhancing delete confirmation tooltip.The separation of delete permissions using
mayDeleteis implemented correctly. However, consider making the delete confirmation more explicit by mentioning that this action cannot be undone.- [confirmTooltip]="'artemisApp.metis.confirmDeleteAnswer' | artemisTranslate" + [confirmTooltip]="'artemisApp.metis.confirmDeleteAnswerWarning' | artemisTranslate"Add to translation files:
"artemisApp.metis.confirmDeleteAnswerWarning": "Are you sure? This action cannot be undone."src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (2)
47-51: Add explicit types to boolean propertiesThe separation of edit and delete permissions is good, but let's enhance type safety.
- mayEdit: boolean; - mayDelete: boolean; + mayEdit = false; + mayDelete = false;
192-199: Improve readability of delete permission logicWhile the logic is correct, we can make it more readable by extracting the complex condition.
setMayDelete(): void { this.isAtLeastInstructorInCourse = this.metisService.metisUserIsAtLeastInstructorInCourse(); const isCourseWideChannel = getAsChannelDTO(this.posting.conversation)?.isCourseWide ?? false; - const mayDeleteOtherUsersAnswer = - (isCourseWideChannel && this.isAtLeastInstructorInCourse) || (getAsChannelDTO(this.metisService.getCurrentConversation())?.hasChannelModerationRights ?? false); - this.mayDelete = !this.readOnlyMode && !this.previewMode && (this.isAuthorOfPosting || mayDeleteOtherUsersAnswer); + const hasModeratorRights = getAsChannelDTO(this.metisService.getCurrentConversation())?.hasChannelModerationRights ?? false; + const mayDeleteOtherUsersAnswer = (isCourseWideChannel && this.isAtLeastInstructorInCourse) || hasModeratorRights; + const isEditingAllowed = !this.readOnlyMode && !this.previewMode; + + this.mayDelete = isEditingAllowed && (this.isAuthorOfPosting || mayDeleteOtherUsersAnswer); this.mayDeleteOutput.emit(this.mayDelete); }src/main/webapp/app/shared/metis/post/post.component.html (1)
Line range hint
152-163: Consider documenting the permission modelThe separation of edit and delete permissions is well implemented. Consider adding documentation (either in the component or in a central location) that explains:
- The permission model for post actions
- How
mayEditandmayDeleteare determined- The relationship between user roles and these permissions
This will help maintain consistency as the permission system evolves.
src/main/webapp/app/shared/metis/post/post.component.ts (1)
80-81: LGTM! Good separation of edit and delete permissions.The separation of
mayEditOrDeleteinto distinctmayEditandmayDeleteproperties improves the granularity of permission control, making it easier to implement the requirement of restricting edit capabilities to post authors while potentially maintaining different rules for deletion.src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (1)
141-159: LGTM! Comprehensive test coverage for edit permissions with room for enhancement.The test cases effectively verify the core requirement that only authors can edit their posts. They follow good testing practices with clear setup and assertions.
Consider enhancing the test suite with additional cases:
- Edge cases (e.g., undefined user roles)
- More specific role combinations (e.g., instructor-author, tutor-author)
Would you like me to help generate additional test cases to cover these scenarios?
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (4)
172-190: Consider a more specific test nameThe test implementation correctly verifies that instructors cannot edit others' posts, which is crucial for this PR. Consider renaming it to be more specific about the instructor role:
- it('should not display the edit option to user (even instructor) if s/he is not the author of posting', () => { + it('should not display the edit option to instructor when they are not the author of the posting', () => {
192-210: Consider enhancing the edit button verificationWhile the test correctly verifies basic edit button visibility, consider adding assertions to verify the button's properties:
- expect(getEditButton()).not.toBeNull(); + const editButton = getEditButton(); + expect(editButton).not.toBeNull(); + expect(editButton?.nativeElement.classList.contains('edit')).toBeTrue();
240-247: Add context about announcement permissionsConsider adding a descriptive comment explaining why tutors cannot modify announcements and making the test name more specific:
- it('should not display edit and delete options to tutor if posting is announcement', () => { + // Announcements are course-wide communications that should only be manageable by instructors + it('should not display edit and delete options to tutor for announcement posts', () => {
248-257: Consider splitting the test for better clarityConsider splitting this test into two separate cases for better clarity and maintainability:
- it('should display edit and delete options to instructor if his posting is announcement', () => { + it('should display edit option to instructor for their own announcement', () => { + metisServiceUserIsAtLeastInstructorStub.mockReturnValue(true); + metisServiceUserIsAuthorOfPostingStub.mockReturnValue(true); + component.posting = metisAnnouncement; + component.ngOnInit(); + fixture.detectChanges(); + expect(getEditButton()).not.toBeNull(); + }); + + it('should display delete option to instructor for their own announcement', () => { + metisServiceUserIsAtLeastInstructorStub.mockReturnValue(true); + metisServiceUserIsAuthorOfPostingStub.mockReturnValue(true); + component.posting = metisAnnouncement; + component.ngOnInit(); + fixture.detectChanges(); + expect(getDeleteButton()).not.toBeNull(); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(2 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(2 hunks)src/main/webapp/app/shared/metis/post/post.component.html(2 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts(4 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts(5 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
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-reactions-bar/answer-post-reactions-bar/answer-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/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/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-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}}
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 (17)
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (4)
1-1: LGTM: Import changes align with new output properties
The addition of the output import supports the new permission-based output properties.
25-28: LGTM: Well-structured permission separation
The separation of edit and delete permissions into distinct outputs and boolean flags improves clarity and aligns with the PR's objective of controlling edit message visibility.
39-40: LGTM: Proper lifecycle hook implementation
The lifecycle hooks correctly initialize and update the separated permission states.
Also applies to: 45-46
83-85: LGTM: Edit permission correctly restricted to authors
The implementation perfectly aligns with the PR objective by ensuring only authors can edit their messages.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (2)
35-36: LGTM! Good separation of edit and delete permissions.
The separation of mayEditOrDeleteOutput into distinct mayDeleteOutput and mayEditOutput events improves clarity and follows the single responsibility principle. This change aligns well with the PR's objective of fixing edit message visibility.
68-74: Verify permission logic implementation.
Please ensure that:
- The
mayEditproperty is properly set based on the user being the original author - Instructors no longer have edit access to messages they didn't author
- Integration tests cover these permission scenarios
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html (1)
76-80: LGTM! Edit button implementation aligns with requirements.
The separation of edit permissions and use of the new @if syntax is correct. This change properly implements the visibility restriction for the edit option as per the PR objectives.
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html (1)
Line range hint 111-121: LGTM! Edit button visibility now correctly reflects user permissions.
The change from mayEditOrDelete to mayEdit properly implements the requirement to restrict editing capabilities based on message authorship. The implementation maintains good accessibility with tooltips and proper disabled states.
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (3)
1-1: LGTM: Import changes align with Angular style guide
76-77: LGTM: Proper initialization of permissions in lifecycle hooks
The permission checks are correctly placed in both ngOnInit and ngOnChanges to ensure proper updates.
Also applies to: 112-113
201-204: LGTM: Edit permissions correctly restricted to authors
The implementation perfectly aligns with the PR objective by ensuring only authors can edit their messages.
src/main/webapp/app/shared/metis/post/post.component.html (2)
83-84: LGTM: Clean separation of edit and delete permissions
The split of the combined output into separate mayEditOutput and mayDeleteOutput events provides better granular control over permissions, which aligns well with the PR objective.
152-157: LGTM: Proper implementation of edit message visibility
The edit button visibility is now correctly controlled by the mayEdit check, which implements the PR objective of restricting edit functionality to message authors. The implementation also follows the coding guidelines by using the new @if syntax.
src/main/webapp/app/shared/metis/post/post.component.ts (1)
116-121: 🛠️ Refactor suggestion
Consider adding permission validation in handlers.
While the handlers correctly implement the basic functionality, consider adding validation to ensure the permission changes are legitimate and authorized. This is especially important for the edit functionality which should only be enabled for post authors.
Example enhancement:
onMayEdit(value: boolean) {
// Validate that the current user is the post author before allowing edit
this.mayEdit = value && this.posting.author?.id === this.currentUserId;
}src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (1)
131-140: LGTM! Well-structured test case for instructor delete permissions.
The test case properly verifies that instructors can delete student posts in course-wide channels while maintaining clear setup and assertions.
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (2)
Line range hint 152-171: LGTM! Well-structured test for delete permissions
The test case properly verifies that users with channel moderation rights can see the delete option even when they're not the author.
258-267: LGTM! Well-structured test for instructor delete permissions
The test case properly verifies that instructors can delete student posts in course-wide channels while maintaining proper role-based access control.
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
feature/communication/update-instructor-edit-message-permission
branch
Checklist
General
Client
Motivation and Context
As agreed, the "edit message" option should only be enabled for users if they are the author of the post. However, instructors currently have the ability to edit messages posted by other users.
Description
The "edit message" option is only visible if the logged-in user is the author of the post.
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
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests