Communication: Allow users to search for answer post in course wide channels
#9616
Conversation
…ssages # Conflicts: # src/main/webapp/app/overview/course-faq/course-faq.component.html # src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts
github-actions
bot
added
server
WalkthroughThe changes in this pull request enhance the functionality of the Changes
Possibly related PRs
Suggested labels
Suggested 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? 🪧 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: 1
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java (2)
73-75: Consider using Optional for null safety.The null check is good, but consider using Java's Optional for more robust null handling:
-String searchText = postContextFilter.searchText() != null ? postContextFilter.searchText() : ""; +String searchText = Optional.ofNullable(postContextFilter.searchText()).orElse("");
Line range hint
1-123: Consider implementing full-text search for better scalability.While the current implementation adds search capabilities, consider these architectural improvements for better scalability:
Implement full-text search using database-specific features (e.g., PostgreSQL's tsvector) or dedicated search engines like Elasticsearch for:
- Better performance with large datasets
- Advanced search features (fuzzy matching, relevance scoring)
- Language-aware search capabilities
Consider implementing search result caching to improve performance for frequent searches.
Add search result highlighting to improve user experience.
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
95-99: Architecture Advice: Consider implementing separate specifications for different channel types.Instead of removing predicates, consider implementing separate specifications for different channel types (public/private) to maintain the Single Responsibility Principle and improve code maintainability.
Consider refactoring the specifications into:
getPublicChannelsSpecificationgetPrivateChannelsSpecificationgetAllChannelsSpecificationThis approach would:
- Maintain clear separation of concerns
- Make the code more maintainable
- Allow for better access control
- Improve testability
Would you like me to provide a detailed implementation for these separate specifications?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java (1)
Pattern
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_importssrc/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
Pattern
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
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java (2)
94-101: LGTM! Good performance monitoring.The method maintains good performance tracking with TimeLogUtil and proper logging while incorporating the new search functionality.
121-123:⚠️ Potential issueAddress SQL performance and security concerns.
The current implementation has several potential issues:
- Using
LIKE %:searchText%with wildcards on both sides prevents the use of indexes, potentially impacting performance on large datasets.- Case sensitivity depends on database collation and might not meet user expectations.
- The query might be vulnerable to SQL wildcards in user input.
Consider these improvements:
- For better performance:
-- Use OR with LOWER() and indexes WHERE p.id IN :postIds AND ( LOWER(p.content) LIKE LOWER(CONCAT('%', :searchText, '%')) OR LOWER(a.content) LIKE LOWER(CONCAT('%', :searchText, '%')) )
- For security, escape SQL wildcards in the service layer:
searchText = searchText.replace("%", "\\%").replace("_", "\\_");Let's verify if there are any existing full-text search indexes:
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
95-99:⚠️ Potential issueSecurity and Performance Concerns: Reconsider removing channel filtering predicates.
The removal of
isCourseWidePredicateandisChannelPredicateraises several concerns:
- Security: Removing the course-wide channel check might expose messages from private channels that should remain restricted.
- Performance: Without type filtering (
isChannelPredicate), the query might fetch unnecessary conversation subtypes, impacting query performance.- Data Integrity: The specification no longer ensures we're working with proper channel types.
Restore the predicates with proper access control:
public static Specification<Post> getCourseWideChannelsSpecification(Long courseId) { return (root, query, criteriaBuilder) -> { final var conversationJoin = root.join(Post_.conversation, JoinType.LEFT); final var isInCoursePredicate = criteriaBuilder.equal(conversationJoin.get(Channel_.COURSE).get(Course_.ID), courseId); - // final var isCourseWidePredicate = criteriaBuilder.isTrue(conversationJoin.get(Channel_.IS_COURSE_WIDE)); - // make sure we only fetch channels (which are sub types of conversations) - // this avoids the creation of sub queries - // final var isChannelPredicate = criteriaBuilder.equal(conversationJoin.type(), criteriaBuilder.literal(Channel.class)); - return criteriaBuilder.and(isInCoursePredicate); + final var isCourseWidePredicate = criteriaBuilder.isTrue(conversationJoin.get(Channel_.IS_COURSE_WIDE)); + // make sure we only fetch channels (which are sub types of conversations) + // this avoids the creation of sub queries + final var isChannelPredicate = criteriaBuilder.equal(conversationJoin.type(), criteriaBuilder.literal(Channel.class)); + return criteriaBuilder.and(isInCoursePredicate, isChannelPredicate); }; }Let's verify the impact of these changes on the codebase:
...main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
client
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/i18n/en/conversation.json (1)
48-51: Consider adding tooltips for new conversation types.To enhance user experience, consider adding tooltip translations to explain the difference between group chats and direct messages.
Add these entries under the existing structure:
"groupChat": "Group Chat", "groupChats": "Group Chats", + "groupChatTooltip": "Chat with multiple selected participants", "directMessage": "Direct Message", "directMessages": "Direct Messages", + "directMessageTooltip": "Private conversation between two people"src/main/webapp/i18n/de/conversation.json (1)
48-51: LGTM with a minor suggestion for consistency.The new German translations for group chats and direct messages are grammatically correct and follow proper German capitalization rules. The translations fit well with the existing entries and support the new search functionality.
Consider adding tooltips or descriptions for these new conversation types, similar to how other UI elements have descriptive text. This would help users understand the difference between these communication channels. For example:
"conversationSelectionSideBar": { "groupChat": "Gruppenchat", "groupChats": "Gruppenchats", "directMessage": "Direktnachricht", "directMessages": "Direktnachrichten", + "groupChatDescription": "Chat zwischen dir und mehreren anderen Nutzer:innen", + "directMessageDescription": "Privater Chat zwischen dir und einer anderen Person"src/main/webapp/app/shared/metis/metis.service.ts (1)
53-53: Consider using constructor injection instead of inject function.While using the
injectfunction is valid, constructor injection is the more conventional approach in Angular services and provides better testability.- private translateService = inject(TranslateService); + constructor( + protected postService: PostService, + protected answerPostService: AnswerPostService, + protected reactionService: ReactionService, + protected accountService: AccountService, + protected exerciseService: ExerciseService, + private jhiWebsocketService: JhiWebsocketService, + private conversationService: ConversationService, + notificationService: NotificationService, + private translateService: TranslateService, + ) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (2 hunks)
- src/main/webapp/app/shared/metis/metis.service.ts (4 hunks)
- src/main/webapp/i18n/de/conversation.json (1 hunks)
- src/main/webapp/i18n/en/conversation.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/main/webapp/i18n/de/conversation.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".
🔇 Additional comments (2)
src/main/webapp/i18n/en/conversation.json (1)
48-51: LGTM! The new translation keys are well-structured.The additions follow good i18n practices with clear, consistent naming and proper singular/plural forms.
Let's verify the corresponding German translations exist:
✅ Verification successful
German translations are properly synchronized with the English ones ✓
The German translations for all new keys are present and correctly formatted:
- "groupChat" → "Gruppenchat"
- "groupChats" → "Gruppenchats"
- "directMessage" → "Direktnachricht"
- "directMessages" → "Direktnachrichten"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the German translations exist for the new keys # Expected: Should find matching entries in the German translation file rg -A 1 '"groupChat"|"directMessage"' 'src/main/webapp/i18n/de/conversation.json'Length of output: 278
src/main/webapp/app/shared/metis/metis.service.ts (1)
30-34: LGTM: Clean import organization.The new imports for conversation types and TranslateService are properly organized and follow Angular's style guide.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/i18n/en/conversation.json (1)
48-51: LGTM! Consider documenting the usage context.The new translation keys for singular forms ("groupChat" and "directMessage") complement the existing plural forms well. This distinction suggests they're used in different UI contexts (e.g., singular for labels, plural for section headers).
Consider adding a code comment in the JSON to document where these singular forms are used in the UI, as it would help other developers understand the distinction between singular and plural forms:
"conversationSelectionSideBar": { + // Singular forms used for individual chat labels "groupChat": "Group Chat", "groupChats": "Group Chats", "directMessage": "Direct Message", "directMessages": "Direct Messages",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (2 hunks)
- src/main/webapp/app/shared/metis/metis.service.ts (4 hunks)
- src/main/webapp/i18n/de/conversation.json (1 hunks)
- src/main/webapp/i18n/en/conversation.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/main/webapp/i18n/de/conversation.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".
🔇 Additional comments (3)
src/main/webapp/i18n/en/conversation.json (1)
48-51: Verify translation consistency across languages.The changes look good, but we should ensure these new keys are consistently added across all language files.
✅ Verification successful
Let me gather more information to properly verify the translation consistency.
Let me try one more verification with a different approach to check the files' content.
Translation consistency verified across all language files
The new keys "groupChat", "groupChats", "directMessage", and "directMessages" are properly added and translated in both English and German language files under the
conversationSelectionSideBarsection. The translations are consistent and complete, with no missing keys.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new keys exist in all language files # Expected: Both keys should exist in all i18n JSON files echo "Checking for new translation keys across all language files..." for lang_file in $(find src/main/webapp/i18n -name 'conversation.json'); do echo "Checking $lang_file..." jq -r '.artemisApp.conversationSelectionSideBar | select(.groupChat and .directMessage)' "$lang_file" || echo "Missing keys in $lang_file" doneLength of output: 835
Script:
#!/bin/bash # Let's check the actual content of both conversation.json files to see the translation keys echo "=== Content of English translation file ===" jq '.artemisApp.conversationSelectionSideBar' src/main/webapp/i18n/en/conversation.json echo -e "\n=== Content of German translation file ===" jq '.artemisApp.conversationSelectionSideBar' src/main/webapp/i18n/de/conversation.json # Let's also check if there are any other language files we might have missed echo -e "\n=== All available language directories ===" ls src/main/webapp/i18n/Length of output: 517
Script:
#!/bin/bash # Let's check the raw content of both files to understand the structure echo "=== Content of English translation file ===" cat src/main/webapp/i18n/en/conversation.json echo -e "\n=== Content of German translation file ===" cat src/main/webapp/i18n/de/conversation.jsonLength of output: 27309
src/main/webapp/i18n/de/conversation.json (1)
48-51: LGTM! The translations are accurate and follow guidelines.The new German translations for group chat and direct message features:
- Use the correct informal tone (dutzen)
- Maintain consistency with existing translations
- Follow common German tech terminology
- Include proper plural forms
src/main/webapp/app/shared/metis/metis.service.ts (1)
9-9: LGTM! Clean import organization.The imports are well-organized and follow Angular's style guidelines.
Also applies to: 30-34
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/main/webapp/app/shared/metis/metis.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
cremertim
changed the title
Communication: Allow users to search for answer post and private messagesCommunication: Allow users to search for answer post
cremertim
changed the title
Communication: Allow users to search for answer postCommunication: Allow users to search for answer post in course wide channels
github-actions
bot
removed
the
client
…faq-in-messages' into feature/communication/add-answers-to-post-search
github-actions
bot
added
tests
client
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (12)
src/test/javascript/spec/helpers/mocks/service/mock-metis-service.service.ts (1)
97-99: LGTM! Consider adding test coverage.
The implementation is consistent with other link generation methods in the class and correctly mocks the real service's behavior.
Consider adding a test case to verify the mock's behavior:
describe('MockMetisService', () => {
let service: MockMetisService;
beforeEach(() => {
service = new MockMetisService();
});
it('should generate correct FAQ link', () => {
const course = service.getCourse();
expect(service.getLinkForFaq()).toBe(`/courses/${course.id}/faq`);
});
});src/main/webapp/app/shared/metis/metis.util.ts (1)
Line range hint 82-111: Consider adding documentation for ReferenceType enum.
While interfaces like PostingContentPart and PatternMatch are well documented, the ReferenceType enum lacks documentation explaining the purpose of each type and when to use them.
Add JSDoc comments like this:
+/**
+ * Defines the types of content that can be referenced within the application.
+ * Used for routing, rendering, and content processing logic.
+ */
export enum ReferenceType {
+ /** Regular discussion posts */
POST = 'POST',
+ /** Course lecture content */
LECTURE = 'LECTURE',
// ... add documentation for other types
+ /** Frequently Asked Questions */
FAQ = 'FAQ',
IMAGE = 'IMAGE',
}src/main/webapp/app/entities/course.model.ts (1)
32-34: Add JSDoc comment for consistency.
The function implementation is correct and follows the established patterns. Consider adding a JSDoc comment similar to other utility functions in this file for better documentation.
+/**
+ * Checks if FAQ feature is enabled for the given course
+ * Note: Keep in sync with method in CourseRepository.java
+ */
export function isFaqEnabled(course: Course | undefined) {
return course?.faqEnabled;
}src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
Line range hint 44-57: Consider adding accessibility attributes for FAQ references.
Since we're adding FAQ reference functionality to the markdown editor, ensure that:
- The FAQ reference button has proper ARIA labels
- The FAQ selection interface is keyboard navigable
- Screen readers can properly announce FAQ selections
This will improve the user experience for users relying on assistive technologies.
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (2)
171-183: Consider enhancing scroll test coverage
While the happy path is well tested, consider adding the following improvements:
- Verify that focus() is called after scrollIntoView
- Add a test case for when the FAQ ID doesn't exist in the list
Example addition:
it('should handle non-existent faq id gracefully', () => {
const nativeElement = { id: 'faq-1', scrollIntoView: jest.fn(), focus: jest.fn() };
const elementRef = new ElementRef(nativeElement);
courseFaqComponent.faqElements = signal([elementRef]);
courseFaqComponent.scrollToFaq(999); // non-existent id
expect(nativeElement.scrollIntoView).not.toHaveBeenCalled();
expect(nativeElement.focus).not.toHaveBeenCalled();
});182-182: Consider using more specific jest matchers
According to the coding guidelines, prefer using specific jest matchers. For scrollIntoView verification, consider using toHaveBeenCalledExactlyOnceWith instead of toHaveBeenCalledWith to be more explicit about the expected number of calls.
-expect(nativeElement1.scrollIntoView).toHaveBeenCalledWith({ behavior: 'smooth', block: 'start' });
+expect(nativeElement1.scrollIntoView).toHaveBeenCalledExactlyOnceWith({ behavior: 'smooth', block: 'start' });src/test/javascript/spec/helpers/sample/metis-sample-data.ts (1)
42-44: Enhance test data diversity for FAQ samples.
The current FAQ test data uses identical values for questionTitle and questionAnswer across all samples. Consider using more diverse and realistic test data to cover different scenarios and edge cases.
Consider this improvement:
-export const metisFaq1 = { id: 1, questionTitle: 'title', questionAnswer: 'answer' };
-export const metisFaq2 = { id: 2, questionTitle: 'title', questionAnswer: 'answer' };
-export const metisFaq3 = { id: 3, questionTitle: 'title', questionAnswer: 'answer' };
+export const metisFaq1 = { id: 1, questionTitle: 'How to submit assignments?', questionAnswer: 'Click the Submit button on the assignment page.' };
+export const metisFaq2 = { id: 2, questionTitle: 'Where to find course materials?', questionAnswer: 'All materials are available in the Resources section.' };
+export const metisFaq3 = { id: 3, questionTitle: 'What is the grading policy?', questionAnswer: 'Assignments: 40%, Exams: 60%' };src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
Line range hint 1-250: Consider extracting reference parsing logic to a dedicated service.
The component is handling multiple responsibilities with complex string parsing logic. Consider these architectural improvements:
- Extract reference parsing to a dedicated service:
@Injectable()
export class ReferenceParserService {
parseReference(content: string, type: ReferenceType): ParsedReference {
// Move parsing logic here
}
}- Create type-safe interfaces for parsed references:
interface ParsedReference {
text: string;
link?: string[];
queryParams?: Params;
}This would:
- Improve testability by isolating parsing logic
- Make the component more focused on its primary responsibility
- Allow reuse of parsing logic in other components
- Make it easier to add new reference types
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
42-42: LGTM: Proper spy setup with a minor suggestion.
The spy setup follows best practices for HTTP mocking. Consider being more explicit about the return type for better type safety.
- const returnValue = of(new HttpResponse({ body: [], status: 200 }));
+ const returnValue = of(new HttpResponse<any[]>({ body: [], status: 200 }));Also applies to: 123-126
src/test/javascript/spec/service/metis/metis.service.spec.ts (1)
333-337: LGTM! The test case is well-structured and follows established patterns.
The test case follows good testing practices:
- Clear and descriptive test name
- Proper setup with course context
- Focused assertion testing a single behavior
Consider extracting the expected URL path to a constant at the top of the file, similar to how other test data is managed:
+const FAQ_PATH = '/faq';
+
describe('Metis Service', () => {
// ... existing code ...
it('should determine the router link required for referencing a faq', () => {
metisService.setCourse(course);
const link = metisService.getLinkForFaq();
- expect(link).toBe(`/courses/${metisCourse.id}/faq`);
+ expect(link).toBe(`/courses/${metisCourse.id}${FAQ_PATH}`);
});src/main/webapp/app/shared/metis/metis.service.ts (1)
497-500: Fix incorrect JSDoc description.
The JSDoc comment incorrectly mentions "exercise" in the description. It should describe the FAQ navigation purpose instead.
- * returns the router link required for navigating to the exercise referenced within a faq
+ * returns the router link required for navigating to the FAQ section of a coursesrc/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (1)
495-509: Consider using fakeAsync for consistency.
The test case is well-structured and correctly verifies FAQ reference handling. However, for consistency with other similar test cases in this file, consider wrapping it with fakeAsync.
- it('should compute parts when referencing a faq', () => {
+ it('should compute parts when referencing a faq', fakeAsync(() => {
component.content = `I want to reference [faq]faq(/courses/1/faq?faqId=45)[/faq].`;
const matches = component.getPatternMatches();
component.computePostingContentParts(matches);
expect(component.postingContentParts()).toEqual([
{
contentBeforeReference: 'I want to reference ',
linkToReference: ['/courses/1/faq'],
referenceStr: `faq`,
referenceType: ReferenceType.FAQ,
contentAfterReference: '.',
queryParams: { faqId: '45' },
} as PostingContentPart,
]);
- });
+ }));📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
- 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 (4 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-part/posting-content-part.components.ts (3 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/overview/course-faq/course-faq.component.spec.ts (5 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 (5 hunks)
- src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (9 hunks)
- src/test/javascript/spec/helpers/mocks/service/mock-metis-service.service.ts (1 hunks)
- src/test/javascript/spec/helpers/sample/metis-sample-data.ts (1 hunks)
- src/test/javascript/spec/service/metis/metis.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
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-part/posting-content-part.components.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/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/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}}
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.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/helpers/mocks/service/mock-metis-service.service.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/helpers/sample/metis-sample-data.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 (30)
src/main/webapp/app/overview/course-faq/course-faq.component.html (3)
29-29: Verify the visual impact of removing margin classes.
The removal of mb-1 mt-1 classes from the <hr> element might affect the spacing and visual hierarchy. Please ensure this doesn't negatively impact the layout and readability of the FAQ sections.
35-37: LGTM! Clean implementation of FAQ element references.
The addition of template references and unique IDs for FAQ elements is well-structured and follows Angular best practices. This enables programmatic scrolling and manipulation of individual FAQ elements.
Line range hint 8-26: LGTM! Excellent use of new Angular control flow syntax.
The implementation correctly uses the new @if and @for syntax as recommended by the coding guidelines. The proper use of tracking in @for loops is also commendable for performance optimization.
Also applies to: 31-43
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
18-18: LGTM: Import follows Angular style guidelines.
The faQuestion import is properly grouped with other FontAwesome imports and maintains alphabetical ordering.
47-47: LGTM: Icon declaration follows component patterns.
The faQuestion icon declaration maintains consistency with other icon declarations and follows proper access patterns.
src/main/webapp/app/shared/metis/metis.util.ts (1)
109-109: LGTM! Verify integration points.
The addition of FAQ to ReferenceType follows the existing enum pattern and naming conventions.
Let's verify the integration points mentioned in the AI summary:
✅ Verification successful
Integration verified successfully
The FAQ enum value is properly integrated across the codebase:
- Used in
posting-content.components.tsfor FAQ reference parsing - Icon mapping in
posting-content-part.components.tsusingfaQuestion - Test coverage in place with proper test cases including FAQ references and query parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that FAQ reference type is properly integrated across components
# Test 1: Check for FAQ handling in posting content components
rg -l "ReferenceType.FAQ" "src/main/webapp/app/shared/metis"
# Test 2: Verify test coverage
rg -l "ReferenceType.FAQ" "src/test/javascript/spec"
Length of output: 391
Script:
#!/bin/bash
# Let's examine the actual usage of ReferenceType.FAQ in these files
# to ensure proper integration
# Check posting-content component implementation
rg "ReferenceType.FAQ" "src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts" -B 2 -A 2
# Check posting-content-part component implementation
rg "ReferenceType.FAQ" "src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts" -B 2 -A 2
# Check test implementation
rg "ReferenceType.FAQ" "src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts" -B 2 -A 2
Length of output: 1416
src/main/webapp/app/entities/course.model.ts (2)
16-16: LGTM: Import statement follows Angular conventions.
The import statement for the Faq model is properly structured and necessary for the new faqs property.
106-106: Verify FAQ initialization in related components.
The property declaration looks good and follows TypeScript/Angular conventions. However, ensure that components using this property handle the undefined case appropriately.
✅ Verification successful
Let me check the initialization context by examining the component code.
Let me check how the faqs are used in the template to ensure proper null handling.
Let me check the FAQ component implementation to verify the complete handling.
Property initialization and usage are properly handled
The verification shows that the faqs property is correctly handled throughout the codebase:
- Initialization is done safely in
course-overview.component.tsthrough a service call - The FAQ component properly initializes and manages the array with null checks in the template
- Filtering, searching, and sorting operations are properly implemented with null-safe operations
- The template uses safe navigation operator (?) for null checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of course.faqs to verify proper null checking
rg "course\.faqs" --type ts
Length of output: 130
Script:
#!/bin/bash
# Get the component code with context
rg "course\.faqs" -B 5 -A 5 --type ts
Length of output: 1025
Script:
#!/bin/bash
# Search for faqs usage in the template
rg "faqs" --type html
Length of output: 617
Script:
#!/bin/bash
# Get the FAQ component implementation
cat src/main/webapp/app/overview/course-faq/course-faq.component.ts
Length of output: 5724
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (2)
24-24: LGTM: Import statements follow Angular conventions.
The new imports are properly organized and necessary for the FAQ functionality implementation.
Also applies to: 37-37
97-98: 🛠️ Refactor suggestion
Consider optimizing FAQ action initialization.
While the implementation is functionally correct, consider these improvements:
- Memoize the FAQ action to avoid unnecessary instance creation:
- const faqAction = isFaqEnabled(this.metisService.getCourse()) ? [new FaqReferenceAction(this.metisService)] : [];
+ private readonly faqAction = computed(() =>
+ isFaqEnabled(this.metisService.getCourse()) ? [new FaqReferenceAction(this.metisService)] : []
+ );
ngOnInit(): void {
// ...
- ...faqAction,
+ ...this.faqAction(),- Verify if the
FaqReferenceActionneeds cleanup inngOnDestroyto prevent memory leaks.
Also applies to: 111-111
src/main/webapp/i18n/en/metis.json (1)
49-50: LGTM! The new FAQ translation entry follows the existing pattern.
The addition of the FAQ translation key is properly structured and maintains consistency with the existing format.
Let's verify the corresponding German translation exists:
✅ Verification successful
Translation key 'faq' is properly synchronized in both English and German localization files ✅
The verification confirms that the German localization file (src/main/webapp/i18n/de/metis.json) contains the corresponding FAQ translation entry with the same value, maintaining consistency across languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the FAQ translation exists in the German localization file
# Expected: Should find "faq": "FAQ" in the German file
rg '"faq":\s*"FAQ"' 'src/main/webapp/i18n/de/metis.json'
Length of output: 85
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (3)
22-23: LGTM: Service setup follows best practices
The addition of SortService follows the established pattern of service initialization in the test suite.
Also applies to: 41-41, 111-111
71-73: LGTM: Route mock properly configured
The queryParams mock is correctly implemented using an Observable, which matches Angular's ActivatedRoute behavior.
165-169: LGTM: Sort functionality properly tested
The test correctly verifies the interaction with sortService using appropriate jest spies and expectations.
src/main/webapp/i18n/de/metis.json (1)
49-50: LGTM! The translation is correct and follows conventions.
The addition of "FAQ" as the German translation is appropriate as this is the standard term used in German interfaces.
src/test/javascript/spec/helpers/sample/metis-sample-data.ts (1)
53-53: LGTM! Clean integration of FAQs into metisCourse.
The FAQs array is properly integrated into the metisCourse object, maintaining consistency with the existing structure.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (3)
11-11: LGTM: Import changes are appropriate.
The new imports are necessary for the added functionality and follow the coding guidelines.
Also applies to: 26-26
181-204: LGTM: Comprehensive test for FAQ enabled scenario.
The test properly verifies the presence of FaqReferenceAction when FAQ is enabled.
206-226: LGTM: Comprehensive test for FAQ disabled scenario.
The test properly verifies the absence of FaqReferenceAction when FAQ is disabled.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (5)
31-32: LGTM! Import and setup changes are consistent with existing patterns.
Also applies to: 47-47
75-75: LGTM! Action initialization is consistent with existing patterns.
186-193: LGTM! Helper function follows existing patterns.
The checkFaqSuggestions function is well-implemented and consistent with other check functions.
214-216: LGTM! Test case execution is consistent with existing patterns.
267-273: LGTM! Initialization test follows existing patterns.
src/test/javascript/spec/service/metis/metis.service.spec.ts (1)
Line range hint 1-585: Verify FAQ feature test coverage.
The test suite has good coverage for the FAQ link generation, but let's verify if we need additional test cases for edge cases.
✅ Verification successful
Let me analyze the test coverage more precisely by looking at the actual FAQ-related functionality in the metis service.
Based on the results, I can now provide a final response regarding the FAQ feature test coverage.
FAQ test coverage is adequate and verified
The implementation of getLinkForFaq() in the metis service is straightforward - it simply returns a URL string using the course ID. The current test case thoroughly verifies this functionality by:
- Setting up the course context
- Calling the method
- Verifying the exact expected URL format
Since the method has no edge cases (it's a simple string template) and the test verifies both the method call and the expected output format, no additional test coverage is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FAQ-related functionality in the codebase
echo "Searching for FAQ-related methods and tests..."
rg -l "getFaq|faq" --type ts
# Search for additional test cases related to FAQ
echo "Searching for FAQ-related test cases..."
ast-grep --pattern 'it($_, function() { $$$ faq $$$ })'
Length of output: 2590
Script:
#!/bin/bash
# Look for the FAQ-related method implementation in metis service
echo "Searching for FAQ method implementation in metis service..."
rg -A 5 "getLinkForFaq" src/main/webapp/app/shared/metis/metis.service.ts
# Look for all FAQ-related test cases in metis service spec
echo "Searching for FAQ test cases in metis service spec..."
rg -B 2 -A 5 "getLinkForFaq|faq" src/test/javascript/spec/service/metis/metis.service.spec.ts
Length of output: 1053
src/main/webapp/app/shared/metis/metis.service.ts (1)
501-503: LGTM: Clean implementation of FAQ navigation link.
The implementation follows the established patterns in the service and correctly uses the course context for URL generation.
src/main/webapp/app/overview/course-overview.component.ts (2)
71-72: LGTM: Clean imports and modern dependency injection!
The imports are well-organized, and the use of the inject function for dependency injection follows Angular's modern best practices.
Also applies to: 196-196
723-723: LGTM: Clean integration with existing code!
The setFaqs method is well-integrated:
- Called at the right time after course is set
- Respects the FAQ feature flag
- Follows the established patterns for sidebar items
Also applies to: 723-723
src/main/webapp/app/shared/monaco-editor/model/actions/communication/faq-reference.action.ts (2)
64-67: LGTM!
The dispose method correctly disposes of the completion provider, preventing potential memory leaks.
69-71: Verify the correctness of the opening identifier.
The getOpeningIdentifier method returns '[faq]'. Ensure that this identifier aligns with the expected syntax in the editor's parsing logic and functions correctly within the application.
| case ReferenceType.FAQ: | ||
| return faQuestion; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting icon mappings to improve maintainability.
While the FAQ case is correctly implemented, the referenceIcon method is growing with many cases. Consider extracting the icon mappings to a constant object for better maintainability.
Here's a suggested refactoring:
private static readonly REFERENCE_TYPE_ICONS: Record<ReferenceType, IconProp> = {
[ReferenceType.POST]: faMessage,
[ReferenceType.LECTURE]: faChalkboardUser,
[ReferenceType.PROGRAMMING]: faKeyboard,
[ReferenceType.MODELING]: faProjectDiagram,
[ReferenceType.QUIZ]: faCheckDouble,
[ReferenceType.TEXT]: faFont,
[ReferenceType.FILE_UPLOAD]: faFileUpload,
[ReferenceType.SLIDE]: faFile,
[ReferenceType.FAQ]: faQuestion
};
referenceIcon(reference: ReferenceType): IconProp {
return PostingContentPartComponent.REFERENCE_TYPE_ICONS[reference] ?? faPaperclip;
}This approach:
- Reduces method complexity
- Makes it easier to add new mappings
- Improves type safety
- Follows Angular's style guidelines
| @@ -206,9 +212,10 @@ export class PostingContentComponent implements OnInit, OnChanges, OnDestroy { | |||
| // Group 8: reference pattern for Lecture Units | |||
| // Group 9: reference pattern for Users | |||
| // Group 10: pattern for embedded images | |||
| // Group 11: reference pattern for FAQ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving regex maintainability and validation.
The regex pattern could benefit from these improvements:
- Split the long regex into named constants for better maintainability:
private static readonly PATTERNS = {
POST: '#\\d+',
FAQ: '\\[faq].*?\\[\\/faq]',
// ... other patterns
};
private static readonly COMBINED_PATTERN = new RegExp(
Object.entries(PATTERNS)
.map(([key, pattern]) => `(?<${key}>${pattern})`)
.join('|'),
'g'
);- Make the FAQ pattern more specific to validate the expected format:
FAQ: '\\[faq]\\s*([^\\[\\]]+?)\\s*\\(\\s*\\/courses\\/[^\\s]+\\?faqId=\\d+\\s*\\)\\s*\\[\\/faq]'Also applies to: 218-218
| } else if (ReferenceType.FAQ === referenceType) { | ||
| referenceStr = this.content.substring(this.content.indexOf(']', patternMatch.startIndex)! + 1, this.content.indexOf('(/courses', patternMatch.startIndex)!); | ||
| linkToReference = [ | ||
| this.content.substring(this.content.indexOf('(/courses', patternMatch.startIndex)! + 1, this.content.indexOf('?faqId', patternMatch.startIndex)), | ||
| ]; | ||
| queryParams = { faqId: this.content.substring(this.content.indexOf('=') + 1, this.content.indexOf(')')) } as Params; |
There was a problem hiding this comment.
Add null checks and improve string parsing robustness.
The FAQ reference parsing logic could be more resilient to malformed input. Consider these improvements:
} else if (ReferenceType.FAQ === referenceType) {
- referenceStr = this.content.substring(this.content.indexOf(']', patternMatch.startIndex)! + 1, this.content.indexOf('(/courses', patternMatch.startIndex)!);
- linkToReference = [
- this.content.substring(this.content.indexOf('(/courses', patternMatch.startIndex)! + 1, this.content.indexOf('?faqId', patternMatch.startIndex)),
- ];
- queryParams = { faqId: this.content.substring(this.content.indexOf('=') + 1, this.content.indexOf(')')) } as Params;
+ const closingBracketIndex = this.content.indexOf(']', patternMatch.startIndex);
+ const coursePathIndex = this.content.indexOf('(/courses', patternMatch.startIndex);
+ const faqIdIndex = this.content.indexOf('?faqId', patternMatch.startIndex);
+ const equalsIndex = this.content.indexOf('=', faqIdIndex);
+ const closingParenIndex = this.content.indexOf(')', equalsIndex);
+
+ if (closingBracketIndex === -1 || coursePathIndex === -1 || faqIdIndex === -1 ||
+ equalsIndex === -1 || closingParenIndex === -1) {
+ console.warn('Malformed FAQ reference detected');
+ return;
+ }
+
+ referenceStr = this.content.substring(closingBracketIndex + 1, coursePathIndex);
+ linkToReference = [this.content.substring(coursePathIndex + 1, faqIdIndex)];
+ queryParams = { faqId: this.content.substring(equalsIndex + 1, closingParenIndex) } as Params;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (ReferenceType.FAQ === referenceType) { | |
| referenceStr = this.content.substring(this.content.indexOf(']', patternMatch.startIndex)! + 1, this.content.indexOf('(/courses', patternMatch.startIndex)!); | |
| linkToReference = [ | |
| this.content.substring(this.content.indexOf('(/courses', patternMatch.startIndex)! + 1, this.content.indexOf('?faqId', patternMatch.startIndex)), | |
| ]; | |
| queryParams = { faqId: this.content.substring(this.content.indexOf('=') + 1, this.content.indexOf(')')) } as Params; | |
| } else if (ReferenceType.FAQ === referenceType) { | |
| const closingBracketIndex = this.content.indexOf(']', patternMatch.startIndex); | |
| const coursePathIndex = this.content.indexOf('(/courses', patternMatch.startIndex); | |
| const faqIdIndex = this.content.indexOf('?faqId', patternMatch.startIndex); | |
| const equalsIndex = this.content.indexOf('=', faqIdIndex); | |
| const closingParenIndex = this.content.indexOf(')', equalsIndex); | |
| if (closingBracketIndex === -1 || coursePathIndex === -1 || faqIdIndex === -1 || | |
| equalsIndex === -1 || closingParenIndex === -1) { | |
| console.warn('Malformed FAQ reference detected'); | |
| return; | |
| } | |
| referenceStr = this.content.substring(closingBracketIndex + 1, coursePathIndex); | |
| linkToReference = [this.content.substring(coursePathIndex + 1, faqIdIndex)]; | |
| queryParams = { faqId: this.content.substring(equalsIndex + 1, closingParenIndex) } as Params; |
| it('should have set the correct default commands on init if faq is enabled', () => { | ||
| jest.spyOn(CourseModel, 'isFaqEnabled').mockReturnValueOnce(true); | ||
| component.ngOnInit(); | ||
|
|
||
| expect(component.defaultActions).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.any(BoldAction), | ||
| expect.any(ItalicAction), | ||
| expect.any(UnderlineAction), | ||
| expect.any(QuoteAction), | ||
| expect.any(CodeAction), | ||
| expect.any(CodeBlockAction), | ||
| expect.any(EmojiAction), | ||
| expect.any(UrlAction), | ||
| expect.any(AttachmentAction), | ||
| expect.any(UserMentionAction), | ||
| expect.any(ChannelReferenceAction), | ||
| expect.any(ExerciseReferenceAction), | ||
| expect.any(FaqReferenceAction), | ||
| ]), | ||
| ); | ||
|
|
||
| expect(component.lectureAttachmentReferenceAction).toEqual(new LectureAttachmentReferenceAction(metisService, lectureService)); | ||
| }); | ||
|
|
||
| it('should have set the correct default commands on init if faq is disabled', () => { | ||
| jest.spyOn(CourseModel, 'isFaqEnabled').mockReturnValueOnce(false); | ||
| component.ngOnInit(); | ||
|
|
||
| expect(component.defaultActions).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.any(BoldAction), | ||
| expect.any(ItalicAction), | ||
| expect.any(UnderlineAction), | ||
| expect.any(QuoteAction), | ||
| expect.any(CodeAction), | ||
| expect.any(CodeBlockAction), | ||
| expect.any(EmojiAction), | ||
| expect.any(UrlAction), | ||
| expect.any(AttachmentAction), | ||
| expect.any(ExerciseReferenceAction), | ||
| ]), | ||
| ); | ||
|
|
||
| expect(component.lectureAttachmentReferenceAction).toEqual(new LectureAttachmentReferenceAction(metisService, lectureService)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing duplication and adding integration test.
- The expectation arrays in both tests contain significant duplication. Consider extracting common actions to a shared constant.
- Add a test to verify the actual integration with FaqReferenceAction.
// Extract common actions
const BASE_ACTIONS = [
expect.any(BoldAction),
expect.any(ItalicAction),
expect.any(UnderlineAction),
expect.any(QuoteAction),
expect.any(CodeAction),
expect.any(CodeBlockAction),
expect.any(EmojiAction),
expect.any(UrlAction),
expect.any(AttachmentAction),
];
const COMMUNICATION_ACTIONS = [
expect.any(UserMentionAction),
expect.any(ChannelReferenceAction),
];
// Add integration test
it('should properly integrate FaqReferenceAction when enabled', () => {
jest.spyOn(CourseModel, 'isFaqEnabled').mockReturnValueOnce(true);
component.ngOnInit();
const faqAction = component.defaultActions.find(action => action instanceof FaqReferenceAction);
expect(faqAction).toBeTruthy();
// Test the action's behavior
const mockEditor = { /* ... */ };
faqAction.run(mockEditor);
// Add expectations for the action's behavior
});| @@ -92,11 +96,13 @@ describe('MonacoEditorCommunicationActionIntegration', () => { | |||
| { actionId: ChannelReferenceAction.ID, defaultInsertText: '#', triggerCharacter: '#' }, | |||
| { actionId: UserMentionAction.ID, defaultInsertText: '@', triggerCharacter: '@' }, | |||
| { actionId: ExerciseReferenceAction.ID, defaultInsertText: '/exercise', triggerCharacter: '/' }, | |||
| { actionId: FaqReferenceAction.ID, defaultInsertText: '/faq', triggerCharacter: '/' }, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a type alias for better maintainability.
The union type for the action variable is growing longer. Consider extracting it to a type alias.
type SupportedAction = ChannelReferenceAction | UserMentionAction | ExerciseReferenceAction | FaqReferenceAction;
let action: SupportedAction;Also applies to: 101-101, 105-105, 115-115
| register(editor: TextEditor, translateService: TranslateService): void { | ||
| super.register(editor, translateService); | ||
| const faqs = this.metisService.getCourse().faqs ?? []; | ||
| this.setValues( | ||
| faqs.map((faq) => ({ | ||
| id: faq.id!.toString(), | ||
| value: faq.questionTitle!, | ||
| type: 'faq', | ||
| })), | ||
| ); | ||
|
|
||
| this.disposableCompletionProvider = this.registerCompletionProviderForCurrentModel<ValueItem>( | ||
| editor, | ||
| () => Promise.resolve(this.getValues()), | ||
| (item: ValueItem, range: TextEditorRange) => | ||
| new TextEditorCompletionItem( | ||
| `/faq ${item.value}`, | ||
| item.type, | ||
| `[${item.type}]${item.value}(${this.metisService.getLinkForFaq()}?faqId=${item.id})[/${item.type}]`, | ||
| TextEditorCompletionItemKind.Default, | ||
| range, | ||
| ), | ||
| '/', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating FAQs dynamically to reflect changes.
Currently, the FAQs are fetched once during the registration of the action. If FAQs are added or modified after the editor initializes, these changes won't appear in the completion suggestions. To ensure users always see the most recent FAQs, consider implementing a mechanism to refresh the FAQ list when updates occur.
| `/faq ${item.value}`, | ||
| item.type, | ||
| `[${item.type}]${item.value}(${this.metisService.getLinkForFaq()}?faqId=${item.id})[/${item.type}]`, |
There was a problem hiding this comment.
Localize user-facing strings in completion items.
The strings used in the TextEditorCompletionItem constructor, such as /faq ${item.value} and [${item.type}]${item.value}(...)[/${item.type}], are displayed to the user. To adhere to localization guidelines (localize:true), please use the translateService to localize these user-facing texts.
| @@ -1,6 +1,6 @@ | |||
| import { Component, OnDestroy, OnInit, ViewEncapsulation, inject } from '@angular/core'; | |||
| import { Component, ElementRef, OnDestroy, OnInit, ViewEncapsulation, effect, inject, viewChildren } from '@angular/core'; | |||
There was a problem hiding this comment.
Correct the casing of ViewChildren in the import statement
The import statement incorrectly uses viewChildren instead of ViewChildren. In Angular, decorators and class names should follow PascalCase naming convention.
Apply this diff to fix the import:
-import { Component, ElementRef, OnDestroy, OnInit, ViewEncapsulation, effect, inject, viewChildren } from '@angular/core';
+import { Component, ElementRef, OnDestroy, OnInit, ViewEncapsulation, effect, inject, ViewChildren } from '@angular/core';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Component, ElementRef, OnDestroy, OnInit, ViewEncapsulation, effect, inject, viewChildren } from '@angular/core'; | |
| import { Component, ElementRef, OnDestroy, OnInit, ViewEncapsulation, effect, inject, ViewChildren } from '@angular/core'; |
| } | ||
|
|
||
| scrollToFaq(faqId: number): void { | ||
| const faqElement = this.faqElements().find((faq) => faq.nativeElement.id === 'faq-' + String(faqId)); |
There was a problem hiding this comment.
Access faqElements correctly and remove parentheses
The faqElements is a property, not a method. Remove the parentheses when accessing it. Additionally, use toArray() to convert the QueryList to an array before using find.
Apply this diff to fix the usage:
-const faqElement = this.faqElements().find((faq) => faq.nativeElement.id === 'faq-' + String(faqId));
+const faqElement = this.faqElements.toArray().find((faq) => faq.nativeElement.id === 'faq-' + String(faqId));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const faqElement = this.faqElements().find((faq) => faq.nativeElement.id === 'faq-' + String(faqId)); | |
| const faqElement = this.faqElements.toArray().find((faq) => faq.nativeElement.id === 'faq-' + String(faqId)); |
| @@ -27,10 +29,12 @@ | |||
| imports: [ArtemisSharedComponentModule, ArtemisSharedModule, CourseFaqAccordionComponent, CustomExerciseCategoryBadgeComponent, SearchFilterComponent, ArtemisMarkdownModule], | |||
| }) | |||
| export class CourseFaqComponent implements OnInit, OnDestroy { | |||
| faqElements = viewChildren<ElementRef>('faqElement'); | |||
There was a problem hiding this comment.
Use the @ViewChildren decorator for faqElements
Currently, faqElements is assigned using viewChildren, but it should be declared using the @ViewChildren decorator to properly query child elements.
Apply this diff to correct the property declaration:
-faqElements = viewChildren<ElementRef>('faqElement');
+@ViewChildren('faqElement') faqElements: QueryList<ElementRef>;Also, ensure you import QueryList from @angular/core:
+import { ..., QueryList } from '@angular/core';Committable suggestion was skipped due to low confidence.
cremertim
deleted the
feature/communication/add-answers-to-post-search
branch
Checklist
General
Server
Motivation and Context
Currently, the search only filters the content of the original post and also excludes private conversations a user migh have
Description
I removed the private channel check and added answer posts, where the original post does not match the content but an answer post matches, to the search result
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests