Communication: Add profile pictures to channel member overview
#9450
Conversation
PaRangger
added
client
github-actions
bot
added
tests
server
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces modifications to several components, primarily adding a new 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: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (10)
- src/main/java/de/tum/cit/aet/artemis/core/dto/UserPublicInfoDTO.java (4 hunks)
- src/main/webapp/app/core/user/user.model.ts (1 hunks)
- src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html (1 hunks)
- src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html (1 hunks)
- src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss (2 hunks)
- src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (5 hunks)
- src/main/webapp/i18n/de/metis.json (1 hunks)
- src/main/webapp/i18n/en/metis.json (1 hunks)
- src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts (2 hunks)
- src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/core/dto/UserPublicInfoDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/core/user/user.model.ts (1)
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.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/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.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/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.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/test/javascript/spec/component/overview/course-conversations/course-wide-search.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/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.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 (25)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss (2)
1-1: LGTM: Good use of SCSS variable for profile picture sizingThe introduction of
$profile-picture-heightvariable is a good practice. It ensures consistency in profile picture sizing and makes future updates easier.
Line range hint
1-35: Overall: Well-implemented styles for profile picturesThe changes in this file effectively implement the styles needed for adding profile pictures to the channel member overview, as outlined in the PR objectives. The code follows good SCSS practices, uses variables for consistency, and supports theming through CSS variables. The minor suggestions provided earlier would further enhance the implementation, but the current code is already of high quality and ready for integration.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html (3)
4-13: Excellent implementation of user profile display!The code effectively handles both cases where a user image is available and where it isn't. Here are the strengths of this implementation:
- Correct usage of new Angular syntax (@if and @else) as per coding guidelines.
- Proper fallback to user initials when no image URL is available.
- Inclusion of alt text for the image, enhancing accessibility.
- Consistent styling with background color applied to both image and initials.
This implementation provides a robust and user-friendly way to display user profiles.
Line range hint
1-54: Overall changes enhance user representation without disrupting existing functionality.The modifications to this component successfully introduce profile picture display and user role indication while maintaining the existing structure and functionality. Key points:
- The new profile picture/initials display is well-integrated into the component.
- The addition of the non-student icon enhances user role visibility.
- Existing functionality, such as the dropdown menu for user actions, remains intact.
These changes improve the visual representation of users in the conversation member row without introducing any apparent regressions.
18-20: 🧹 Nitpick (assertive)Good addition of user role indicator, minor formatting suggestion.
The implementation correctly displays an icon for non-student users, which enhances the UI's informativeness. The use of the new Angular @if syntax is correct and aligns with the coding guidelines.
Consider adjusting the indentation of this block to match the surrounding code for better readability:
- @if (!conversationMember.isStudent) { - <fa-icon class="ms-1 text-secondary" [icon]="userIcon" [ngbTooltip]="userTooltip" /> - } + @if (!conversationMember.isStudent) { + <fa-icon class="ms-1 text-secondary" [icon]="userIcon" [ngbTooltip]="userTooltip" /> + }Also, please verify the content of the
userTooltip. Ensure it provides clear and accurate information about the user's role.✅ Verification successful
Verification of
userTooltipAssignmentsThe
userTooltipis correctly assigned based on the user's role:
- Instructor: Displays the appropriate tooltip.
- Tutor: Displays the appropriate tooltip.
- Student: Although the icon isn't shown for students, the tooltip is set appropriately for other contexts.
No issues found with the tooltip assignments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the userTooltip content in the component's TypeScript file rg "userTooltip\s*=" src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.tsLength of output: 511
src/main/java/de/tum/cit/aet/artemis/core/dto/UserPublicInfoDTO.java (5)
28-29: LGTM: New fieldimageUrladded correctly.The addition of the
imageUrlfield aligns with the PR objective of incorporating profile pictures. The field follows proper naming conventions and access modifiers.
48-48: LGTM: Constructor updated to initializeimageUrl.The constructor has been correctly updated to initialize the new
imageUrlfield, maintaining consistency with other field initializations and adhering to the single responsibility principle.
107-114: LGTM: Getter and setter methods forimageUrladded correctly.The
getImageUrl()andsetImageUrl(String imageUrl)methods have been properly implemented, following Java naming conventions and providing appropriate access to the newimageUrlfield.
166-167: LGTM:toString()method updated to includeimageUrl.The
toString()method has been correctly updated to include the newimageUrlfield in the string representation of the object, maintaining consistency with the existing format.
Line range hint
28-167: Overall changes align well with PR objectives and coding guidelines.The addition of the
imageUrlfield, along with its getter, setter, and updates to the constructor andtoString()method, successfully implements the PR objective of incorporating profile pictures into theUserPublicInfoDTO. The changes adhere to Java naming conventions, maintain encapsulation, and follow the single responsibility principle. The implementation is consistent with the existing code structure and meets the requirements specified in the PR objectives.src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html (2)
6-6: LGTM! Proper internationalization and adherence to guidelines.The change correctly implements internationalization for the "All Messages" text using the
jhiTranslatedirective. The use of@ifinstead of*ngIfadheres to the coding guidelines. The translation key "artemisApp.metis.overview.allPublicMessages" seems appropriate and follows a consistent naming convention.
8-8: Excellent implementation of internationalization with dynamic content!The change correctly implements internationalization for the search results text using the
jhiTranslatedirective. The use of@elsewith@ifadheres to the coding guidelines. The translation key "artemisApp.metis.overview.searchResults" is appropriate and follows a consistent naming convention.Notably, the inclusion of the search term as a translation value (
[translateValues]="{ search: courseWideSearchConfig.searchTerm }") is a commendable practice for handling dynamic content in translations.src/main/webapp/i18n/en/metis.json (4)
93-93: LGTM: Added trailing commaThe addition of a trailing comma after the "ANNOUNCEMENT" value is a good practice. It makes future additions to the JSON structure easier and reduces diff noise in version control.
93-95: Summary: Localization enhancements for public messages and search resultsThe changes in this file successfully implement the localization enhancements mentioned in the PR objectives:
- Added a trailing comma to the "ANNOUNCEMENT" key for better maintainability.
- Introduced a new "allPublicMessages" key for localizing the "All Public Messages" string.
- Added a new "searchResults" key with a placeholder for dynamic search terms.
These changes improve the user experience by providing localized strings for public messages and search functionality. The implementation follows existing conventions and is well-integrated into the file structure.
94-94: LGTM: Added "allPublicMessages" localization keyThe addition of the "allPublicMessages" key with the value "All Public Messages" is consistent with the PR objectives and follows the existing naming conventions.
Let's verify the usage of this new key in the codebase:
95-95: LGTM: Added "searchResults" localization keyThe addition of the "searchResults" key with the value "Search Results for '{{ search }}'" is consistent with the PR objectives, follows the existing naming conventions, and correctly uses a placeholder for dynamic content.
Let's verify the usage of this new key in the codebase:
✅ Verification successful
Action Required: Re-run Verification Script
Let's correct and re-execute the script to verify the usage of the new "searchResults" key:
Action Required: Verify "searchResults" Usage in HTML Templates
To ensure the new "searchResults" localization key is properly used in the UI, let's search for its usage in HTML files:
LGTM: "searchResults" Localization Key Verified
The "searchResults" key has been successfully added and is properly utilized within the codebase:
- TypeScript: Used in
import-table.component.tsfor handling search results.- HTML Templates: Implemented in multiple components for dynamic content translation.
No issues were found regarding its implementation and usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new "searchResults" key in the codebase. # Test: Search for the usage of "searchResults" key in TypeScript/JavaScript files. rg --type-add 'web:*.{ts,js}' --type web -A 5 $'searchResults'Length of output: 1225
Script:
# Re-run the verification script with corrected syntax rg --type-add 'web:*.{ts,js}' --type web -A 5 'searchResults'Length of output: 1131
Script:
#!/bin/bash # Description: Verify the usage of the new "searchResults" key in HTML template files. rg --type-add 'templates:*.html' --type templates -A 5 'searchResults'Length of output: 4085
src/main/webapp/i18n/de/metis.json (3)
93-93: LGTM: Trailing comma added.The addition of a trailing comma after the "ANNOUNCEMENT" entry is a good practice. It facilitates easier additions in the future and results in cleaner diffs when changes are made.
94-94: LGTM: New translation added for "All public messages".The new entry "allPublicMessages": "Alle öffentlichen Nachrichten" accurately translates "All public messages" to German. It aligns with the PR objective of localizing the "All Messages" string and specifying that it refers to public messages. The translation correctly uses the informal "du" form as per the coding guidelines.
95-95: LGTM: New translation added for search results.The new entry "searchResults": "Suchergebnisse für {{ search }}" correctly translates and formats the search results string in German. It includes a placeholder for dynamic content ({{ search }}), which is a good practice for localization. This addition aligns with the PR objective of enhancing the course-wide search component. The translation correctly uses the informal "du" form as per the coding guidelines.
src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts (2)
17-17: LGTM: Import statement updated correctly.The addition of
MockDirectiveto the import statement from 'ng-mocks' is a good practice. It allows for proper mocking of directives, which helps in isolating component tests.
17-17: Verify the impact of MockDirective addition on test coverage.The changes to import and use
MockDirectiveforTranslateDirectiveimprove the isolation of the component under test and align with best practices for Angular testing. These modifications enhance the test suite without altering existing tests.To ensure these changes haven't inadvertently affected test coverage, please run the following command and verify that the test coverage remains the same or has improved:
Also applies to: 72-72
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (3)
2-2: LGTM: New imports added correctly.The new imports for
faUserGraduate,getBackgroundColorHue, andgetInitialsFromStringare correctly added and follow the coding guidelines for TypeScript files. These imports support the enhancements for user representation in the component.Also applies to: 23-24
253-253: LGTM: Updated icon for instructor role.The change from
faChalkboardTeachertofaUserGraduatefor the instructor role is correctly implemented and aligns with the PR objective of updating user role icons. The code follows the coding guidelines for TypeScript files.
Line range hint
1-263: LGTM: Successfully implemented profile picture support and user role icon updates.The changes in this file successfully implement the PR objectives:
- Added support for profile pictures with fallback to user initials and background color.
- Updated user role icons, specifically for instructors.
The implementation follows Angular best practices and the provided coding guidelines. It maintains good performance by avoiding unnecessary REST calls and ensures proper unsubscription from observables to prevent memory leaks.
The code is well-structured, readable, and maintains consistency with the existing codebase. Great job on enhancing the user representation in the conversation member row component!
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (1)
25-25: LGTM: Import statement forgetElementutility function.The import statement for the
getElementutility function is correctly placed and follows the existing import structure. This function will be useful for selecting DOM elements in the new test case.
...log/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss
Show resolved
Hide resolved
...log/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss
Show resolved
Hide resolved
...javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts
Show resolved
Hide resolved
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Show resolved
Hide resolved
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Show resolved
Hide resolved
.../tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts
Show resolved
Hide resolved
.../tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts
Show resolved
Hide resolved
| "ANNOUNCEMENT": "Ankündigung" | ||
| "ANNOUNCEMENT": "Ankündigung", | ||
| "allPublicMessages": "Alle öffentlichen Nachrichten", | ||
| "searchResults": "Suchergebnisse für {{ search }}" |
There was a problem hiding this comment.
Since the English translation has the quotation marks, do we want them here as well?
| "searchResults": "Suchergebnisse für {{ search }}" | |
| "searchResults": "Suchergebnisse für '{{ search }}'" |
farisd16
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
farisd16
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS6, your steps produced expected results. However, I noticed that I couldn't find myself as a member of a channel when searching with my first or last name, but only when searching with the TUM identifier. I don't know if your changes have anything to do with this.
krusche
deleted the
feature/communication/display-profile-pictures-in-channel-participants-view
branch
Checklist
General
Client
Motivation and Context
Currently the member overview only shows icons depicting the user roles. Adding profile pictures will further improve the clarity of the module.
Description
I added profile pictures to the member overview screen and moved the icons of the user roles to the end of the name. This was done by adding the imageUrl to the ConversationUserDTO. If the image is null, the default profile picture will be shown.
Additionally, I localized the string for "All Messages" into english/german and added "Public", since it only looks for public messages. The string used to be hardcoded.
Steps for Testing
Prerequisites:
Additionally to check the translations:
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
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
imageUrlfield for displaying user profile images.Bug Fixes
Documentation