Adaptive learning: Learner profile - interface to set feedback preference
#10687
Conversation
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
github-actions
bot
added
server
WalkthroughThis change introduces a new "Learner Profile" feature for collecting and updating user feedback preferences. On the backend, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LearnerProfileComponent (Angular)
participant LearnerProfileApiService
participant Backend API
participant LearnerProfileResource (REST)
participant LearnerProfileRepository
User->>LearnerProfileComponent (Angular): Navigates to Learner Profile Settings
LearnerProfileComponent (Angular)->>LearnerProfileApiService: getLearnerProfileForCurrentUser()
LearnerProfileApiService->>Backend API: GET /api/atlas/learner-profiles
Backend API->>LearnerProfileResource (REST): getLearnerProfile()
LearnerProfileResource (REST)->>LearnerProfileRepository: findByUser()
LearnerProfileRepository-->>LearnerProfileResource (REST): LearnerProfile
LearnerProfileResource (REST)-->>Backend API: LearnerProfileDTO
Backend API-->>LearnerProfileApiService: LearnerProfileDTO
LearnerProfileApiService-->>LearnerProfileComponent (Angular): LearnerProfileDTO
LearnerProfileComponent (Angular)-->>User: Displays profile sliders
User->>LearnerProfileComponent (Angular): Adjusts sliders, clicks Save
LearnerProfileComponent (Angular)->>LearnerProfileApiService: putUpdatedLearnerProfile(learnerProfileDTO)
LearnerProfileApiService->>Backend API: PUT /api/atlas/learner-profiles/{id}
Backend API->>LearnerProfileResource (REST): updateLearnerProfile()
LearnerProfileResource (REST)->>LearnerProfileRepository: findByIdAndUser()
LearnerProfileRepository-->>LearnerProfileResource (REST): LearnerProfile
LearnerProfileResource (REST)->>LearnerProfileRepository: save(updated LearnerProfile)
LearnerProfileRepository-->>LearnerProfileResource (REST): updated LearnerProfile
LearnerProfileResource (REST)-->>Backend API: LearnerProfileDTO
Backend API-->>LearnerProfileApiService: LearnerProfileDTO
LearnerProfileApiService-->>LearnerProfileComponent (Angular): LearnerProfileDTO
LearnerProfileComponent (Angular)-->>User: Shows success or error alert
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
🪧 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
🧹 Nitpick comments (17)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (2)
42-61: Consider default values for the new feedback fieldsThe feedback preference fields are well-defined with appropriate column mapping and validation constraints. However, they don't have default initialization values, which could cause unexpected behavior if accessed before being set.
- private int feedbackPracticalTheoretical; + private int feedbackPracticalTheoretical = 3; // default middle value - private int feedbackCreativeGuidance; + private int feedbackCreativeGuidance = 3; // default middle value - private int feedbackFollowupSummary; + private int feedbackFollowupSummary = 3; // default middle value - private int feedbackBriefDetailed; + private int feedbackBriefDetailed = 3; // default middle value
90-120: Consider adding validation in settersThe setters are simple and follow the standard pattern. However, for better error handling, consider adding validation in the setters to ensure the values comply with the @min(1) and @max(5) constraints.
public void setFeedbackPracticalTheoretical(int feedbackPracticalTheoretical) { + if (feedbackPracticalTheoretical < 1 || feedbackPracticalTheoretical > 5) { + throw new IllegalArgumentException("Feedback value must be between 1 and 5"); + } this.feedbackPracticalTheoretical = feedbackPracticalTheoretical; }This pattern should be applied to all four setter methods.
src/main/webapp/i18n/de/userSettings.json (1)
11-11: Space in "Lerner Profil" may be unnecessaryIn German, compound nouns are typically written as a single word. Consider using "Lernerprofil" instead of "Lerner Profil" for better German localization.
- "learnerProfile": "Lerner Profil", + "learnerProfile": "Lernerprofil",src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.scss (2)
14-44: Consider alternatives to ::ng-deep for component stylingThe use of ::ng-deep is deprecated and will be removed in a future Angular version. While often necessary for styling Angular Material components, it can lead to future maintenance issues.
Consider these alternatives:
- Use component view encapsulation: ViewEncapsulation.None (less preferred)
- Create a global theme extension for Material components
- Use CSS custom properties defined at the component level and consumed inside
Example for option 3:
:host { --slider-thumb-size: 24px; --slider-track-height: 8px; --slider-primary-color: var(--bs-primary); } // Then use these variables in your selectors
61-63: Use theme variables consistentlyThe hover state uses a hardcoded color (#007bff) with the darken function, while the rest of the styles use CSS variables (var(--bs-primary)).
For consistency, consider:
- background-color: darken(#007bff, 5%); + background-color: darken(var(--bs-primary), 5%);Or better yet, use a CSS variable for the hover state:
- background-color: darken(#007bff, 5%); + background-color: var(--bs-primary-dark, darken(var(--bs-primary), 5%));src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (1)
12-14: Simplify HTTP request URLThe string template literal with empty interpolation can be simplified. Also, consider adding error handling for this HTTP request.
getLearnerProfileForCurrentUser(): Observable<LearnerProfileDTO> { - return this.http.get<LearnerProfileDTO>(`${this.resourceUrl}`); + return this.http.get<LearnerProfileDTO>(this.resourceUrl); }src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java (1)
8-8: Add validation or documentation for allowed value rangesThe record declaration is clean, but it would be helpful to document the expected range of values for the feedback fields.
- public record LearnerProfileDTO(long id, int feedbackPracticalTheoretical, int feedbackCreativeGuidance, int feedbackFollowupSummary, int feedbackBriefDetailed) { + /** + * Learner Profile Data Transfer Object. + * Feedback values are expected to be in the range of 1-5, where: + * - 1 represents the first preference (practical, creative, follow-up, brief) + * - 5 represents the second preference (theoretical, focused, summary, detailed) + */ + public record LearnerProfileDTO(long id, int feedbackPracticalTheoretical, int feedbackCreativeGuidance, int feedbackFollowupSummary, int feedbackBriefDetailed) {src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts (3)
46-59: Add loading state indicator during save operationWhile the component tracks the saving state with
isSaving, it doesn't seem to be used in the UI to provide visual feedback to users during the save operation.Consider adding a loading spinner or disabling the save button while saving:
// In the template (learner-profile.component.html), you should have something like: // <button [disabled]="isSaving" (click)="save()"> // <fa-icon *ngIf="!isSaving" [icon]="faSave"></fa-icon> // <span *ngIf="isSaving" class="spinner-border spinner-border-sm" role="status" aria-hidden="true"></span> // {{ 'save' | translate }} // </button>
29-29: Improve documentation for hideTooltip methodThe purpose of the
hideTooltipmethod isn't clear. If it's intended to prevent tooltips from showing, this should be documented.-hideTooltip = () => ''; +// Returns empty string to disable default tooltips on sliders +hideTooltip = () => '';
47-48: Use type for subscribe callback parameterFor better type safety, specify the type for the updatedProfile parameter in the subscribe callback.
this.learnerProfileApiService.putUpdatedLearnerProfile(this.learnerProfile).subscribe({ - next: (updatedProfile: LearnerProfileDTO) => { + next: (updatedProfile: LearnerProfileDTO) => {Note: This is already correctly typed, so I'm approving this part of the code. The type annotation provides good clarity.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (7)
28-30: Consider using an enum for profile value constraintsUsing static final integers for MIN and MAX profile values is good, but an enum would provide better type safety and encapsulation of related constants.
-private static final int MIN_PROFILE_VALUE = 1; -private static final int MAX_PROFILE_VALUE = 5; +private enum ProfileValueBounds { + MIN(1), + MAX(5); + + private final int value; + + ProfileValueBounds(int value) { + this.value = value; + } + + public int getValue() { + return value; + } +}Then update the validation method:
private void validateProfileField(int value, String fieldName) { - if (value < MIN_PROFILE_VALUE || value > MAX_PROFILE_VALUE) { + if (value < ProfileValueBounds.MIN.getValue() || value > ProfileValueBounds.MAX.getValue()) { throw new BadRequestAlertException(fieldName + " field is outside valid bounds", LearnerProfile.ENTITY_NAME, fieldName.toLowerCase() + "OutOfBounds", true); } }
47-51: Consider improving error message clarity in validationThe current error message doesn't specify the valid range, which would be useful information for clients.
private void validateProfileField(int value, String fieldName) { if (value < MIN_PROFILE_VALUE || value > MAX_PROFILE_VALUE) { - throw new BadRequestAlertException(fieldName + " field is outside valid bounds", LearnerProfile.ENTITY_NAME, fieldName.toLowerCase() + "OutOfBounds", true); + throw new BadRequestAlertException(fieldName + " field must be between " + MIN_PROFILE_VALUE + " and " + MAX_PROFILE_VALUE, LearnerProfile.ENTITY_NAME, fieldName.toLowerCase() + "OutOfBounds", true); } }
53-59: Add more comprehensive API documentationThe GET endpoint lacks JavaDoc comments that describe what the method does, unlike the PUT endpoint which has detailed documentation.
+/** + * GET /learner-profiles : get the {@link LearnerProfile} for the current user. + * + * @return A ResponseEntity with status 200 (OK) and the current user's learner profile, + * or an error response if the profile cannot be found. + */ @GetMapping("learner-profiles") @EnforceAtLeastStudent public ResponseEntity<LearnerProfileDTO> getLearnerProfile() {
72-81: Consider consolidating validation checksThe method has multiple separate validation checks. Consider consolidating them for better readability and maintainability.
-if (learnerProfileDTO.id() != learnerProfileId) { - throw new BadRequestAlertException("Provided learnerProfileId does not match learnerProfile.", LearnerProfile.ENTITY_NAME, "objectDoesNotMatchId", true); -} - -Optional<LearnerProfile> optionalLearnerProfile = learnerProfileRepository.findByUser(user); - -if (optionalLearnerProfile.isEmpty()) { - throw new BadRequestAlertException("LearnerProfile not found.", LearnerProfile.ENTITY_NAME, "LearnerProfileNotFound", true); -} +// Validate request integrity +if (learnerProfileDTO.id() != learnerProfileId) { + throw new BadRequestAlertException("Provided learnerProfileId does not match learnerProfile.", LearnerProfile.ENTITY_NAME, "objectDoesNotMatchId", true); +} + +// Fetch and validate learner profile existence +LearnerProfile updateProfile = learnerProfileRepository.findByUser(user) + .orElseThrow(() -> new BadRequestAlertException("LearnerProfile not found.", LearnerProfile.ENTITY_NAME, "LearnerProfileNotFound", true));
83-86: Consider using a batch validation approachThe validation calls are repetitive. Consider using a more elegant approach to validate all fields.
-validateProfileField(learnerProfileDTO.feedbackPracticalTheoretical(), "FeedbackPracticalTheoretical"); -validateProfileField(learnerProfileDTO.feedbackCreativeGuidance(), "FeedbackCreativeGuidance"); -validateProfileField(learnerProfileDTO.feedbackFollowupSummary(), "FeedbackFollowupSummary"); -validateProfileField(learnerProfileDTO.feedbackBriefDetailed(), "FeedbackBriefDetailed"); +// Validate all feedback fields +Map<String, Integer> fieldsToValidate = Map.of( + "FeedbackPracticalTheoretical", learnerProfileDTO.feedbackPracticalTheoretical(), + "FeedbackCreativeGuidance", learnerProfileDTO.feedbackCreativeGuidance(), + "FeedbackFollowupSummary", learnerProfileDTO.feedbackFollowupSummary(), + "FeedbackBriefDetailed", learnerProfileDTO.feedbackBriefDetailed() +); + +fieldsToValidate.forEach(this::validateProfileField);Remember to update the
validateProfileFieldmethod signature to accept these parameters.
88-92: Consider using a builder pattern or mapping utilityThe manual setting of each field is verbose and error-prone. Consider using a mapping utility or builder pattern.
-LearnerProfile updateProfile = optionalLearnerProfile.get(); -updateProfile.setFeedbackPracticalTheoretical(learnerProfileDTO.feedbackPracticalTheoretical()); -updateProfile.setFeedbackCreativeGuidance(learnerProfileDTO.feedbackCreativeGuidance()); -updateProfile.setFeedbackFollowupSummary(learnerProfileDTO.feedbackFollowupSummary()); -updateProfile.setFeedbackBriefDetailed(learnerProfileDTO.feedbackBriefDetailed()); +// Update learner profile fields +updateProfile = updateLearnerProfileFromDTO(updateProfile, learnerProfileDTO); // Add this method to the class: +/** + * Updates a LearnerProfile entity with values from DTO + * + * @param profile The existing profile to update + * @param dto The DTO containing new values + * @return The updated profile + */ +private LearnerProfile updateLearnerProfileFromDTO(LearnerProfile profile, LearnerProfileDTO dto) { + profile.setFeedbackPracticalTheoretical(dto.feedbackPracticalTheoretical()); + profile.setFeedbackCreativeGuidance(dto.feedbackCreativeGuidance()); + profile.setFeedbackFollowupSummary(dto.feedbackFollowupSummary()); + profile.setFeedbackBriefDetailed(dto.feedbackBriefDetailed()); + return profile; +}
93-96: Consider adding transaction managementWhile saving to the repository, it's a good practice to ensure the operation is handled within a transaction.
+import org.springframework.transaction.annotation.Transactional; // In the method signature: +@Transactional public ResponseEntity<LearnerProfileDTO> updateLearnerProfile(@PathVariable long learnerProfileId, @RequestBody LearnerProfileDTO learnerProfileDTO) {However, note that the coding guidelines mention "avoid_transactions", so this suggestion might need further discussion to understand if transactions are handled at a different layer in this application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250409191359_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.scss(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts(1 hunks)src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.html(1 hunks)src/main/webapp/app/core/user/settings/user-settings.route.ts(1 hunks)src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts(1 hunks)src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts(1 hunks)src/main/webapp/i18n/de/learnerProfile.json(1 hunks)src/main/webapp/i18n/de/userSettings.json(1 hunks)src/main/webapp/i18n/en/learnerProfile.json(1 hunks)src/main/webapp/i18n/en/userSettings.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`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/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/learnerProfile.jsonsrc/main/webapp/i18n/de/userSettings.json
`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/**/*.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/core/user/settings/user-settings-container/user-settings-container.component.htmlsrc/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html
`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:PascalC...
src/main/webapp/app/learner-profile/service/learner-profile-api.service.tssrc/main/webapp/app/core/user/settings/user-settings.route.tssrc/main/webapp/app/learner-profile/shared/entities/learner-profile.model.tssrc/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/atlas/dto/LearnerProfileDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java
🧬 Code Graph Analysis (3)
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-tests
- GitHub Check: server-tests
🔇 Additional comments (7)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (1)
32-32: Good addition of entity name constantAdding the entity name as a constant provides consistency for logging and error messages. This follows good practice for entity classes.
src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.html (1)
25-25: Appropriately placed navigation itemThe new learner profile link is appropriately positioned in the navigation menu between the account information and notification settings links. The HTML follows the established pattern and uses the correct translation key.
src/main/webapp/i18n/en/userSettings.json (1)
11-11: Consistent with user settings terminologyThe English translation for "learnerProfile" follows the capitalization pattern of other settings entries (like "Account Information"). The translation is clear and appropriately describes the feature.
src/main/webapp/app/core/user/settings/user-settings.route.ts (1)
28-34: Route implementation looks good!The new 'profile' route correctly follows Angular best practices with proper lazy loading, consistent with the project's pattern. The route structure and naming are clear and aligned with existing routes.
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
1-21: Model implementation is clean and well-structuredThe LearnerProfileDTO class is properly defined with appropriate property names following camelCase convention. The default values (3 for preferences) provide a sensible middle value for initialization.
src/main/webapp/i18n/en/learnerProfile.json (1)
1-36: LGTM: Well-structured localization fileThe localization structure follows good practices with clear hierarchical organization and comprehensive coverage of all UI elements.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (1)
36-39: LGTM - Good use of constructor injectionThe class properly uses constructor injection for dependencies, which follows Spring best practices and the provided Java coding guidelines.
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
WalkthroughThis change introduces a new learner profile feature to the application. It adds a domain entity with feedback preference fields, a corresponding DTO, and a REST API for retrieving and updating the learner profile for the current user. On the frontend, it implements an Angular component and service for displaying and editing learner profile preferences, including UI elements such as sliders and localized text in both English and German. Routing and navigation are updated to include the new learner profile settings page, and associated SCSS styles are provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LearnerProfileComponent (Angular)
participant LearnerProfileApiService
participant Backend REST API
participant LearnerProfileResource (Controller)
participant LearnerProfileRepository
User->>LearnerProfileComponent (Angular): Open Learner Profile Settings
LearnerProfileComponent->>LearnerProfileApiService: getLearnerProfileForCurrentUser()
LearnerProfileApiService->>Backend REST API: GET /api/atlas/learner-profiles
Backend REST API->>LearnerProfileResource: getLearnerProfile()
LearnerProfileResource->>LearnerProfileRepository: findByUser()
LearnerProfileRepository-->>LearnerProfileResource: LearnerProfile
LearnerProfileResource-->>Backend REST API: LearnerProfileDTO
Backend REST API-->>LearnerProfileApiService: LearnerProfileDTO
LearnerProfileApiService-->>LearnerProfileComponent: LearnerProfileDTO
LearnerProfileComponent-->>User: Display profile sliders
User->>LearnerProfileComponent: Adjust sliders & click Save
LearnerProfileComponent->>LearnerProfileApiService: putUpdatedLearnerProfile(updated DTO)
LearnerProfileApiService->>Backend REST API: PUT /api/atlas/learner-profiles/{id}
Backend REST API->>LearnerProfileResource: updateLearnerProfile()
LearnerProfileResource->>LearnerProfileRepository: findByIdAndUser()
LearnerProfileRepository-->>LearnerProfileResource: LearnerProfile
LearnerProfileResource->>LearnerProfileRepository: save(updated LearnerProfile)
LearnerProfileRepository-->>LearnerProfileResource: updated LearnerProfile
LearnerProfileResource-->>Backend REST API: updated LearnerProfileDTO
Backend REST API-->>LearnerProfileApiService: updated LearnerProfileDTO
LearnerProfileApiService-->>LearnerProfileComponent: updated LearnerProfileDTO
LearnerProfileComponent-->>User: Show success message
Possibly related issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (11)
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.scss (3)
29-36: Consider using theme variables for consistent color management.The hardcoded color values for the slider track elements could be replaced with theme variables for better consistency with the application's theme system, especially since you're already using
var(--bs-primary)for active elements.- background-color: #e0e0e0 !important; + background-color: var(--bs-light) !important;This would ensure your component adapts properly to any theme changes in the application.
50-51: Use theme variables for text colors.Similar to the previous comment, it's better to use theme variables for text colors rather than hardcoded values like
#666.- color: #666; + color: var(--bs-secondary-text);
62-62: Use theme variables instead of hardcoded colors.Using a hardcoded color and darken function is less maintainable than using theme variables.
- background-color: darken(#007bff, 5%); + background-color: var(--bs-primary-dark, darken(var(--bs-primary), 5%));src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
1-21: Consider adding type constraints or documentation for valid ranges.The model looks well-structured with appropriate naming conventions. However, since these feedback preference fields likely have a valid range (possibly 1-5 based on the UI sliders), it would be helpful to either:
- Add documentation about the valid range
- Use a more specific type than
numberto ensure correct usage+/** + * Data Transfer Object for Learner Profile settings + * Feedback preferences are on a scale of 1-5, with 3 as the default (neutral) value + */ export class LearnerProfileDTO { public id: number; + /** Feedback preference from 1 (practical) to 5 (theoretical) */ public feedbackPracticalTheoretical: number; + /** Feedback preference from 1 (creative) to 5 (focused) */ public feedbackCreativeGuidance: number; + /** Feedback preference from 1 (follow-up) to 5 (summary) */ public feedbackFollowupSummary: number; + /** Feedback preference from 1 (brief) to 5 (detailed) */ public feedbackBriefDetailed: number;src/main/webapp/i18n/en/learnerProfile.json (1)
5-5: Ensure description matches the translation scope.The current description is quite general and doesn't specifically mention feedback preferences like the German translation does. Consider making it more specific to align with the actual purpose of these settings.
- "description": "Customize your learning experience by adjusting your preferences.", + "description": "Customize your learning experience by adjusting your feedback style preferences.",src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java (1)
10-19: Consider adding documentation for feedback preference valuesThe
ofmethod is well implemented to convert from the domain entity to the DTO. However, consider adding some JavaDoc comments to explain what each feedback preference field represents and what the scale values (1-5) mean.public record LearnerProfileDTO(long id, int feedbackPracticalTheoretical, int feedbackCreativeGuidance, int feedbackFollowupSummary, int feedbackBriefDetailed) { + /** + * Feedback fields represent user preferences on a scale of 1-5 where: + * - feedbackPracticalTheoretical: 1=practical, 5=theoretical + * - feedbackCreativeGuidance: 1=creative, 5=guided/focused + * - feedbackFollowupSummary: 1=follow-up, 5=summary + * - feedbackBriefDetailed: 1=brief, 5=detailed + */ + /** * Creates LearnerProfileDTO from given LearnerProfile. * * @param learnerProfile The given LearnerProfile * @return LearnerProfile DTO for transfer */src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html (3)
25-32: Standardize binding approach across slidersThe first slider uses separate [value] and (valueChange) bindings while the other sliders (lines 42, 53, 64) use [(ngModel)]. For consistency, use the same approach across all sliders.
<mat-slider min="1" max="5" step="1" [discrete]="true" showTickMarks [displayWith]="hideTooltip"> <input matSliderThumb - [value]="learnerProfile.feedbackPracticalTheoretical" - (valueChange)="learnerProfile.feedbackPracticalTheoretical = $event" + [(ngModel)]="learnerProfile.feedbackPracticalTheoretical" (change)="onSliderChange()" /> </mat-slider>
25-65: Add accessibility attributes to slidersThe sliders are missing accessibility attributes such as
aria-labelthat would improve user experience for those using assistive technologies.<mat-slider min="1" max="5" step="1" [discrete]="true" showTickMarks [displayWith]="hideTooltip"> <input matSliderThumb + [attr.aria-label]="'artemisApp.learnerProfile.feedback.feedbackPracticalTheoretical.title' | translate" [value]="learnerProfile.feedbackPracticalTheoretical" (valueChange)="learnerProfile.feedbackPracticalTheoretical = $event" (change)="onSliderChange()" /> </mat-slider>
33-69: Consider adding numeric indicators for slider valuesThe sliders show labels for the extremes (1 and 5), but users might benefit from seeing the current numeric value they've selected. Consider adding a visual indicator of the current value.
<div class="labels"> <span jhiTranslate="artemisApp.learnerProfile.feedback.feedbackPracticalTheoretical.practical"></span> + <span class="current-value">{{ learnerProfile.feedbackPracticalTheoretical }}</span> <span jhiTranslate="artemisApp.learnerProfile.feedback.feedbackPracticalTheoretical.theoretical"></span> </div>src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (1)
90-120: Add documentation to feedback getter/setter methodsConsider adding JavaDoc comments to the getter and setter methods to improve code documentation and explain what each preference represents.
+/** + * Gets the feedback preference for practical vs theoretical content. + * Lower values (1) indicate preference for practical content, higher values (5) for theoretical. + * @return feedback preference value between 1-5 + */ public int getFeedbackPracticalTheoretical() { return feedbackPracticalTheoretical; } +/** + * Sets the feedback preference for practical vs theoretical content. + * @param feedbackPracticalTheoretical value between 1-5 + */ public void setFeedbackPracticalTheoretical(int feedbackPracticalTheoretical) { this.feedbackPracticalTheoretical = feedbackPracticalTheoretical; }src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (1)
61-96: Consider reducing duplication in the update operation.The update method contains redundant code for setting each feedback field. Consider refactoring this to reduce duplication. Additionally, consider adding explicit transaction management for clarity.
+import org.springframework.transaction.annotation.Transactional; @PutMapping(value = "learner-profiles/{learnerProfileId}") @EnforceAtLeastStudent +@Transactional public ResponseEntity<LearnerProfileDTO> updateLearnerProfile(@PathVariable long learnerProfileId, @RequestBody LearnerProfileDTO learnerProfileDTO) { User user = userRepository.getUser(); if (learnerProfileDTO.id() != learnerProfileId) { throw new BadRequestAlertException("Provided learnerProfileId does not match learnerProfile.", LearnerProfile.ENTITY_NAME, "objectDoesNotMatchId", true); } - Optional<LearnerProfile> optionalLearnerProfile = learnerProfileRepository.findByUser(user); - - if (optionalLearnerProfile.isEmpty()) { - throw new BadRequestAlertException("LearnerProfile not found.", LearnerProfile.ENTITY_NAME, "LearnerProfileNotFound", true); - } + LearnerProfile updateProfile = learnerProfileRepository.findByUserElseThrow(user); validateProfileField(learnerProfileDTO.feedbackPracticalTheoretical(), "FeedbackPracticalTheoretical"); validateProfileField(learnerProfileDTO.feedbackCreativeGuidance(), "FeedbackCreativeGuidance"); validateProfileField(learnerProfileDTO.feedbackFollowupSummary(), "FeedbackFollowupSummary"); validateProfileField(learnerProfileDTO.feedbackBriefDetailed(), "FeedbackBriefDetailed"); - LearnerProfile updateProfile = optionalLearnerProfile.get(); updateProfile.setFeedbackPracticalTheoretical(learnerProfileDTO.feedbackPracticalTheoretical()); updateProfile.setFeedbackCreativeGuidance(learnerProfileDTO.feedbackCreativeGuidance()); updateProfile.setFeedbackFollowupSummary(learnerProfileDTO.feedbackFollowupSummary()); updateProfile.setFeedbackBriefDetailed(learnerProfileDTO.feedbackBriefDetailed()); LearnerProfile result = learnerProfileRepository.save(updateProfile); return ResponseEntity.ok(LearnerProfileDTO.of(result)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250409191359_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.scss(1 hunks)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts(1 hunks)src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.html(1 hunks)src/main/webapp/app/core/user/settings/user-settings.route.ts(1 hunks)src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts(1 hunks)src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts(1 hunks)src/main/webapp/i18n/de/learnerProfile.json(1 hunks)src/main/webapp/i18n/de/userSettings.json(1 hunks)src/main/webapp/i18n/en/learnerProfile.json(1 hunks)src/main/webapp/i18n/en/userSettings.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`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/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/userSettings.jsonsrc/main/webapp/i18n/de/learnerProfile.json
`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:PascalC...
src/main/webapp/app/core/user/settings/user-settings.route.tssrc/main/webapp/app/learner-profile/shared/entities/learner-profile.model.tssrc/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.tssrc/main/webapp/app/learner-profile/service/learner-profile-api.service.ts
`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/**/*.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/core/user/settings/user-settings-container/user-settings-container.component.htmlsrc/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/atlas/domain/profile/LearnerProfile.javasrc/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java
🧬 Code Graph Analysis (4)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (1)
src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (3)
src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java (1)
AtlasEnabled(13-25)src/main/webapp/app/learner-profile/shared/entities/learner-profile.model.ts (1)
LearnerProfileDTO(1-21)src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts (1)
save(41-59)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Analyse
- GitHub Check: client-tests
- GitHub Check: server-tests
🔇 Additional comments (14)
src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.html (1)
25-25: The navigation link looks goodThe navigation link for the learner profile is correctly placed and follows the same pattern as other navigation items.
src/main/webapp/i18n/en/userSettings.json (1)
11-11: English translation is correctThe English translation for the learner profile is appropriate and follows the capitalization pattern used in other menu items.
src/main/webapp/app/core/user/settings/user-settings.route.ts (1)
28-34: Route configuration is well-implementedThe route configuration follows Angular best practices with lazy loading and proper naming conventions. The translation key for the page title is correctly referenced.
src/main/webapp/i18n/en/learnerProfile.json (1)
1-36: Well-structured and comprehensive localization entries.The English translation file provides clear, descriptive text for all UI elements related to learner profile feedback preferences. The explanations of each feedback dimension are informative and will help users understand the purpose of each setting.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearnerProfileDTO.java (1)
7-8: Record definition looks goodUsing
recordwith@JsonIncludeis appropriate for this DTO. The structure aligns well with the TypeScript model in the frontend.src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.html (1)
1-17: Header layout and save button implementation look goodThe implementation of the header with conditional save button using
@if(replacing the deprecated*ngIf) is appropriate and follows the guidelines. The save button correctly shows only when changes are detected.src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (1)
7-10: Service setup and dependency injection look goodThe service is properly set up with
@Injectableand the moderninjectpattern for HttpClient injection. The resource URL path is appropriate.src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (1)
32-32: Good addition of ENTITY_NAME constantAdding a constant for the entity name is a good practice for consistency and maintainability.
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts (2)
1-9: Imports look well-organized and follow Angular standards.The imports include all necessary modules for Angular forms, translations, Material components, and FontAwesome icons, following the Angular style guide recommendations.
11-17: Component declaration follows Angular best practices.The component is properly declared as standalone with appropriate imports and follows Angular's style guide for component structure. The selector uses the project's 'jhi-' prefix convention, and stylesheets are properly referenced.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (4)
1-22: Good organization of imports and packages.The imports are well-organized and do not use star imports, which aligns with Java best practices. The code correctly imports necessary classes from appropriate packages.
23-39: Constructor injection is correctly implemented.The class follows Spring best practices by using constructor injection rather than field injection. The conditional annotation ensures this controller is only active when the Atlas feature is enabled, which is a good approach for feature toggling.
41-51: Validation method is concise and reusable.The validation method is well-designed with proper constants for min/max values and throws appropriate exceptions with detailed error messages. This encourages code reuse and maintains consistency in validation.
53-59: GET endpoint is secure and follows REST principles.The endpoint properly enforces student-level access and uses appropriate repository methods. It correctly converts the domain entity to a DTO before returning it, following REST best practices.
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts
Show resolved
Hide resolved
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/learner-profile/learner-profile.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
|
Closing this as the UI needs to adapt to #10440 as that PR already went over the UI with @rabeatwork. |
🚨 Contains DB Migration - Deploy only to TS3
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently, students have no option to adapt the automated feedback they receive from Athena. The system takes the submission and the solution into account and generates feedback. This PR sets the foundation to introduce personalized feedback generation by:
Description
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Style
Documentation