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
CommunicationAllow editors to propose FAQ #9457CommunicationAllow editors to propose FAQ #9457Changes from 7 commits
0544837280e3f4a7dbc992eb73162be8182a1c35620ab7a9147e51ccedf208e491e3688045a7f29add81ebc28b2a893024315406487d73ecdb90e89d8c99a2225924101396256deb17cd801ef8330a6771f6cc8787207627beeea6ba80f4e059a0b5File 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
subscribemethod used onthis.activatedRoute.parent?.datainngOnInitmay lead to a memory leak if the component is destroyed before the observable completes. To prevent this, consider using thetakeUntiloperator with anngOnDestroylifecycle hook to unsubscribe when the component is destroyed.Apply this refactor to manage the subscription:
import { Component, OnInit, OnDestroy, inject } from '@angular/core'; import { Subject } from 'rxjs'; +import { takeUntil } from 'rxjs/operators'; export class FaqUpdateComponent implements OnInit, OnDestroy { private ngUnsubscribe = new Subject<void>(); ngOnInit() { // ... - this.activatedRoute.parent?.data.subscribe((data) => { + this.activatedRoute.parent?.data.pipe(takeUntil(this.ngUnsubscribe)).subscribe((data) => { // existing code }); } + ngOnDestroy() { + this.ngUnsubscribe.next(); + this.ngUnsubscribe.complete(); + } }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.faqWhile assigning
faqState, ensure thatthis.faqis 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
translatepipe 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.
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
rejectFaqandacceptProposedFaqto reduce code duplicationThe methods
rejectFaqandacceptProposedFaqhave similar logic. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring them into a single reusable method.You can create a generic method to handle FAQ state updates:
Then, refactor the original methods to utilize this new function:
This refactoring enhances maintainability and simplifies future updates.