Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tutorial groups: Add profile pictures to tutorial page #9353

Merged

Conversation

PaRangger
Copy link
Contributor

@PaRangger PaRangger commented Sep 22, 2024

Checklist

General

Server

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

Currently, only the name of the tutor is displayed in the tutorial page. For better reference within the communication module, the profile picture is also displayed.

Description

Displayed the profile picture of the specified tutor in the tutorial page. This was done by adding more transient parameters to the tutorial group object (id and image url of the tutor).

Steps for Testing

Prerequisites:

  • 1 Tutor
  • 1 Student
  • 1 Course with a tutorial that has the tutor assigned to it
  1. Log in to Artemis
  2. Navigate to the course
  3. In the sidebar, go to the tutorials tab
  4. Click the specified tutorial
  5. It should show either the image or the default image of the tutor

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Screenshots

Bildschirmfoto 2024-09-22 um 18 13 14

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for displaying teaching assistants' profile pictures in tutorial groups.
    • Enhanced tutorial group details with teaching assistant ID and image URL.
  • Bug Fixes

    • Improved handling of teaching assistant information within the tutorial group service.
  • Localization

    • Added new entries for tutor profile picture labels in both English and German.
  • UI Enhancements

    • Updated detail overview components to dynamically render images and default profile pictures based on available data.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) labels Sep 22, 2024
@PaRangger PaRangger temporarily deployed to artemis-test5.artemis.cit.tum.de September 22, 2024 09:42 — with GitHub Actions Inactive
@PaRangger PaRangger marked this pull request as ready for review September 22, 2024 09:53
@PaRangger PaRangger requested a review from a team as a code owner September 22, 2024 09:53
Copy link

coderabbitai bot commented Sep 22, 2024

Walkthrough

The changes involve the addition of new fields related to teaching assistants in the TutorialGroup class, modifications to the service layer to handle these fields, and updates to the front-end components to display the teaching assistant's profile picture. Additionally, new utility functions and interfaces are introduced to support these features, along with localization updates for the user interface. The test files are adjusted to reflect these changes, ensuring consistency in the data model used across the application.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/domain/TutorialGroup.java Added transient fields teachingAssistantId and teachingAssistantImageUrl with corresponding getters and setters.
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/service/TutorialGroupService.java Updated setTransientPropertiesForUser method to handle new teaching assistant properties.
src/main/webapp/app/course/tutorial-groups/shared/tutorial-group-detail/tutorial-group-detail.component.ts Added logic for displaying teaching assistant's profile picture based on availability of image URL.
src/main/webapp/app/detail-overview-list/detail-overview-list.component.html Introduced new cases in a switch statement for rendering DetailType.Image and DetailType.DefaultProfilePicture.
src/main/webapp/app/detail-overview-list/detail-overview-list.component.scss Added new variable $details-image-height for styling image and default profile picture classes.
src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts Added new enum values DefaultProfilePicture and Image to DetailType enum.
src/main/webapp/app/detail-overview-list/detail.model.ts Introduced new interfaces ImageDetail and DefaultProfilePicture, updated ShownDetail type.
src/main/webapp/app/entities/tutorial-group/tutorial-group.model.ts Added optional properties teachingAssistantId and teachingAssistantImageUrl to TutorialGroup class.
src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts Removed two private methods and updated logic to use new utility functions for generating initials and background colors.
src/main/webapp/app/utils/color.utils.ts Added new utility function getBackgroundColorHue for generating background color hue.
src/main/webapp/app/utils/text.utils.ts Introduced two new functions: deterministicRandomValueFromString and getInitialsFromString.
src/main/webapp/i18n/de/tutorialGroups.json Added new localized strings for profile picture in German.
src/main/webapp/i18n/en/tutorialGroups.json Added new localized strings for profile picture in English.
src/test/javascript/spec/component/tutorial-groups/helpers/tutorialGroupExampleModels.ts Added parameters teachingAssistantId and teachingAssistantImageUrl to generateExampleTutorialGroup function.
src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/create-tutorial-group.component.spec.ts Removed teachingAssistantId and teachingAssistantImageUrl from exampleTutorialGroup object.
src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/edit-tutorial-group.component.spec.ts Removed teachingAssistantId and teachingAssistantImageUrl from exampleTutorialGroup object.

Possibly related PRs

Suggested labels

feature, documentation, small

Suggested reviewers

  • JohannesStoehr
  • az108
  • florian-glombik
  • pzdr7
  • EneaGore
  • BBesrour
  • sarpsahinalp

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Tip

Early access features: enabled

We are currently testing new code review model(s) that may lead to higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 22

Outside diff range comments (4)
src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts (2)

Line range hint 52-53: Consider improving type safety for the sections input property.

To enhance type safety and make the code more robust, consider using a more specific type for the sections input property instead of DetailOverviewSection[].

You could create a custom type or interface that explicitly defines the structure of a section, including the headline and details properties. This would provide better type checking and autocompletion support. For example:

interface DetailOverviewSection {
  headline: string;
  details: Detail[];
}

@Input() sections: DetailOverviewSection[];

Line range hint 89-97: Fix typo in method name downloadApollonDiagramAsPDf

There's a minor typo in the method name where "PDF" is incorrectly capitalized.

Consider renaming the method to downloadApollonDiagramAsPDF for consistency and correct capitalization.

- downloadApollonDiagramAsPDf(umlModel?: UMLModel, title?: string) {
+ downloadApollonDiagramAsPDF(umlModel?: UMLModel, title?: string) {
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/service/TutorialGroupService.java (2)

Line range hint 1-724: Overall assessment: Improved teaching assistant handling with room for further refinement

The changes to the TutorialGroupService class enhance the handling of teaching assistant properties, providing more detailed information when available. This improvement aligns well with the PR objective of displaying profile pictures for tutors on the tutorial page.

Key points:

  1. The implementation correctly handles cases where the teaching assistant is null or not loaded.
  2. The changes are localized and don't introduce any apparent issues in the rest of the class.
  3. There's an opportunity for further refactoring to improve code readability and maintainability.

Consider the following architectural improvements:

  1. Implement a DTO (Data Transfer Object) for teaching assistant information to encapsulate these properties and make them easier to manage and extend in the future.
  2. Review the overall structure of the TutorialGroupService class. At 724 lines, it's quite large and might benefit from being split into smaller, more focused service classes (e.g., TutorialGroupRegistrationService, TutorialGroupExportService, etc.) to adhere better to the Single Responsibility Principle.
  3. Consider using a mapper library like MapStruct to handle the mapping between entities and DTOs, which could simplify and standardize the property setting logic throughout the class.

These suggestions aim to improve the overall architecture and maintainability of the codebase in the long term.


Potential Missing Test Coverage for New Properties

The recent additions of teachingAssistantId and teachingAssistantImageUrl in the TutorialGroup domain class are not reflected in the existing test files. Adequate test coverage is essential to ensure that these new properties behave as expected and to maintain the overall reliability of the TutorialGroupService class.

Actions Required:

  • Manual Verification of Test Coverage:

    • Review Existing Tests: Examine the test cases within AbstractTutorialGroupIntegrationTest.java and other relevant test classes to determine if scenarios involving teachingAssistantId and teachingAssistantImageUrl are being tested.
    • Add Missing Tests: If tests for these new properties are absent, create new test cases to validate their functionality. This includes:
      • Ensuring that teachingAssistantId and teachingAssistantImageUrl are correctly set and retrieved.
      • Verifying that any business logic related to these properties behaves as expected.
      • Checking edge cases, such as null values or invalid data assignments.
  • Update Test Coverage Reports:

    • Ensure that your test coverage tools are configured to include these new properties. This will provide visibility into their coverage status and highlight any remaining gaps.

Enhancing the test suite to cover these new properties will help in maintaining the integrity of the TutorialGroupService and prevent potential issues related to untested code paths.

Analysis chain

Line range hint 1-724: Verify impact on other methods and test coverage

While the changes are localized to the setTransientPropertiesForUser method, it's important to ensure that other parts of the class that might use these new properties are updated accordingly.

Please run the following script to check for any usage of the new properties throughout the codebase:

Also, ensure that the test coverage for the setTransientPropertiesForUser method is updated to include scenarios with and without a teaching assistant, and verify that the new properties are set correctly in each case.


Verify Test Coverage for New Properties

The new properties teachingAssistantId and teachingAssistantImageUrl have been introduced in the TutorialGroup domain class. It's essential to ensure that these additions are covered by existing tests to maintain code reliability.

Please run the following script to check for any usages of the new properties in the test files:

Additionally, ensure that the AbstractTutorialGroupIntegrationTest.java class includes tests that validate the functionality and correctness of the new properties.


Verify Comprehensive Test Coverage for New Properties

The initial search did not find any usage of the teachingAssistantId and teachingAssistantImageUrl properties in the test files. To ensure that these new properties are adequately tested, please run the following scripts to perform a more comprehensive search within the test suite:

Next Steps:

  • Review the Output: After running the above scripts, examine the results to identify if the new properties are being utilized or asserted within your test cases.
  • Update Tests Accordingly: If the properties are not covered, consider writing new test cases to validate their functionality and ensure they behave as expected.
  • Enhance Test Coverage Reports: Ensure that your test coverage tools are updated to include these new properties, providing visibility into their test coverage status.

Ensuring thorough test coverage for these new properties will help maintain the integrity and reliability of the TutorialGroupService class.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of new teaching assistant properties

# Test: Search for usage of teachingAssistantId and teachingAssistantImageUrl
echo "Searching for teachingAssistantId usage:"
rg --type java 'teachingAssistantId'

echo "Searching for teachingAssistantImageUrl usage:"
rg --type java 'teachingAssistantImageUrl'

# Test: Check if there are any test files that need to be updated
echo "Checking for potential test files that might need updates:"
rg --type java 'TutorialGroupService' | grep -i 'test'

Length of output: 1812


Script:

# 
#!/bin/bash
# Description: Check for usage of new teaching assistant properties in test files

echo "Searching for teachingAssistantId usage in test files:"
rg --type java 'teachingAssistantId' src/test/

echo "Searching for teachingAssistantImageUrl usage in test files:"
rg --type java 'teachingAssistantImageUrl' src/test/

Length of output: 349


Script:

# 
#!/bin/bash
# Description: Comprehensive search for new teaching assistant properties in test files

# Search for getter methods related to teachingAssistantId and teachingAssistantImageUrl
echo "Searching for getter methods of teachingAssistantId in test files:"
rg --type java 'getTeachingAssistantId\(\)' src/test/

echo "Searching for getter methods of teachingAssistantImageUrl in test files:"
rg --type java 'getTeachingAssistantImageUrl\(\)' src/test/

# Search for setter methods related to teachingAssistantId and teachingAssistantImageUrl
echo "Searching for setter methods of teachingAssistantId in test files:"
rg --type java 'setTeachingAssistantId\(' src/test/

echo "Searching for setter methods of teachingAssistantImageUrl in test files:"
rg --type java 'setTeachingAssistantImageUrl\(' src/test/

# Search for assertions involving teachingAssistantId and teachingAssistantImageUrl
echo "Searching for assertions on teachingAssistantId in test files:"
rg --type java 'assert.*teachingAssistantId' src/test/

echo "Searching for assertions on teachingAssistantImageUrl in test files:"
rg --type java 'assert.*teachingAssistantImageUrl' src/test/

# Optional: Check for usage in test coverage reports or annotations
echo "Checking for test coverage annotations for teachingAssistantId and teachingAssistantImageUrl:"
rg --type java '@Test.*teachingAssistant' src/test/

Length of output: 1480

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 0354197 and 773c5d5.

Files selected for processing (16)
  • src/main/java/de/tum/cit/aet/artemis/tutorialgroup/domain/TutorialGroup.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/tutorialgroup/service/TutorialGroupService.java (1 hunks)
  • src/main/webapp/app/course/tutorial-groups/shared/tutorial-group-detail/tutorial-group-detail.component.ts (2 hunks)
  • src/main/webapp/app/detail-overview-list/detail-overview-list.component.html (1 hunks)
  • src/main/webapp/app/detail-overview-list/detail-overview-list.component.scss (2 hunks)
  • src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts (1 hunks)
  • src/main/webapp/app/detail-overview-list/detail.model.ts (2 hunks)
  • src/main/webapp/app/entities/tutorial-group/tutorial-group.model.ts (1 hunks)
  • src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts (2 hunks)
  • src/main/webapp/app/utils/color.utils.ts (1 hunks)
  • src/main/webapp/app/utils/text.utils.ts (1 hunks)
  • src/main/webapp/i18n/de/tutorialGroups.json (1 hunks)
  • src/main/webapp/i18n/en/tutorialGroups.json (1 hunks)
  • src/test/javascript/spec/component/tutorial-groups/helpers/tutorialGroupExampleModels.ts (2 hunks)
  • src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/create-tutorial-group.component.spec.ts (1 hunks)
  • src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/edit-tutorial-group.component.spec.ts (1 hunks)
Additional context used
Path-based instructions (14)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/domain/TutorialGroup.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_imports

src/main/java/de/tum/cit/aet/artemis/tutorialgroup/service/TutorialGroupService.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_imports

src/main/webapp/app/course/tutorial-groups/shared/tutorial-group-detail/tutorial-group-detail.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/detail-overview-list/detail-overview-list.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/detail-overview-list/detail-overview-list.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/detail-overview-list/detail.model.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/entities/tutorial-group/tutorial-group.model.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/utils/color.utils.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/utils/text.utils.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/i18n/de/tutorialGroups.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/tutorial-groups/helpers/tutorialGroupExampleModels.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/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/create-tutorial-group.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/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/edit-tutorial-group.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}}

Biome
src/main/webapp/app/course/tutorial-groups/shared/tutorial-group-detail/tutorial-group-detail.component.ts

[error] 92-92: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

src/main/webapp/app/utils/color.utils.ts

[error] 9-9: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/create-tutorial-group.component.spec.ts

[error] 58-58: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)


[error] 59-59: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/edit-tutorial-group.component.spec.ts

[error] 93-93: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)


[error] 94-94: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

Additional comments not posted (17)
src/main/webapp/app/utils/color.utils.ts (2)

1-1: LGTM: Import statement is correct and necessary.

The import statement follows the correct syntax and imports a function that is used in this file. The use of relative import is consistent with Angular style guidelines.


7-13: LGTM: Function implementation is robust and follows best practices.

The getBackgroundColorHue function is well-implemented:

  • It correctly handles undefined input, ensuring robustness.
  • The use of deterministicRandomValueFromString ensures consistent color generation for the same input.
  • The HSL color string is correctly formatted.
  • The function is pure and has no side effects, which is good for maintainability and testing.
  • The use of arrow function syntax is consistent with modern JavaScript practices.
Tools
Biome

[error] 9-9: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

src/main/webapp/app/detail-overview-list/detail-overview-list.component.scss (2)

1-1: LGTM: Good use of SCSS variable for consistency

The introduction of $details-image-height variable is a good practice. It promotes consistency across the stylesheet and makes future adjustments easier.


Line range hint 1-32: Overall assessment: Well-implemented styling for profile pictures

The changes in this file successfully implement the styling required for displaying profile pictures on the tutorial page. The use of SCSS variables, consistent naming conventions, and appropriate CSS properties contribute to a maintainable and visually appealing solution. These changes align well with the PR objective of enhancing the tutorial page by displaying tutor profile pictures.

src/test/javascript/spec/component/tutorial-groups/helpers/tutorialGroupExampleModels.ts (1)

Line range hint 1-77: Summary: Changes successfully implement new teaching assistant fields

The modifications to this file successfully implement the new fields for the teaching assistant's ID and image URL in the generateExampleTutorialGroup function. These changes align with the PR objectives of adding profile pictures to the tutorial page.

The additions are minimal and focused, maintaining the existing structure and functionality of the file. The new parameters have default values, ensuring backwards compatibility with existing tests.

src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/create-tutorial-group.component.spec.ts (1)

58-59: LGTM with a minor suggestion for optimization

The removal of teachingAssistantId and teachingAssistantImageUrl properties from exampleTutorialGroup aligns with the PR objectives to update the tutorial group model. This change ensures that the test data matches the expected structure after the modifications.

For a slight performance optimization, consider using undefined assignment instead of the delete operator:

exampleTutorialGroup.teachingAssistantId = undefined;
exampleTutorialGroup.teachingAssistantImageUrl = undefined;

To ensure that the test coverage remains adequate after these changes, please verify that:

  1. The create method of TutorialGroupsService is still called with the correct parameters.
  2. The navigation after successful creation is still tested.

You can run the following command to check the test coverage for this specific file:

This will help ensure that the changes haven't inadvertently affected the test coverage.

Tools
Biome

[error] 58-58: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)


[error] 59-59: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts (2)

11-12: LGTM: New utility function imports.

The addition of getBackgroundColorHue and getInitialsFromString imports aligns with the refactoring mentioned in the summary. These utility functions replace the removed private methods, promoting code reuse as per the guidelines.


Line range hint 1-108: Overall: Successful refactoring with improved code reuse.

The changes in this file effectively refactor the code to use utility functions for generating initials and background colors. This aligns well with the coding guidelines, particularly in terms of code reuse and maintainability. The functionality remains intact while simplifying the implementation.

Key points:

  1. Removed private methods have been replaced with imported utility functions.
  2. The changes are localized and don't impact the overall structure of the directive.
  3. The modifications adhere to the TypeScript/Angular coding guidelines provided.

The refactoring contributes to the PR's objective of enhancing the tutorial page by providing a more maintainable way to handle user profile picture information.

src/test/javascript/spec/component/tutorial-groups/tutorial-groups-management/tutorial-groups/crud/edit-tutorial-group.component.spec.ts (1)

93-94: Clarify the removal of teaching assistant properties and consider using undefined assignment.

The removal of teachingAssistantId and teachingAssistantImageUrl properties from the exampleTutorialGroup object seems to contradict the PR objective of adding profile pictures to the tutorial page. Could you please clarify the reasoning behind this change?

Additionally, while the use of the delete operator in test files doesn't impact production performance, for consistency with best practices, consider using undefined assignment instead. This approach aligns better with performance recommendations and maintains consistency across the codebase.

Could you please confirm if these properties should indeed be removed from the update operation? If so, could you explain how this aligns with the PR objectives?

As an alternative to using delete, consider the following:

exampleTutorialGroup.teachingAssistantId = undefined;
exampleTutorialGroup.teachingAssistantImageUrl = undefined;

This approach achieves the same result while adhering to performance best practices.

Tools
Biome

[error] 93-93: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)


[error] 94-94: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

src/main/webapp/app/detail-overview-list/detail.model.ts (2)

25-26: LGTM: New detail types added correctly

The addition of ImageDetail and DefaultProfilePicture to the ShownDetail type is consistent with the PR objectives and follows the correct naming convention (PascalCase). These changes enhance the model's capability to handle image-related details and default profile picture specifications without breaking existing functionality.


68-71: LGTM: DefaultProfilePicture interface added correctly

The DefaultProfilePicture interface is well-structured and follows the coding guidelines. It extends DetailBase appropriately and includes relevant properties for default profile pictures. The use of required properties for color and initials ensures that necessary information is always provided when using this interface.

src/main/webapp/app/detail-overview-list/detail-overview-list.component.html (1)

127-136: Summary: Profile picture display successfully implemented

The changes to add profile picture display functionality to the detail overview list component have been implemented correctly. The new cases for DetailType.Image and DetailType.DefaultProfilePicture follow Angular best practices and the provided coding guidelines. Minor suggestions for improving accessibility have been made.

These changes align well with the PR objectives of enhancing the tutorial page by displaying profile pictures of tutors alongside their names, which should improve user reference within the communication module.

src/main/webapp/app/course/tutorial-groups/shared/tutorial-group-detail/tutorial-group-detail.component.ts (2)

14-15: LGTM: New imports for utility functions.

The new imports for getInitialsFromString and getBackgroundColorHue are correctly added and follow the coding guidelines. These utility functions will be used for generating default profile pictures, which aligns with the PR objectives.


Line range hint 1-180: Summary: Successful implementation of profile picture functionality.

The changes in this file successfully implement the display of teaching assistant profile pictures in the tutorial group details. The implementation:

  1. Adds necessary utility function imports.
  2. Enhances the getTutorialDetailSections method to include profile picture logic.
  3. Gracefully handles cases with and without profile picture URLs.
  4. Generates default profile pictures with initials and color-coded backgrounds when needed.

These changes align well with the PR objectives and follow Angular best practices and the provided coding guidelines. The implementation is clean, localized, and doesn't introduce any breaking changes to existing functionality.

Tools
Biome

[error] 92-92: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

src/main/java/de/tum/cit/aet/artemis/tutorialgroup/domain/TutorialGroup.java (2)

305-315: LGTM: Getter methods for new fields

The getter methods for teachingAssistantId and teachingAssistantImageUrl are correctly implemented and properly annotated. They follow the Java naming conventions and ensure that these fields are included in JSON serialization, which aligns with the PR objective of displaying tutor information.


Line range hint 1-428: Summary of TutorialGroup class changes

The changes to the TutorialGroup class successfully implement the addition of teachingAssistantId and teachingAssistantImageUrl fields, along with their corresponding getter and setter methods. These changes align with the PR objective of enhancing the tutorial page by displaying profile pictures of tutors.

Key points:

  1. The new fields are correctly annotated as @Transient.
  2. Getter methods are properly annotated for JSON serialization.
  3. The implementation follows Java naming conventions and coding guidelines.

Suggestions for improvement:

  1. Update the comments for the new fields to accurately describe their purpose.
  2. Add input validation for the setter methods to ensure data integrity and URL format correctness.

Overall, the changes are well-implemented and achieve the desired functionality. Addressing the suggested improvements will enhance the robustness and clarity of the code.

src/main/webapp/i18n/de/tutorialGroups.json (1)

57-57: LGTM: Appropriate translation for "profilePicture"

The translation "Tutor:in Profilbild" is concise, clear, and uses gender-neutral language. It adheres to the informal language guideline (dutzen) as required.

Copy link
Contributor

@ole-ve ole-ve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the code, looks good 👍

Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on Ts2

Copy link
Contributor

@sarpsahinalp sarpsahinalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ts2, with both profile picture and default profile picture

Copy link
Contributor

@dmytropolityka dmytropolityka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works on ts2

Copy link
Contributor

@asliayk asliayk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested locally and works as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) component:TutorialGroups ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants