Communication: Allow users to reference FAQs in post
#9566
Conversation
github-actions
bot
added
tests
client
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/shared/metis/metis.util.ts (1)
108-108: Add JSDoc documentation for the FAQ enum value.Consider adding JSDoc documentation to describe the FAQ reference type, similar to how interfaces like
PostingContentPartare documented.CHANNEL = 'CHANNEL', + /** References to Frequently Asked Questions (FAQs) in course content */ FAQ = 'FAQ',src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
90-90: Consider grouping related actions together.The FAQ action could be grouped with other communication-related actions for better organization. Consider moving it next to
messagingOnlyActionssince they're both communication features.new CodeAction(), new CodeBlockAction(), ...messagingOnlyActions, + ...faqAction, new ExerciseReferenceAction(this.metisService), - ...faqAction,src/main/webapp/app/entities/course.model.ts (1)
32-37: Consider adding return type annotation for consistencyThe function implementation is clean and follows the established pattern. For better type safety and consistency with TypeScript best practices, consider adding an explicit return type annotation.
-export function isFaqEnabled(course: Course | undefined) { +export function isFaqEnabled(course: Course | undefined): boolean {src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (2)
208-208: Update regex pattern documentation.The comments above the regex pattern need to be updated to include the description for the FAQ group (Group 13).
Update the comments above the pattern to include:
// Group 13: reference pattern for FAQs
119-122: Consider refactoring reference type handling for better maintainability.As more reference types are added (like FAQ), the
computePostingContentPartsmethod becomes increasingly complex with multiple if-else blocks. Consider refactoring this to use a strategy pattern or a map of handlers for better maintainability.Here's a suggested approach:
interface ReferenceHandler { extractReferenceStr(content: string, match: PatternMatch): string; extractLinkAndParams(content: string, match: PatternMatch): { link?: string[], params?: Params }; } private referenceHandlers = new Map<ReferenceType, ReferenceHandler>([ [ReferenceType.FAQ, { extractReferenceStr: (content, match) => content.substring(content.indexOf(']', match.startIndex)! + 1, content.indexOf('(', match.startIndex)!), extractLinkAndParams: (content, match) => { // FAQ-specific extraction logic return { link: [...], params: { faqId: ... } }; } }], // Add other handlers... ]);This would make it easier to:
- Add new reference types without modifying existing code
- Test each reference type handler independently
- Maintain the logic for each reference type separately
Would you like me to help create a detailed implementation plan for this refactoring?
src/main/webapp/app/shared/metis/metis.service.ts (1)
497-504: Fix incorrect JSDoc description.The JSDoc comment incorrectly describes this method as returning a link for an "exercise referenced within a faq". This should be updated to accurately reflect the method's purpose.
- /** - * returns the router link required for navigating to the exercise referenced within a faq - * @return {string} router link of the faq - */ + /** + * returns the router link required for navigating to the course's FAQ section + * @return {string} router link of the FAQ page + */src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (1)
495-509: LGTM! Consider using consistent test data.The test case for FAQ references is well-structured and follows the established patterns. However, for consistency with other test cases, consider using more descriptive test data.
Consider this minor improvement:
- component.content = `I want to reference [faq]faq(/courses/1/faq?faqId=45)[/faq].`; + component.content = `I want to reference [faq]How to submit assignments(/courses/1/faq?faqId=45)[/faq].`;This makes the test data more realistic and consistent with other test cases that use descriptive reference strings.
src/main/webapp/app/overview/course-overview.component.ts (1)
192-193: Consider maintaining consistency in dependency injection patternWhile using
inject()is a modern approach, consider maintaining consistency with the rest of the service injections that use constructor-based dependency injection. This helps maintain a uniform codebase pattern.- private faqService = inject(FaqService); + constructor( + private courseService: CourseManagementService, + private courseExerciseService: CourseExerciseService, + // ... other services ... + private faqService: FaqService, + ) {}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- src/main/webapp/app/entities/course.model.ts (3 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.html (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (6 hunks)
- src/main/webapp/app/overview/course-overview.component.ts (5 hunks)
- src/main/webapp/app/shared/metis/metis.service.ts (1 hunks)
- src/main/webapp/app/shared/metis/metis.util.ts (1 hunks)
- src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (2 hunks)
- src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (4 hunks)
- src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts (1 hunks)
- src/main/webapp/i18n/de/metis.json (1 hunks)
- src/main/webapp/i18n/en/metis.json (1 hunks)
- src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (1 hunks)
- src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/main/webapp/app/entities/course.model.ts (1)
src/main/webapp/app/overview/course-faq/course-faq.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/overview/course-faq/course-faq.component.ts (1)
src/main/webapp/app/overview/course-overview.component.ts (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/main/webapp/app/shared/metis/metis.util.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts (1)
src/main/webapp/i18n/de/metis.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/shared/metis/posting-content/posting-content.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/shared/metis/postings-markdown-editor/postings-markdown-editor.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}}
🔇 Additional comments (16)
src/main/webapp/app/overview/course-faq/course-faq.component.html (2)
35-37: LGTM! Well-structured FAQ element wrapper.The addition of the wrapper div with template reference and dynamic ID provides the necessary DOM hooks for programmatic FAQ navigation, which aligns well with the PR's objective of enabling FAQ references in posts.
Line range hint
1-43: LGTM! Consistent use of new Angular control flow syntax.The template correctly uses the new
@ifand@forsyntax throughout, following the latest Angular best practices and project guidelines.src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (3)
20-20: LGTM: Import statements are properly organized.The new imports for FAQ functionality are correctly placed and follow Angular style guidelines.
Also applies to: 33-33
79-80: LGTM: Clean implementation of conditional FAQ action.The feature flag check and array initialization follow the established pattern and best practices.
Line range hint
79-90: Verify handling of FAQ feature state changes.While the initial FAQ feature state is correctly handled, we should ensure that the component properly responds to runtime changes in the FAQ feature state.
✅ Verification successful
Let me try a different approach to verify the FAQ feature state handling.
This should help us understand:
- How the FAQ feature state is managed
- If there are any subscriptions in the component
- If there are any observables for FAQ state changes
Let me try one final verification to check the MetisService implementation for any course state updates.
Based on the verification results, I can now provide a final response:
FAQ feature state changes are handled appropriately.
The implementation correctly handles FAQ feature state changes because:
- The FAQ action is determined by
isFaqEnabled()which is a pure function that checks the current course state- The course state is managed by MetisService which is injected into the component
- The component initializes actions in
ngOnInit()and doesn't need to subscribe to course state changes because:
- The editor actions are only used for new instances of the editor
- Any changes to the course's FAQ state would result in a route change/component recreation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any runtime feature state change handlers rg -l "isFaqEnabled.*subscribe" # Check if there are any feature toggle services or similar patterns ast-grep --pattern 'class $_ implements OnDestroy { $$$ ngOnDestroy() { $$$ unsubscribe $$$ } }'Length of output: 274
Script:
#!/bin/bash # Search for FAQ-related code and state management patterns rg -A 5 "isFaqEnabled" # Look for any subscriptions or state management in the component rg -A 10 "class PostingMarkdownEditorComponent" # Check if there are any FAQ-related observables or subjects rg "FAQ.*Subject|FAQ.*Observable"Length of output: 7100
Script:
#!/bin/bash # Check MetisService implementation for course state management rg -A 10 "class MetisService" # Look for any course state updates or observables rg "getCourse\(\)|courseSubject|courseUpdate" # Check if the component subscribes to any course updates ast-grep --pattern 'class PostingMarkdownEditorComponent { $$$ ngOnInit() { $$$ subscribe $$$ } }'Length of output: 96602
src/main/webapp/app/entities/course.model.ts (3)
16-16: LGTM: Import statement follows Angular conventionsThe import statement follows the Angular style guide and maintains consistency with other model imports.
109-109: LGTM: Property follows established patternsThe faqs property is correctly implemented following the class's established patterns for array properties and TypeScript conventions.
109-109: Verify FAQ feature integration pointsSince this model change enables FAQ references in posts, let's verify the integration points:
✅ Verification successful
Let me gather more specific information about the FAQ integration by checking the actual implementation details.
Based on the gathered information, I can now provide a final response as the integration points are clear.
FAQ feature is properly integrated across the codebase
The FAQ feature shows complete and proper integration:
- The
Coursemodel correctly defines the optionalfaqspropertyFaqReferenceActionis properly implemented for editor integrationCourseFaqComponenthandles the FAQ functionality with proper course relationship- Proper bidirectional relationship exists between
CourseandFaqmodels- Feature toggle through
isFaqEnabledis correctly implemented and used in the markdown editorThe code shows a well-structured implementation with:
- Proper model relationships
- Complete UI components
- Feature toggle support
- Search and filter capabilities
- Proper routing and navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper integration of FAQ feature across the codebase # Test 1: Check for proper usage of isFaqEnabled function echo "Checking isFaqEnabled function usage..." rg "isFaqEnabled" -t ts # Test 2: Check for FAQ-related components and services echo "Checking FAQ-related components..." fd -e ts -e html . src/main/webapp/app | rg -i "faq" # Test 3: Verify proper typing in components using Course model with faqs echo "Checking Course model usage with faqs property..." ast-grep --pattern 'course: Course' -l tsLength of output: 2038
Script:
#!/bin/bash # Check the implementation of FAQ-related components and their interaction with Course model # Check FaqReferenceAction implementation echo "Checking FaqReferenceAction implementation..." rg -A 5 "class FaqReferenceAction" src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts # Check Course FAQ component implementation echo "Checking Course FAQ component implementation..." cat src/main/webapp/app/overview/course-faq/course-faq.component.ts # Check FAQ model and its relationship with Course echo "Checking FAQ model..." cat src/main/webapp/app/entities/faq.model.ts # Check actual usage of faqs property in Course model echo "Checking usage of faqs property..." rg "faqs\?" -t tsLength of output: 6914
src/main/webapp/i18n/en/metis.json (1)
49-50: LGTM! The FAQ translation entry is properly added.The new translation entry follows the existing pattern and uses the widely recognized "FAQ" acronym.
Let's verify the consistency of FAQ translations across language files:
✅ Verification successful
Let me gather more specific information about which language files we're dealing with:
Let me check the exact content of both files to ensure they're properly synchronized:
FAQ translation is properly synchronized across both language files
The verification confirms that:
- Both English (en) and German (de) localization files contain the FAQ translation
- The translation is consistently set as "FAQ" in both files, which is appropriate as it's a commonly recognized acronym in both languages
- The entry is properly placed in the same position (after "user") in both files
- The JSON structure is maintained correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if FAQ translation exists in all language files # Expected: All language files should have the editor.faq translation # Find all language files fd 'metis.json' 'src/main/webapp/i18n' --exec jq -r '.artemisApp.metis.editor.faq // "Missing FAQ translation"' {}Length of output: 122
Script:
#!/bin/bash # List all metis.json files with their paths to identify the language files fd 'metis.json' 'src/main/webapp/i18n' --exec-batch echo "{}"Length of output: 129
Script:
#!/bin/bash # Show the editor section of both language files echo "=== English (en) ===" jq '.artemisApp.metis.editor' src/main/webapp/i18n/en/metis.json echo -e "\n=== German (de) ===" jq '.artemisApp.metis.editor' src/main/webapp/i18n/de/metis.jsonLength of output: 513
src/main/webapp/i18n/de/metis.json (1)
49-50: LGTM! The translation is appropriate.The added FAQ translation maintains consistency with German web terminology and the informal language style used throughout the file.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
27-27: LGTM!The import statement follows the established pattern and is correctly placed with other action imports.
src/main/webapp/app/shared/metis/metis.service.ts (1)
501-503: LGTM: Method implementation follows Angular patterns.The implementation is clean, follows the established patterns in the service for link generation, and correctly uses the course ID from the service state.
src/main/webapp/app/overview/course-overview.component.ts (2)
71-72: LGTM: Clean import additionsThe new imports for FAQ functionality are well-organized and follow Angular's style guide.
Line range hint
450-465: LGTM: Clean FAQ menu integrationThe FAQ menu item integration follows the established pattern and correctly handles feature toggling through the
faqEnabledflag.src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts (1)
64-67: Proper disposal of resourcesGood job ensuring that
disposableCompletionProvideris disposed of in thedisposemethod, preventing potential memory leaks.src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
128-130: ThesortFaqs()method is correctly implementedThe method effectively sorts the FAQs by their
idin ascending order using theSortService.
...c/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
Outdated
Show resolved
Hide resolved
...est/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (3)
163-167: Enhance sort test coverage.While the test verifies that
sortService.sortByPropertyis called, it could be more comprehensive:
- Verify the correct parameters are passed to
sortByProperty- Assert the effect on component's state after sorting
Consider this improved implementation:
it('should call sortService when sortRows is called', () => { - jest.spyOn(sortService, 'sortByProperty').mockReturnValue([]); + const sortedFaqs = [faq2, faq1, faq3]; + jest.spyOn(sortService, 'sortByProperty').mockReturnValue(sortedFaqs); courseFaqComponent.sortFaqs(); - expect(sortService.sortByProperty).toHaveBeenCalledOnce(); + expect(sortService.sortByProperty).toHaveBeenCalledExactlyOnceWith( + courseFaqComponent.faqs, + 'questionTitle', + 'asc' + ); + expect(courseFaqComponent.faqs).toEqual(sortedFaqs); });
169-179: Improve query params test setup.While the test verifies the correct behavior, the setup could be improved:
Consider this more maintainable approach:
it('should call scrollToFaq when faqId is in query params', () => { const scrollToFaqSpy = jest.spyOn(courseFaqComponent, 'scrollToFaq'); - - const route = TestBed.inject(ActivatedRoute); - (route.queryParams as any) = of({ faqId: '1' }); - + TestBed.overrideProvider(ActivatedRoute, { + useValue: { + parent: { + params: of({ courseId: '1' }) + }, + queryParams: of({ faqId: '1' }) + } + }); + + // Re-create component to pick up new route config + courseFaqComponentFixture = TestBed.createComponent(CourseFaqComponent); + courseFaqComponent = courseFaqComponentFixture.componentInstance; courseFaqComponent.goToFaq(); expect(scrollToFaqSpy).toHaveBeenCalledOnce(); expect(scrollToFaqSpy).toHaveBeenCalledWith('1'); });
181-195: Enhance scroll and focus test robustness.The test could be improved in several ways:
- Add test case for element not found scenario
- Use proper typing for the mock element
- Match the actual component's element id format
Consider these improvements:
+interface MockHTMLElement extends Partial<HTMLElement> { + scrollIntoView: jest.Mock; + focus: jest.Mock; +} it('should scroll and focus on the faq element with given id', () => { - const nativeElement = { - id: 'faq-1', + const nativeElement: MockHTMLElement = { + id: 'faq_1', scrollIntoView: jest.fn(), focus: jest.fn(), }; const elementRef = new ElementRef(nativeElement); const queryList = new QueryList<ElementRef>(); queryList.reset([elementRef]); courseFaqComponent.faqElements = queryList; + + // Test successful case courseFaqComponent.scrollToFaq(1); - expect(nativeElement.scrollIntoView).toHaveBeenCalledWith({ behavior: 'smooth', block: 'start' }); expect(nativeElement.focus).toHaveBeenCalled(); + + // Test element not found + courseFaqComponent.scrollToFaq(999); + expect(nativeElement.scrollIntoView).toHaveBeenCalledTimes(1); // Should not be called again + expect(nativeElement.focus).toHaveBeenCalledTimes(1); // Should not be called again });src/test/javascript/spec/service/metis/metis.service.spec.ts (1)
333-337: Test implementation looks good, but could be more comprehensive.The basic test case correctly verifies the FAQ link generation. However, consider enhancing the test coverage:
Consider adding these test cases:
it('should determine the router link required for referencing a faq', () => { + metisService.setCourse(metisCourse); const link = metisService.getLinkForFaq(); expect(link).toBe(`/courses/${metisCourse.id}/faq`); }); +it('should handle faq link generation when course is not set', () => { + metisService.setCourse(undefined); + const link = metisService.getLinkForFaq(); + expect(link).toBeUndefined(); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (4 hunks)
- src/test/javascript/spec/service/metis/metis.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/overview/course-faq/course-faq.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/service/metis/metis.service.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}}
🔇 Additional comments (1)
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)
22-23: LGTM: Clean setup of new dependencies!The SortService integration follows testing best practices with proper mocking and initialization.
Also applies to: 41-41, 110-110
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
103-104: LGTM with a suggestion for future maintainability.The FAQ case is correctly implemented. However, as the number of reference types grows, consider refactoring this switch statement into a mapping object for better maintainability.
Consider refactoring to use a mapping object:
private readonly referenceIconMap: Record<ReferenceType, IconProp> = { [ReferenceType.POST]: faMessage, [ReferenceType.LECTURE]: faChalkboardUser, // ... other mappings [ReferenceType.FAQ]: faQuestion, }; referenceIcon(reference: ReferenceType): IconProp { return this.referenceIconMap[reference] ?? faPaperclip; }This approach would:
- Reduce cognitive complexity
- Make it easier to add new reference types
- Potentially improve performance by eliminating the switch statement
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (6 hunks)
- src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (3 hunks)
- src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
src/test/javascript/spec/component/overview/course-faq/course-faq.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}}
🔇 Additional comments (3)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
18-18: LGTM: Icon import and property declaration follow guidelines.The addition of the
faQuestionicon follows the established patterns and Angular style guidelines.Also applies to: 47-47
47-47: Verify FAQ icon display in the UI.While the implementation looks correct, please ensure that:
- The FAQ icon is visible and properly aligned in the UI
- The icon maintains consistent styling with other reference icons
- The icon is visible in both light and dark themes
Also applies to: 103-104
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)
22-23: LGTM: Clean setup of SortService dependency!The setup follows Angular testing best practices with proper dependency injection.
Also applies to: 41-41, 110-110
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts
Outdated
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Hi, I found some issues:
-
After clicking on a refereced FAQ, it doesn't show up in the FAQs overview, it just dissapear from the FAQs list
In the first screenshot are all the faqs available
After clicking on faq1, which is referenced in a channel, I was redirect to the page with all the faqs, but the faq1 is nowhere to be found
-
After disabling FAQ, I can still see and click onto already referenced FAQs. Referencing new faq is, as expected, not possible
|
cremertim
dismissed stale reviews from coderabbitai[bot], anian03, julian-wls, PaRangger, asliayk, florian-glombik, and eylulnc
via
b6ecb0e
src/test/java/de/tum/cit/aet/artemis/core/DatabaseQueryCountTest.java
Outdated
Show resolved
Hide resolved
cremertim
dismissed stale reviews from coderabbitai[bot], PaRangger, and florian-glombik
via
03c5ae9
flbrgit
added
deploy:artemis-test3
deploy:artemis-test5
and removed
deploy:artemis-test3
labels
flbrgit
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS5. After disabling the FAQ, you can't reference it anymore by the message options unless you copy an existing reference or type it in manually. Then it's still possible to access the FAQ, even though it is disabled. It's just confusing that the FAQ then does not show up in the task bar. As this is not covered with this PR, reapprove
Checklist
General
Client
Motivation and Context
We introduced the new FAQ feature. For now, it is not able to directly reference a FAQ. It is cumbersome for Students to manually navigate into the FAQ section and manually search for the corresponding FAQ, if they are hinted to it.
Description
The markdown editor now offers a new Action: FAQ-Reference Action. IT will create a link to the corresponding FAQ, and automatically scrolles to the referenced FAQ.
Steps for Testing
Prerequisites:
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
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Walkthrough
The changes in this pull request involve enhancements related to Frequently Asked Questions (FAQs) across multiple components of the application. Key modifications include the introduction of new properties and methods in the
Coursemodel to manage FAQs, updates to theCourseFaqComponentfor dynamic scrolling and sorting of FAQ items, and enhancements in theCourseOverviewComponentfor fetching FAQs. Additionally, new routing capabilities for FAQs are added in theMetisService, alongside updates to localization files and test suites to ensure comprehensive coverage of the new features.Changes
src/main/webapp/app/entities/course.model.tsFaq, new function `isFaqEnabled(course: Coursesrc/main/webapp/app/overview/course-faq/course-faq.component.html<hr>element and added a<div>wrapper around<jhi-course-faq-accordion>withidand template reference variable.src/main/webapp/app/overview/course-faq/course-faq.component.tsfaqIdandfaqElements, and methodssortFaqs()andscrollToFaq(faqId: number). UpdatedngOnInitto handlefaqIdfrom query parameters.src/main/webapp/app/overview/course-overview.component.tsFaqandFaqServiceimports, introduced method `setFaqs(course: Coursesrc/main/webapp/app/shared/metis/metis.service.tsgetLinkForFaq(): stringto generate a router link for FAQs.src/main/webapp/app/shared/metis/metis.util.tsReferenceTypeenum to includeFAQ.src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.tsfaQuestionicon forReferenceType.FAQinreferenceIconmethod.src/main/webapp/app/shared/metis/posting-content/posting-content.components.tscomputePostingContentPartsmethod to handleFAQreferences and modified regex ingetPatternMatchesto include[faq]...[/faq].src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.tsisFaqEnabledandFaqReferenceAction, integrated FAQ action intongOnInit.src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.tsFaqReferenceActionclass for inserting FAQ references in the text editor.src/main/webapp/i18n/de/metis.json"faq": "FAQ"in German.src/main/webapp/i18n/en/metis.json"faq": "FAQ"in English.src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.tsSortService, added tests for sorting and scrolling FAQs.src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.tssrc/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.tsPostingMarkdownEditorComponentto validate behavior with FAQ feature toggled on and off.src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.tsFaqReferenceAction.src/test/javascript/spec/helpers/mocks/service/mock-metis-service.service.tsgetLinkForFaq()toMockMetisService.src/test/javascript/spec/helpers/sample/metis-sample-data.tsmetisFaq1,metisFaq2,metisFaq3and added them tometisCourseunderfaqs.src/test/javascript/spec/service/metis/metis.service.spec.tsgetLinkForFaq()method inMetisService.Possibly related PRs
Communication: Add FAQs to Artemis #9325: Introduces new functions and properties related to FAQs in thecourse.model.tsfile, enhancing the management of FAQs.Communication: Allow tutors to propose FAQ #9477: Allows tutors to propose FAQs, directly relating to the changes in this PR that enhance the FAQ management system.Communication: Add default message to empty FAQ view #9467: Adds default messages for empty FAQ views, complementing the changes that enhance the user interface for displaying FAQs.Communication: Remove announcements from unresolved filter #9561: Modifies filtering logic for conversations, relevant to refining the filtering of conversations to display only relevant channels.Development: Fix console errors on course overview page #9526: Addresses console errors related to undefined checks in the FAQ component, enhancing the robustness of the component's rendering logic.Communication: Reduce height of channel and chat item card #9480: Reduces the height of channel and chat item cards, related to overall user interface improvements in the FAQ section.Development: Remove deprecated router module in client tests #9439: Removes deprecated router modules, relevant to the modernization of the application's structure, including the FAQ management system.Development: Migrate the git diff report module to standalone components #9443: Migrates the git diff report module to standalone components, aligning with modernization efforts in the FAQ management system.Suggested labels
ready to merge,communication,small,bugfixSuggested reviewers
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:
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.@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.@coderabbitaiin 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 pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.Other keywords and placeholders
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed.@coderabbitai summaryto generate the high-level summary at a specific location in the PR description.@coderabbitaior@coderabbitai titleanywhere in the PR title to generate the title automatically.Documentation and Community