Feature: Implement FAQ system to Artemis
#9325
Conversation
cremertim
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
Files selected for processing (11)
- src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1 hunks)
- src/main/webapp/app/entities/faq.model.ts (1 hunks)
- src/main/webapp/app/faq/faq-update.component.scss (1 hunks)
- src/main/webapp/app/faq/faq-update.component.ts (1 hunks)
- src/main/webapp/app/faq/faq.component.html (1 hunks)
- src/main/webapp/app/faq/faq.component.ts (1 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 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 (1 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (1 hunks)
- src/test/javascript/spec/service/faq.service.spec.ts (1 hunks)
Additional context used
Path-based instructions (10)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.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/webapp/app/entities/faq.model.ts (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/app/faq/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/faq/faq.component.ts (1)
src/main/webapp/app/faq/faq.service.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/test/javascript/spec/component/faq/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/faq.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}}
Biome
src/main/webapp/app/faq/faq-update.component.ts
[error] 78-78: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 78-78: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 99-99: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 109-109: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 117-117: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 123-123: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/main/webapp/app/faq/faq.component.ts
[error] 31-31: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/main/webapp/app/faq/faq.service.ts
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 142-142: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/main/webapp/app/overview/course-faq/course-faq.component.ts
[error] 62-62: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq.component.spec.ts
[error] 113-113: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 116-116: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/service/faq.service.spec.ts
[error] 54-54: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 66-66: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 70-70: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 86-86: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 110-110: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 130-130: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (23)
src/main/webapp/app/entities/faq.model.ts (5)
1-3: LGTM!The imports are necessary and follow the correct naming convention and directory structure.
5-9: LGTM!The enum follows the PascalCase naming convention and represents the possible states of an FAQ.
11-11: LGTM!The class name follows the PascalCase naming convention and implements the
BaseEntityinterface.
12-17: LGTM!The property names follow the camelCase naming convention, and the types are correctly defined. The
courseproperty is of typeCourse, which is imported from another file.
18-18: LGTM!The property name follows the camelCase naming convention, and the type is an array of
FAQCategory, which is imported from another file.src/main/webapp/app/overview/course-faq/course-faq.component.html (3)
1-27: LGTM!The filter dropdown code looks good and follows the coding guidelines. It generates unique
idattributes for each category and uses the recommended@ifand@fordirectives.
20-20: Inline styles have been replaced with a CSS class.The
<jhi-custom-exercise-category-badge>component now uses a CSS classcategory-badgeinstead of inline styles, improving maintainability and reusability.
1-35: Overall, the code looks good and follows the coding guidelines.The file structure is well-organized, and the code is readable and maintainable. The identified issues and improvements have been addressed in the previous review comments.
src/main/webapp/app/overview/course-faq/course-faq.component.ts (7)
1-20: LGTM!The imports are well-organized and follow the Angular style guide. There are no unused imports.
21-30: LGTM!The component is properly defined with the necessary metadata and lifecycle hooks. The usage of the standalone component feature is a good practice.
32-32: Remove unused 'profileSubscription' property.The 'profileSubscription' property is declared but never assigned or used within the component. This is unnecessary and can be safely removed to clean up the code.
76-87: Add error handling for the HTTP request.The 'loadFaqs()' method lacks error handling for the HTTP request. If an error occurs while fetching FAQs, the user will not be notified, and the application might behave unexpectedly. Consider adding an error handler to manage HTTP errors and inform the user appropriately.
69-69: Rename method to reflect FAQ categories instead of exercise categories.The method 'loadCourseExerciseCategories' is misleading because it loads FAQ categories, not exercise categories. To maintain clarity and consistency, please rename the method to 'loadCourseFaqCategories'.
95-98: Use instance methods of 'faqService' instead of static methods from 'FAQService'.In the 'toggleFilters' method, you are calling 'FAQService.toggleFilter' statically. Since 'FAQService' is an injected service, it's better practice to use its instance method via 'this.faqService' to maintain consistency and allow for easier testing and potential future modifications.
100-103: Use instance methods of 'faqService' instead of static methods from 'FAQService'.In the 'applyFilters' method, you are calling 'FAQService.applyFilters' statically. Since 'FAQService' is an injected service, it's better practice to use its instance method via 'this.faqService' to maintain consistency and allow for easier testing and potential future modifications.
src/test/javascript/spec/service/faq.service.spec.ts (3)
48-61: LGTM!The test case correctly tests the
createmethod of theFAQServiceby mocking the HTTP request and asserting the response.Tools
Biome
[error] 54-54: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
99-117: LGTM!The test case correctly tests the
findAllByCourseIdmethod of theFAQServiceby mocking the HTTP request and asserting the response.Tools
Biome
[error] 110-110: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
119-137: LGTM!The test case correctly tests the
findAllCategoriesByCourseIdmethod of theFAQServiceby mocking the HTTP request and asserting the response.Tools
Biome
[error] 130-130: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/main/webapp/app/faq/faq.component.html (4)
39-39: Previous comment regarding the invalid Bootstrap classd-flex-endstill appliesThe class
d-flex-endis not a valid Bootstrap class. As noted in a previous review, consider replacing it withd-flexandjustify-content-endto achieve the desired alignment.
74-118: Previous comment regarding incorrecttracksyntax in the@forloop still appliesThe
@forloop starting at line 74 usestrack trackId(i, faq), but it should usetrackBy trackId. Please ensure that thetrackIdfunction is correctly implemented in your component.
80-81: Verify the sanitization inhtmlForMarkdownpipe to prevent XSS vulnerabilitiesDisplaying HTML content with
[innerHTML]can expose the application to XSS attacks if not properly sanitized. Please confirm that thehtmlForMarkdownpipe safely sanitizes the Markdown content.
83-84: Verify the sanitization inhtmlForMarkdownpipe to prevent XSS vulnerabilitiesSimilar to the previous comment, ensure that the
htmlForMarkdownpipe used here securely processes the content to prevent potential XSS attacks.src/main/webapp/app/faq/faq-update.component.ts (1)
151-156: Good enhancement of input validation in 'canSave()' methodThe
canSave()method now trims the inputs and checks for meaningful content before allowing the save operation. This improves input validation and aligns with best practices.
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (3)
- src/main/webapp/app/faq/faq-update.component.ts (1 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (1 hunks)
- src/test/javascript/spec/service/faq.service.spec.ts (1 hunks)
Additional context used
Path-based instructions (3)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/test/javascript/spec/component/faq/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/faq.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}}
Biome
src/main/webapp/app/faq/faq-update.component.ts
[error] 76-76: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 76-76: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 97-97: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 107-107: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 115-115: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 121-121: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq.component.spec.ts
[error] 126-126: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 129-129: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/service/faq.service.spec.ts
[error] 54-54: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 66-66: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 70-70: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 86-86: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 90-90: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 110-110: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 130-130: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (3)
src/test/javascript/spec/component/faq/faq.component.spec.ts (3)
1-20: LGTM!The import statements follow the coding guidelines, are well-organized, and include all the necessary dependencies for the test file.
21-106: LGTM!The test setup and configuration follow the best practices and properly mock the necessary dependencies. The
FAQComponentis created and initialized correctly in thebeforeEachblock.
129-129: Avoid using non-null assertions.The test case is using non-null assertions on the
idproperty of thefaq1object, which is not recommended according to the coding guidelines.Apply this diff to remove the non-null assertions:
-expect(deleteSpy).toHaveBeenCalledWith(faq1.id!); +expect(deleteSpy).toHaveBeenCalledWith(faq1.id);Tools
Biome
[error] 129-129: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- src/main/webapp/app/faq/faq-update.component.ts (1 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
- src/test/javascript/spec/component/faq/faq-update.component.spec.ts (1 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (1 hunks)
Additional context used
Path-based instructions (4)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/app/faq/faq.service.ts (1)
src/test/javascript/spec/component/faq/faq-update.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/faq/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}}
Biome
src/main/webapp/app/faq/faq-update.component.ts
[error] 74-74: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 74-74: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 95-95: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 105-105: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 113-113: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 119-119: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/main/webapp/app/faq/faq.service.ts
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 139-139: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq-update.component.spec.ts
[error] 98-98: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 127-127: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq.component.spec.ts
[error] 126-126: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 129-129: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Additional comments not posted (17)
src/test/javascript/spec/component/faq/faq.component.spec.ts (1)
112-120: Investigate the failing test.The test case is failing even though the spy is called when debugging. Please consider the following suggestions to investigate the issue:
- Check if the
ngOnInitmethod is being called correctly.- Verify if the
findAllByCourseIdmethod is returning the expected response.- Consider using
fakeAsyncandtickto wait for the asynchronous operation to complete.src/test/javascript/spec/component/faq/faq-update.component.spec.ts (5)
72-95: LGTM!The test case correctly mocks the
FAQService.createmethod and asserts that it was called with the expected arguments. The usage oftoHaveBeenCalledOnceandtoHaveBeenCalledWithexpectations aligns with the coding guidelines.
97-123: LGTM!The test case correctly mocks the
FAQService.updatemethod and asserts that it was called with the expected arguments. The usage oftoHaveBeenCalledOnceandtoHaveBeenCalledWithexpectations aligns with the coding guidelines.Tools
Biome
[error] 98-98: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
98-98: Use optional chaining or null checks instead of non-null assertions.The static analysis tool has flagged the use of a non-null assertion on line 98. While non-null assertions can be useful in certain scenarios, they should be used sparingly and with caution to avoid potential runtime errors.
Consider using optional chaining or null checks instead:
- activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } }); + activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6 } });If you need assistance in refactoring the code to remove the non-null assertion, please let me know.
Tools
Biome
[error] 98-98: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
125-140: LGTM!The test case correctly mocks the
ActivatedRouteandRouterservices and asserts that thenavigatemethod was called with the expected arguments. The usage oftoHaveBeenCalledOnceexpectation aligns with the coding guidelines.Tools
Biome
[error] 127-127: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
127-127: Use optional chaining or null checks instead of non-null assertions.The static analysis tool has flagged the use of a non-null assertion on line 127. While non-null assertions can be useful in certain scenarios, they should be used sparingly and with caution to avoid potential runtime errors.
Consider using optional chaining or null checks instead:
- activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } }); + activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } });If you need assistance in refactoring the code to remove the non-null assertion, please let me know.
Tools
Biome
[error] 127-127: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/main/webapp/app/faq/faq.service.ts (2)
21-24: Ensure Consistent Usage of API EndpointsThe API endpoints in the
create,update,find, anddeletemethods are directly specified as'api/faqs', whereas other methods usethis.resourceUrl. For consistency and maintainability, consider usingthis.resourceUrlthroughout and includingcourseIdwhere appropriate.Also applies to: 33-34, 41-41, 53-53
17-17: Remove UnusedalertServiceInjectionThe
alertServiceis injected in the constructor but is not used within the service. Removing it will clean up the code and eliminate unnecessary dependencies.Apply this diff to line 17:
constructor( protected http: HttpClient, - protected alertService: AlertService, ) {}Also, remove the unused import at line 7:
- import { AlertService } from 'app/core/util/alert.service';Likely invalid or redundant comment.
src/main/webapp/app/faq/faq-update.component.ts (9)
27-65: Proper Initialization and Data Subscription in 'ngOnInit'The
ngOnInitmethod correctly initializes the component's state and subscribes to route data using optional chaining, ensuring robustness against potential null values. The handling offaqandcourseobjects is appropriate, and the use of the nullish coalescing operator provides default initialization when necessary.
73-75: Navigation Logic in 'previousState' MethodThe
previousStatemethod effectively navigates back to the correct route. The use of thenavigationUtilServicemaintains consistency in navigation throughout the application.Tools
Biome
[error] 74-74: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 74-74: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
80-88: Efficient Save Operation in 'save' MethodThe
savemethod efficiently handles both creation and update scenarios for FAQs. Setting thefaqStatetoACCEPTEDfor new FAQs ensures that they are in the correct state upon creation.
93-98: Appropriate Subscription Handling in 'subscribeToSaveResponse'The method properly subscribes to the save response and includes error handling, which enhances the reliability of the save operation.
Tools
Biome
[error] 95-95: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
103-121: Robust Success Handling in 'onSaveSuccess'The
onSaveSuccessmethod correctly differentiates between creation and update operations. It provides user feedback via alerts and navigates to the appropriate route after saving.Tools
Biome
[error] 105-105: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 113-113: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 119-119: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
127-134: Comprehensive Error Handling in 'onSaveError'The method effectively handles different error scenarios, utilizing optional chaining to prevent runtime errors. It displays relevant alerts to inform the user of issues during the save operation.
136-139: Effective Category Management in 'updateCategories'The
updateCategoriesmethod accurately updates both thefaq.categoriesandfaqCategoriesproperties, ensuring that category changes are consistently reflected in the component state.
141-145: Category Loading in 'loadCourseFaqCategories'The method correctly loads existing FAQ categories for the course, enhancing the user experience by providing up-to-date category information.
147-152: Valid Input Verification in 'canSave'The
canSavemethod ensures that both the question title and answer are non-empty after trimming whitespace, which prevents saving invalid or incomplete FAQs.
src/main/java/de/tum/cit/aet/artemis/communication/service/FaqService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/faq/faq-update.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
Files selected for processing (2)
- src/main/webapp/app/course/manage/course-update.component.ts (3 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
Additional context used
Path-based instructions (2)
src/main/webapp/app/course/manage/course-update.component.ts (1)
src/main/webapp/app/faq/faq.service.ts (1)
Biome
src/main/webapp/app/course/manage/course-update.component.ts
[error] 74-74: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/main/webapp/app/faq/faq.service.ts
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Additional comments not posted (5)
src/main/webapp/app/course/manage/course-update.component.ts (2)
119-119: LGTM: Proper initialization offaqEnabledThe initialization of
this.faqEnabledwithcourse.faqEnabledis correct and consistent with other course property initializations.
196-196: LGTM: Proper addition offaqEnabledto the form controlsThe addition of the
faqEnabledform control to thecourseFormFormGroup is correct and consistent with other form controls. This ensures that the FAQ feature status can be properly managed within the form.src/main/webapp/app/faq/faq.service.ts (3)
17-25: Proper Implementation of thecreateMethodThe
createmethod correctly prepares the FAQ data for server submission and handles the HTTP POST request appropriately.
27-34: Accurate Update Functionality in theupdateMethodThe
updatemethod effectively updates existing FAQs by correctly formatting the data and utilizing the HTTP PUT request with the appropriate endpoint.
40-46: Efficient Retrieval infindAllByCourseIdMethodThe method efficiently retrieves all FAQs associated with a course and properly processes the server response.
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range comments (1)
src/main/webapp/app/course/manage/course-update.component.ts (1)
Line range hint
1-738: Consider refactoring the component for better maintainabilityWhile the changes for the FAQ feature are minimal and correct, the overall
CourseUpdateComponentis quite large and handles multiple responsibilities. Consider the following refactoring suggestions to improve maintainability:
- Extract form initialization logic into a separate method or service.
- Group related properties and methods (e.g., all FAQ-related code, all complaint-related code) for better organization.
- Consider breaking down the component into smaller, more focused components if possible.
- Extract common toggle logic into a reusable method or utility function.
Here's an example of how you could extract the form initialization:
private initForm() { this.courseForm = new FormGroup({ // ... existing form controls ... faqEnabled: new FormControl(this.faqEnabled, Validators.required), // ... other form controls ... }, { validators: CourseValidator }); }Then call
this.initForm()in thengOnInit()method.This refactoring will make the code more modular and easier to maintain as new features are added.
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (8)
- src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1 hunks)
- src/main/webapp/app/course/manage/course-update.component.ts (3 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq-accordion-component.html (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/FaqFactory.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/FaqIntegrationTest.java (1 hunks)
- src/test/javascript/spec/component/faq/faq-update.component.spec.ts (1 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (1 hunks)
Additional context used
Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.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/webapp/app/course/manage/course-update.component.ts (1)
src/main/webapp/app/faq/faq.service.ts (1)
src/main/webapp/app/overview/course-faq/course-faq-accordion-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/test/java/de/tum/cit/aet/artemis/FaqFactory.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/java/de/tum/cit/aet/artemis/FaqIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/javascript/spec/component/faq/faq-update.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/faq/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}}
Biome
src/main/webapp/app/faq/faq.service.ts
[error] 135-135: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq-update.component.spec.ts
[error] 95-95: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/javascript/spec/component/faq/faq.component.spec.ts
[error] 122-122: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Additional comments not posted (28)
src/main/webapp/app/overview/course-faq/course-faq-accordion-component.html (2)
1-14: LGTM: Clean and logical structureThe overall structure of the template is well-organized, with clear separation between the question and answer sections. This layout promotes readability and maintainability.
2-10: Excellent use of new Angular syntax and custom componentsThe question section is well-structured:
- The use of
markdown-previewclass for the question title is appropriate, assuming markdown content.- The implementation of the
@fordirective for category iteration aligns perfectly with the new Angular syntax guidelines.- The use of a custom component (
jhi-custom-exercise-category-badge) for categories promotes reusability and consistency.Great job on following the coding guidelines and creating a modular design!
src/test/java/de/tum/cit/aet/artemis/FaqFactory.java (3)
10-10: Add a private constructor to prevent instantiationSince
FaqFactoryis a utility class with only static methods, adding a private constructor prevents instantiation of the class.Apply this diff to add the private constructor:
public class FaqFactory { + private FaqFactory() { + // Prevent instantiation + }
22-27: Use interface type in variable declarationDeclare
categoriesasSet<String>instead ofHashSet<String>to program to the interface, enhancing flexibility and adherence to best practices.Apply this diff to adjust the variable declaration:
public static Set<String> generateFaqCategories() { - HashSet<String> categories = new HashSet<>(); + Set<String> categories = new HashSet<>();
1-28: Overall assessment: Good foundation with room for minor improvementsThe
FaqFactoryclass provides a solid foundation for creating FAQ objects in tests. The suggested improvements (adding a private constructor, enhancinggenerateFaqmethod, and using interface types) will further improve its flexibility and adherence to best practices. These changes will make the factory more robust and easier to use in various test scenarios.src/test/java/de/tum/cit/aet/artemis/FaqIntegrationTest.java (2)
52-62: LGTM: Effective role-based authorization testingThe
testAll_asTutorandtestAll_asStudentmethods effectively test authorization for different user roles by reusing thetestAllPreAuthorizemethod. This approach ensures consistent authorization checks across different user types and promotes code reuse.
1-115: Overall, well-structured and comprehensive FAQ integration testsThe
FaqIntegrationTestclass provides a solid foundation for testing the FAQ functionality. It covers various scenarios including creation, updating, and retrieval of FAQs, as well as authorization checks for different user roles. The use of@WithMockUserannotations and utility services for test data setup demonstrates good testing practices.Some minor improvements have been suggested throughout the review, including:
- Enhancing test data setup for more varied FAQ scenarios
- Adding GET operation checks in authorization tests
- Increasing assertion specificity in creation and update tests
- Improving error handling and robustness in category retrieval tests
Implementing these suggestions will further enhance the quality and reliability of the test suite. Great job on creating a comprehensive set of integration tests for the FAQ feature!
src/test/javascript/spec/component/faq/faq.component.spec.ts (3)
1-104: LGTM: Comprehensive test setup and mocking.The test setup is well-structured, using appropriate mocks and providers. It creates a realistic test environment for the FAQComponent, which aligns with best practices for Angular unit testing.
106-108: LGTM: Proper test cleanup.The use of
jest.restoreAllMocks()in the afterEach hook ensures proper cleanup after each test, maintaining test isolation. This is a good practice in Jest-based testing.
110-117: LGTM: Effective test for FAQ initialization.This test case effectively verifies the FAQ fetching behavior on component initialization. It uses standard Jest matchers and covers the essential aspects of the functionality.
src/test/javascript/spec/component/faq/faq-update.component.spec.ts (3)
1-67: LGTM: Imports and test setup are well-structured.The imports and test setup follow Angular testing best practices. The use of mock services, components, and modules enhances test isolation. The global ResizeObserver mock is a good approach to handle browser APIs in a test environment.
69-92: LGTM: Well-structured test for creating a FAQ.This test case effectively verifies the FAQ creation process:
- Uses
fakeAsyncfor handling asynchronous operations.- Correctly mocks the
FAQService.createmethod.- Verifies the method call using
toHaveBeenCalledOnce()andtoHaveBeenCalledWith(), adhering to the provided coding guidelines.
1-138: Overall, excellent test coverage and structure for FAQUpdateComponent.This test file demonstrates high-quality unit tests for the FAQUpdateComponent:
- Comprehensive test cases covering creation, editing, and navigation.
- Proper use of Angular testing utilities and Jest expectations.
- Adherence to provided coding guidelines and best practices.
The suggested improvements for non-null assertions will further enhance the code quality. Great job on maintaining a consistent and thorough testing approach!
Tools
Biome
[error] 95-95: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/main/webapp/app/faq/faq.service.ts (15)
17-25:createmethod is correctly implementedThe
createmethod appropriately handles the creation of a new FAQ by converting the FAQ object from the client format and setting its initial state toACCEPTED.
27-34:updatemethod functions as expectedThe
updatemethod correctly updates an existing FAQ by converting it from the client format before sending it to the server.
36-38:findmethod retrieves FAQs accuratelyThe
findmethod successfully retrieves an FAQ by its ID and properly converts the FAQ categories from the server response.
40-46:findAllByCourseIdmethod fetches FAQs effectivelyThis method correctly retrieves all FAQs associated with a specific course and converts the category data from the server.
48-50:deletemethod appropriately removes FAQsThe
deletemethod accurately deletes an FAQ based on its ID.
52-56:findAllCategoriesByCourseIdmethod works correctlyThe method effectively retrieves all FAQ categories for a given course.
61-67:convertFaqCategoriesFromServermethod processes data properlyThis static method correctly checks for the existence of categories and parses them from the server response.
68-75:stringifyFaqCategoriesmethod serializes categories correctlyThe method appropriately converts FAQ category objects into JSON strings for transmission to the server.
76-79:convertFaqCategoriesAsStringFromServermethod deserializes categoriesThe method accurately parses category strings back into
FAQCategoryobjects from the server data.
84-89:convertFaqCategoryArrayFromServermethod handles arrays effectivelyThis static method properly processes each FAQ in an array, ensuring their categories are parsed correctly.
95-102:parseFaqCategoriesmethod parses categories safelyThe method efficiently converts JSON strings into
FAQCategoryobjects, ensuring the FAQ categories are usable in the application.
108-112:convertFaqFromClientmethod prepares FAQs for the serverThe method successfully creates a copy of the FAQ object and serializes its categories before sending it to the server.
114-121:toggleFiltermethod manages filters correctlyThe method accurately adds or removes categories from the active filters set based on user interaction.
123-130:applyFiltersmethod filters FAQs appropriatelyThis method effectively filters the list of FAQs according to the active categories selected by the user.
132-138:hasFilteredCategorymethod checks categories accuratelyThe method correctly determines if an FAQ contains any of the categories in the active filters.
Tools
Biome
[error] 135-135: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| <div class="faq-container"> | ||
| <h4 class="markdown-preview" [innerHTML]="faq().questionAnswer | htmlForMarkdown"></h4> | ||
| </div> |
There was a problem hiding this comment.
Consider adjusting header hierarchy for better semantics
The answer section is well-implemented:
- The use of
innerHTMLbinding with thehtmlForMarkdownpipe is appropriate for rendering potentially rich content safely. - The
markdown-previewclass is consistently applied, maintaining styling coherence with the question section.
However, consider the following suggestion:
The use of <h4> for the answer while <h2> is used for the question might not follow the most semantic header hierarchy. Consider using <h3> for the answer to maintain a more logical structure:
- <h4 class="markdown-preview" [innerHTML]="faq().questionAnswer | htmlForMarkdown"></h4>
+ <h3 class="markdown-preview" [innerHTML]="faq().questionAnswer | htmlForMarkdown"></h3>This change would improve the document outline and potentially benefit accessibility.
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.
| <div class="faq-container"> | |
| <h4 class="markdown-preview" [innerHTML]="faq().questionAnswer | htmlForMarkdown"></h4> | |
| </div> | |
| <div class="faq-container"> | |
| <h3 class="markdown-preview" [innerHTML]="faq().questionAnswer | htmlForMarkdown"></h3> | |
| </div> |
| public static Faq generateFaq(Course course, FaqState state, String title, String answer) { | ||
| Faq faq = new Faq(); | ||
| faq.setCourse(course); | ||
| faq.setFaqState(state); | ||
| faq.setQuestionTitle(title); | ||
| faq.setQuestionAnswer(answer); | ||
| faq.setCategories(generateFaqCategories()); | ||
| return faq; | ||
| } |
There was a problem hiding this comment.
Enhance generateFaq method to accept customizable parameters
To increase reusability and flexibility in tests, consider modifying generateFaq to accept additional parameters for categories. This allows creating FAQs with varying content in different test scenarios.
Consider applying this diff to enhance the method:
- public static Faq generateFaq(Course course, FaqState state, String title, String answer) {
+ public static Faq generateFaq(Course course, FaqState state, String title, String answer, Set<String> categories) {
Faq faq = new Faq();
faq.setCourse(course);
faq.setFaqState(state);
faq.setQuestionTitle(title);
faq.setQuestionAnswer(answer);
- faq.setCategories(generateFaqCategories());
+ faq.setCategories(categories != null ? categories : generateFaqCategories());
return faq;
}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.
| public static Faq generateFaq(Course course, FaqState state, String title, String answer) { | |
| Faq faq = new Faq(); | |
| faq.setCourse(course); | |
| faq.setFaqState(state); | |
| faq.setQuestionTitle(title); | |
| faq.setQuestionAnswer(answer); | |
| faq.setCategories(generateFaqCategories()); | |
| return faq; | |
| } | |
| public static Faq generateFaq(Course course, FaqState state, String title, String answer, Set<String> categories) { | |
| Faq faq = new Faq(); | |
| faq.setCourse(course); | |
| faq.setFaqState(state); | |
| faq.setQuestionTitle(title); | |
| faq.setQuestionAnswer(answer); | |
| faq.setCategories(categories != null ? categories : generateFaqCategories()); | |
| return faq; | |
| } |
| @BeforeEach | ||
| void initTestCase() throws Exception { | ||
| int numberOfTutors = 2; | ||
| userUtilService.addUsers(TEST_PREFIX, 1, numberOfTutors, 0, 1); | ||
| List<Course> courses = courseUtilService.createCoursesWithExercisesAndLectures(TEST_PREFIX, true, true, numberOfTutors); | ||
| this.course1 = this.courseRepository.findByIdWithExercisesAndExerciseDetailsAndLecturesElseThrow(courses.getFirst().getId()); | ||
| this.faq = FaqFactory.generateFaq(course1, FaqState.ACCEPTED, "answer", "title"); | ||
| faqRepository.save(this.faq); | ||
| // Add users that are not in the course | ||
| userUtilService.createAndSaveUser(TEST_PREFIX + "student42"); | ||
| userUtilService.createAndSaveUser(TEST_PREFIX + "instructor42"); | ||
|
|
||
| } |
There was a problem hiding this comment.
Consider enhancing test data setup for FAQ-specific scenarios
The current setup method creates a good foundation for testing. However, to make the tests more robust and specific to FAQ functionality, consider adding more varied FAQ data. For example:
- Create multiple FAQs with different states (ACCEPTED, PROPOSED, REJECTED)
- Add FAQs with different categories
- Create FAQs for different courses
This will allow for more comprehensive testing of FAQ-related operations and edge cases.
| private void testAllPreAuthorize() throws Exception { | ||
| request.postWithResponseBody("/api/faqs", new Faq(), Faq.class, HttpStatus.FORBIDDEN); | ||
| request.putWithResponseBody("/api/faqs/" + this.faq.getId(), this.faq, Faq.class, HttpStatus.FORBIDDEN); | ||
| request.delete("/api/faqs/" + this.faq.getId(), HttpStatus.FORBIDDEN); | ||
| } |
There was a problem hiding this comment.
Add GET operation check in testAllPreAuthorize method
The testAllPreAuthorize method currently checks POST, PUT, and DELETE operations. To ensure comprehensive coverage of authorization checks, include a test for the GET operation as well.
Add the following line to the method:
request.getList("/api/courses/" + course1.getId() + "/faqs", HttpStatus.FORBIDDEN, Faq.class);This addition will verify that unauthorized users cannot retrieve FAQs, aligning with the expected behavior mentioned in the past review comments.
|
|
||
| beforeEach(() => { | ||
| faq1 = new FAQ(); | ||
| faq1.id = 1; | ||
| faq1.questionTitle = 'questionTitle'; | ||
| faq1.questionAnswer = 'questionAnswer'; | ||
| faq1.categories = [new FAQCategory('category1', '#94a11c')]; | ||
|
|
||
| faq2 = new FAQ(); | ||
| faq2.id = 2; | ||
| faq2.questionTitle = 'questionTitle'; | ||
| faq2.questionAnswer = 'questionAnswer'; | ||
| faq2.categories = [new FAQCategory('category2', '#0ab84f')]; | ||
|
|
||
| faq3 = new FAQ(); | ||
| faq3.id = 3; | ||
| faq3.questionTitle = 'questionTitle'; | ||
| faq3.questionAnswer = 'questionAnswer'; | ||
| faq3.categories = [new FAQCategory('category3', '#0ab84f')]; | ||
| TestBed.configureTestingModule({ | ||
| imports: [ArtemisTestModule, ArtemisMarkdownEditorModule, MockModule(BrowserAnimationsModule)], | ||
| declarations: [FAQComponent, MockRouterLinkDirective, MockComponent(CustomExerciseCategoryBadgeComponent)], | ||
| providers: [ | ||
| { provide: TranslateService, useClass: MockTranslateService }, | ||
| { provide: Router, useClass: MockRouter }, | ||
| { | ||
| provide: ActivatedRoute, | ||
| useValue: { | ||
| parent: { | ||
| data: of({ course: { id: 1 } }), | ||
| }, | ||
| snapshot: { | ||
| paramMap: convertToParamMap({ | ||
| courseId: '1', | ||
| }), | ||
| }, | ||
| }, | ||
| }, | ||
| MockProvider(FAQService, { | ||
| findAllByCourseId: () => { | ||
| return of( | ||
| new HttpResponse({ | ||
| body: [faq1, faq2, faq3], | ||
| status: 200, | ||
| }), | ||
| ); | ||
| }, | ||
| delete: () => { | ||
| return of(new HttpResponse({ status: 200 })); | ||
| }, | ||
| findAllCategoriesByCourseId: () => { | ||
| return of( | ||
| new HttpResponse({ | ||
| body: [], | ||
| status: 200, | ||
| }), | ||
| ); | ||
| }, | ||
| applyFilters: () => { | ||
| return [faq2, faq3]; | ||
| }, | ||
| }), | ||
| ], | ||
| }) | ||
| .compileComponents() | ||
| .then(() => { | ||
| global.ResizeObserver = jest.fn().mockImplementation((callback: ResizeObserverCallback) => { | ||
| return new MockResizeObserver(callback); | ||
| }); | ||
| faqComponentFixture = TestBed.createComponent(FAQComponent); | ||
| faqComponent = faqComponentFixture.componentInstance; | ||
|
|
||
| faqService = TestBed.inject(FAQService); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('should fetch faqs when initialized', () => { | ||
| const findAllSpy = jest.spyOn(faqService, 'findAllByCourseId'); | ||
|
|
||
| faqComponentFixture.detectChanges(); | ||
| expect(findAllSpy).toHaveBeenCalledOnce(); | ||
| expect(findAllSpy).toHaveBeenCalledWith(1); | ||
| expect(faqComponent.faqs).toHaveLength(3); | ||
| }); | ||
|
|
||
| it('should delete faq', () => { | ||
| const deleteSpy = jest.spyOn(faqService, 'delete'); | ||
| faqComponentFixture.detectChanges(); | ||
| faqComponent.deleteFaq(faq1.id!); | ||
| expect(deleteSpy).toHaveBeenCalledOnce(); | ||
| expect(deleteSpy).toHaveBeenCalledWith(faq1.id!); | ||
| expect(faqComponent.faqs).toHaveLength(2); | ||
| expect(faqComponent.faqs).not.toContain(faq1); | ||
| expect(faqComponent.faqs).toEqual(faqComponent.filteredFaqs); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider adding more test cases for comprehensive coverage.
The current test suite provides good coverage for the main functionalities of FAQComponent. To further improve the robustness of the tests, consider adding the following test cases:
- Test error handling: Verify the component's behavior when the FAQ service methods fail.
- Test empty state: Check how the component behaves when there are no FAQs.
- Test filtering: Verify that the
applyFiltersmethod works correctly with different filter criteria. - Test pagination: If the component supports pagination, add tests for different page scenarios.
These additional test cases will help ensure the component behaves correctly in various scenarios, increasing the overall reliability of the code.
Tools
Biome
[error] 122-122: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| it('should delete faq', () => { | ||
| const deleteSpy = jest.spyOn(faqService, 'delete'); | ||
| faqComponentFixture.detectChanges(); | ||
| faqComponent.deleteFaq(faq1.id!); | ||
| expect(deleteSpy).toHaveBeenCalledOnce(); | ||
| expect(deleteSpy).toHaveBeenCalledWith(faq1.id!); | ||
| expect(faqComponent.faqs).toHaveLength(2); | ||
| expect(faqComponent.faqs).not.toContain(faq1); | ||
| expect(faqComponent.faqs).toEqual(faqComponent.filteredFaqs); | ||
| }); |
There was a problem hiding this comment.
Remove non-null assertions and use type guards.
The test case effectively verifies the FAQ deletion functionality. However, the use of non-null assertions (!) on faq1.id goes against TypeScript best practices and is flagged by static analysis.
To improve type safety:
- Add a type guard to check if
faq1.idis defined before callingdeleteFaq. - Update the expectations to use the guarded
faq1.id.
Apply this diff to remove non-null assertions and add type guards:
it('should delete faq', () => {
const deleteSpy = jest.spyOn(faqService, 'delete');
faqComponentFixture.detectChanges();
- faqComponent.deleteFaq(faq1.id!);
- expect(deleteSpy).toHaveBeenCalledOnce();
- expect(deleteSpy).toHaveBeenCalledWith(faq1.id!);
+ if (faq1.id !== undefined) {
+ faqComponent.deleteFaq(faq1.id);
+ expect(deleteSpy).toHaveBeenCalledOnce();
+ expect(deleteSpy).toHaveBeenCalledWith(faq1.id);
+ } else {
+ fail('faq1.id is undefined');
+ }
expect(faqComponent.faqs).toHaveLength(2);
expect(faqComponent.faqs).not.toContain(faq1);
expect(faqComponent.faqs).toEqual(faqComponent.filteredFaqs);
});This change improves type safety while maintaining the test's functionality.
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.
| it('should delete faq', () => { | |
| const deleteSpy = jest.spyOn(faqService, 'delete'); | |
| faqComponentFixture.detectChanges(); | |
| faqComponent.deleteFaq(faq1.id!); | |
| expect(deleteSpy).toHaveBeenCalledOnce(); | |
| expect(deleteSpy).toHaveBeenCalledWith(faq1.id!); | |
| expect(faqComponent.faqs).toHaveLength(2); | |
| expect(faqComponent.faqs).not.toContain(faq1); | |
| expect(faqComponent.faqs).toEqual(faqComponent.filteredFaqs); | |
| }); | |
| it('should delete faq', () => { | |
| const deleteSpy = jest.spyOn(faqService, 'delete'); | |
| faqComponentFixture.detectChanges(); | |
| if (faq1.id !== undefined) { | |
| faqComponent.deleteFaq(faq1.id); | |
| expect(deleteSpy).toHaveBeenCalledOnce(); | |
| expect(deleteSpy).toHaveBeenCalledWith(faq1.id); | |
| } else { | |
| fail('faq1.id is undefined'); | |
| } | |
| expect(faqComponent.faqs).toHaveLength(2); | |
| expect(faqComponent.faqs).not.toContain(faq1); | |
| expect(faqComponent.faqs).toEqual(faqComponent.filteredFaqs); | |
| }); |
Tools
Biome
[error] 122-122: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| it('should edit a faq', fakeAsync(() => { | ||
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } }); | ||
|
|
||
| faqUpdateComponentFixture.detectChanges(); | ||
| faqUpdateComponent.faq = { id: 6, questionTitle: 'test1Updated' } as FAQ; | ||
|
|
||
| const updateSpy = jest.spyOn(faqService, 'update').mockReturnValue( | ||
| of<HttpResponse<FAQ>>( | ||
| new HttpResponse({ | ||
| body: { | ||
| id: 6, | ||
| title: 'test1Updated', | ||
| course: { | ||
| id: 1, | ||
| }, | ||
| } as FAQ, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| faqUpdateComponent.save(); | ||
| tick(); | ||
| faqUpdateComponentFixture.detectChanges(); | ||
|
|
||
| expect(updateSpy).toHaveBeenCalledOnce(); | ||
| expect(updateSpy).toHaveBeenCalledWith({ id: 6, questionTitle: 'test1Updated' }); | ||
| })); |
There was a problem hiding this comment.
Replace non-null assertion with optional chaining.
The test case for editing a FAQ is well-structured and follows the same pattern as the create test. However, there's a non-null assertion that should be addressed:
Replace the non-null assertion with optional chaining:
- activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } });
+ activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6 } });This change will make the code safer and align with best practices for handling potentially undefined properties.
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.
| it('should edit a faq', fakeAsync(() => { | |
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } }); | |
| faqUpdateComponentFixture.detectChanges(); | |
| faqUpdateComponent.faq = { id: 6, questionTitle: 'test1Updated' } as FAQ; | |
| const updateSpy = jest.spyOn(faqService, 'update').mockReturnValue( | |
| of<HttpResponse<FAQ>>( | |
| new HttpResponse({ | |
| body: { | |
| id: 6, | |
| title: 'test1Updated', | |
| course: { | |
| id: 1, | |
| }, | |
| } as FAQ, | |
| }), | |
| ), | |
| ); | |
| faqUpdateComponent.save(); | |
| tick(); | |
| faqUpdateComponentFixture.detectChanges(); | |
| expect(updateSpy).toHaveBeenCalledOnce(); | |
| expect(updateSpy).toHaveBeenCalledWith({ id: 6, questionTitle: 'test1Updated' }); | |
| })); | |
| it('should edit a faq', fakeAsync(() => { | |
| activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6 } }); | |
| faqUpdateComponentFixture.detectChanges(); | |
| faqUpdateComponent.faq = { id: 6, questionTitle: 'test1Updated' } as FAQ; | |
| const updateSpy = jest.spyOn(faqService, 'update').mockReturnValue( | |
| of<HttpResponse<FAQ>>( | |
| new HttpResponse({ | |
| body: { | |
| id: 6, | |
| title: 'test1Updated', | |
| course: { | |
| id: 1, | |
| }, | |
| } as FAQ, | |
| }), | |
| ), | |
| ); | |
| faqUpdateComponent.save(); | |
| tick(); | |
| faqUpdateComponentFixture.detectChanges(); | |
| expect(updateSpy).toHaveBeenCalledOnce(); | |
| expect(updateSpy).toHaveBeenCalledWith({ id: 6, questionTitle: 'test1Updated' }); | |
| })); |
Tools
Biome
[error] 95-95: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| it('should navigate to previous state', fakeAsync(() => { | ||
| activatedRoute = TestBed.inject(ActivatedRoute); | ||
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } }); | ||
|
|
||
| faqUpdateComponent.ngOnInit(); | ||
| faqUpdateComponentFixture.detectChanges(); | ||
|
|
||
| const navigateSpy = jest.spyOn(router, 'navigate'); | ||
| const previousState = jest.spyOn(faqUpdateComponent, 'previousState'); | ||
| faqUpdateComponent.previousState(); | ||
| tick(); | ||
| expect(previousState).toHaveBeenCalledOnce(); | ||
|
|
||
| const expectedPath = ['course-management', '1', 'faqs']; | ||
| expect(navigateSpy).toHaveBeenCalledWith(expectedPath); | ||
| })); |
There was a problem hiding this comment.
Replace non-null assertion and LGTM for navigation test.
The test case for navigating to the previous state is well-structured and effectively verifies the navigation functionality. However, there's one issue to address:
Replace the non-null assertion with optional chaining:
- activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } });
+ activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } });The rest of the test case looks good:
- Proper use of spies to verify method calls.
- Correct expectations using
toHaveBeenCalledOnce()andtoHaveBeenCalledWith(). - Verification of the navigation path.
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.
| it('should navigate to previous state', fakeAsync(() => { | |
| activatedRoute = TestBed.inject(ActivatedRoute); | |
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } }); | |
| faqUpdateComponent.ngOnInit(); | |
| faqUpdateComponentFixture.detectChanges(); | |
| const navigateSpy = jest.spyOn(router, 'navigate'); | |
| const previousState = jest.spyOn(faqUpdateComponent, 'previousState'); | |
| faqUpdateComponent.previousState(); | |
| tick(); | |
| expect(previousState).toHaveBeenCalledOnce(); | |
| const expectedPath = ['course-management', '1', 'faqs']; | |
| expect(navigateSpy).toHaveBeenCalledWith(expectedPath); | |
| })); | |
| it('should navigate to previous state', fakeAsync(() => { | |
| activatedRoute = TestBed.inject(ActivatedRoute); | |
| activatedRoute.parent?.data = of({ course: { id: 1 }, faq: { id: 6, questionTitle: '', course: { id: 1 } } }); | |
| faqUpdateComponent.ngOnInit(); | |
| faqUpdateComponentFixture.detectChanges(); | |
| const navigateSpy = jest.spyOn(router, 'navigate'); | |
| const previousState = jest.spyOn(faqUpdateComponent, 'previousState'); | |
| faqUpdateComponent.previousState(); | |
| tick(); | |
| expect(previousState).toHaveBeenCalledOnce(); | |
| const expectedPath = ['course-management', '1', 'faqs']; | |
| expect(navigateSpy).toHaveBeenCalledWith(expectedPath); | |
| })); |
Tools
Biome
[error] 124-124: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| /** | ||
| * GET /courses/:courseId/faqs : get all the faq categories of a course | ||
| * | ||
| * @param courseId the courseId of the course for which all faq categories should be returned | ||
| * @return the ResponseEntity with status 200 (OK) and the list of faqs in body | ||
| */ |
There was a problem hiding this comment.
Correct the Javadoc to match the actual endpoint
The Javadoc for getFaqCategoriesForCourse incorrectly specifies the endpoint as GET /courses/:courseId/faqs. It should be GET /courses/:courseId/faq-categories to accurately reflect the mapped URL and the method's functionality.
Apply this diff to fix the Javadoc:
/**
- * GET /courses/:courseId/faqs : get all the faq categories of a course
+ * GET /courses/:courseId/faq-categories : get all the FAQ categories of a course
*
* @param courseId the courseId of the course for which all FAQ categories should be returned
* @return the ResponseEntity with status 200 (OK) and the list of FAQ categories in the body
*/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.
| /** | |
| * GET /courses/:courseId/faqs : get all the faq categories of a course | |
| * | |
| * @param courseId the courseId of the course for which all faq categories should be returned | |
| * @return the ResponseEntity with status 200 (OK) and the list of faqs in body | |
| */ | |
| /** | |
| * GET /courses/:courseId/faq-categories : get all the FAQ categories of a course | |
| * | |
| * @param courseId the courseId of the course for which all FAQ categories should be returned | |
| * @return the ResponseEntity with status 200 (OK) and the list of FAQ categories in the body | |
| */ |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/main/webapp/app/course/manage/course-update.component.ts (3 hunks)
Additional context used
Path-based instructions (1)
src/main/webapp/app/course/manage/course-update.component.ts (1)
| @@ -71,6 +71,7 @@ export class CourseUpdateComponent implements OnInit { | |||
| faExclamationTriangle = faExclamationTriangle; | |||
| faPen = faPen; | |||
|
|
|||
| faqEnabled = true; //default value | |||
There was a problem hiding this comment.
LGTM! Consider adding a toggle method for consistency.
The addition of the faqEnabled property is correct and follows Angular conventions. However, for consistency with other similar properties in this component, consider adding a method to toggle this property.
Here's a suggested implementation for toggling the faqEnabled property:
changeFaqEnabled() {
this.faqEnabled = !this.faqEnabled;
this.courseForm.controls['faqEnabled'].setValue(this.faqEnabled);
}| @@ -192,6 +193,7 @@ export class CourseUpdateComponent implements OnInit { | |||
| studentCourseAnalyticsDashboardEnabled: new FormControl(this.course.studentCourseAnalyticsDashboardEnabled), | |||
| onlineCourse: new FormControl(this.course.onlineCourse), | |||
| complaintsEnabled: new FormControl(this.complaintsEnabled), | |||
| faqEnabled: new FormControl(this.faqEnabled), | |||
There was a problem hiding this comment.
LGTM! Consider adding validation for consistency.
The integration of faqEnabled into the form controls is correct. However, for consistency with other form controls, consider adding validation if necessary.
If validation is required, you could modify the form control like this:
faqEnabled: new FormControl(this.faqEnabled, Validators.required),Also, ensure that the faqEnabled property is properly handled in the save() method of the component, similar to how other properties are managed.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
It is tedious to answer the same question over and over again. To tackle this issue, this PR introduces a FAQ feature, which allows the custom creation of frequently asked questions.
Description
Server side:
I added the the basic structure of FAQ's. Also, i updated courses to have multiple FAQ's and a flag, that enables using them.
Client side:
The whole interface for creating, updating and deleting new FAQs in a Management Page (Instructor Side)
The whole interface for students to view and filter for FAQs in the Course Overview of a Course (new tab, if faqs are enabled)
Steps for Testing
Prerequisites:
Log in to Artemis as Instructor
Navigate to Course Management
Create a new Course with FAQs enabled / Enable FAQs for a existing course
Go to the FAQ Management
Create / Delete / Update FAQ's
6 Filter for FAQ's in the Management Page
Try to go to the FAQ Management as an Editor/Tutor, this should not be possible.
Log into Artemis as Student within the same course
Go to Course Overview
See previously created FAQ's
Filter the FAQ's
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
Student view (Course Overview FAQ Tab)
Instructor view (FAQ Management Page)
Creation / Update Dialog
Flag to enable FAQ
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style