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
Communication
Allow editors to propose FAQ #9457Communication
Allow editors to propose FAQ #9457Changes from 8 commits
0544837
280e3f4
a7dbc99
2eb7316
2be8182
a1c3562
0ab7a91
47e51cc
edf208e
491e368
8045a7f
29add81
ebc28b2
a893024
3154064
87d73ec
db90e89
d8c99a2
2259241
0139625
6deb17c
d801ef8
330a677
1f6cc87
8720762
7beeea6
ba80f4e
059a0b5
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.
🛠️ Refactor suggestion
Ensure the subscription is properly unsubscribed to prevent memory leaks
The
subscribe
method used onthis.activatedRoute.parent?.data
inngOnInit
may lead to a memory leak if the component is destroyed before the observable completes. To prevent this, consider using thetakeUntil
operator with anngOnDestroy
lifecycle hook to unsubscribe when the component is destroyed.Apply this refactor to manage the subscription:
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)
Consider handling potential null values for
this.faq
While assigning
faqState
, ensure thatthis.faq
is not null or undefined to prevent potential runtime errors.Apply this minor adjustment:
📝 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.
🛠️ Refactor suggestion
Refactor duplicated success message code to improve maintainability
The blocks of code displaying success messages in both the creation and update scenarios are similar, with differences only in the translation keys and messages. To reduce duplication and enhance maintainability, consider refactoring the success message logic into a separate method or variable.
Apply this refactor to consolidate the success message handling:
protected onSaveSuccess(faq: Faq) { this.isSaving = false; const isNewFaq = !this.faq.id; const actionKey = isNewFaq ? (this.isAtLeastInstructor ? 'created' : 'proposed') : (this.isAtLeastInstructor ? 'updated' : 'proposedChange'); const translationKey = `artemisApp.faq.${actionKey}`; + this.alertService.success(this.translateService.instant(translationKey, { id: faq.id })); this.router.navigate(['course-management', this.courseId, 'faqs']); }
Then, you can remove the duplicated code blocks from both if-else branches.
Also applies to: 124-128
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)
Display user-friendly labels for FAQ states
Currently, the FAQ state is displayed using
faq.faqState
, which may show the raw enum value (e.g.,PROPOSED
,APPROVED
). To improve user experience, consider displaying a user-friendly label or translating the state.You can apply this change to use the Angular
translate
pipe with a translation key:Ensure that you have the corresponding entries in your translation files, such as:
Let me know if you need assistance in implementing this change or updating the translation files.
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)
Consider allowing tutors to delete their own proposed FAQs
Currently, only instructors can delete FAQs. To enhance usability and allow tutors to manage their own content, consider permitting tutors to delete their own proposed FAQs before they are reviewed by an instructor.