Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Learning paths
: Add explanation view for learning path users #9392Learning paths
: Add explanation view for learning path users #9392Changes from 2 commits
13e1d48
617679c
8ccfd0e
9300fe9
280514e
2389c02
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this to the future LearnerProfile, that is stored once per student? Or should they get the explanation in each course?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense for the future. @N0W0RK can you add that point to your backlog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure User Enrollment Verification Before Accessing Learning Path
In the
getLearningPathForCurrentUser
method, it's important to verify that the current user is enrolled in the specified course before accessing the learning path. This prevents unauthorized access to learning paths of courses the user is not enrolled in.Apply this diff to add the enrollment check:
public LearningPathDTO getLearningPathForCurrentUser(long courseId) { final var currentUser = userRepository.getUser(); + courseRepository.checkIfUserIsEnrolledInCourseElseThrow(currentUser, courseId); final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(courseId, currentUser.getId()); return LearningPathDTO.of(learningPath); }
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using non-null assertion operators (
!
).The use of non-null assertions on
this.activatedRoute.parent!.parent!
can lead to runtime errors ifparent
isnull
orundefined
. It's safer to use optional chaining or add null checks to ensure robustness.Consider updating the code as follows:
🧰 Tools
🪛 Biome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential
undefined
forthis.learningPath()
.Using
this.learningPath()!
assumes thatlearningPath
is notundefined
, which might not always be the case. This could result in runtime errors iflearningPath
isundefined
.Modify the code to include a null check:
🧰 Tools
🪛 Biome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure safe usage of
learningPath
in theupdate
function.Inside the
update
function, usinglearningPath!
assumes thatlearningPath
is notundefined
. To prevent potential runtime errors, handle the possibility oflearningPath
beingundefined
.Update the code as follows:
🧰 Tools
🪛 Biome
Check failure on line 153 in src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
GitHub Actions / client-tests
Check failure on line 153 in src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
GitHub Actions / client-tests-selected
Check failure on line 153 in src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
GitHub Actions / client-compilation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Redundant
await
inreturn await
statementIn an
async
function, usingreturn await
is redundant because the function automatically returns a Promise. Removingawait
makes the code more concise without affecting functionality.You can simplify the code by removing
await
:📝 Committable suggestion