Communication Allow editors to propose FAQ
#9457
Conversation
github-actions
bot
added
tests
server
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing the FAQ management functionality. Key updates include the addition of methods in 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: 26
🧹 Outside diff range comments (6)
src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
Line range hint
1-98: Overall assessment: Changes improve FAQ management functionality.The modifications in this file successfully implement the filtering of accepted FAQs, aligning with the PR objectives. The code maintains good practices in terms of naming conventions and import statements. However, ensure that these changes are consistently applied across related components and services in the application.
Consider the following to enhance the overall architecture:
- Implement a centralized state management solution (e.g., NgRx) for better control over FAQ states across the application.
- Create a separate FAQ filtering service to encapsulate the filtering logic, promoting reusability and easier maintenance.
src/main/webapp/app/faq/faq.service.ts (1)
Line range hint
1-155: Overall good adherence to coding guidelines. Minor improvements suggested.The file generally follows the provided coding guidelines well. Here are some observations and suggestions:
- Consistent use of camelCase for method names and properties.
- Proper use of PascalCase for types.
- Adherence to the 'no_priv_prefix' guideline.
- Consistent use of single quotes for strings.
- Good use of TypeScript types throughout the file.
Consider the following minor improvements:
- Add JSDoc comments to public methods for better documentation.
- Consider using a more specific error type in the
deletemethod instead ofany.- The
toggleFilterandapplyFiltersmethods seem to be more suited for a component or a dedicated filter service. Consider moving them if they're not directly related to FAQ data fetching or manipulation.Example of adding JSDoc:
/** * Retrieves all FAQs for a given course and state. * @param courseId - The ID of the course * @param faqState - The state of the FAQs to retrieve * @returns An Observable of FAQs matching the criteria */ findAllByCourseIdAndState(courseId: number, faqState: FaqState): Observable<EntityArrayResponseType> { // ... (existing implementation) }src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)
Line range hint
65-72: LGTM: FaqService mock updated correctly.The mock provider for FaqService has been updated to use
findAllByCourseIdAndStateinstead offindAllByCourseId, which aligns with the changes in the actual service.Consider adding a comment to explain the expected parameters for
findAllByCourseIdAndState, as it's not immediately clear from the mock implementation. For example:// Mock findAllByCourseIdAndState to return all FAQs regardless of courseId and state findAllByCourseIdAndState: (courseId: number, state: FaqState) => { return of( new HttpResponse({ body: [faq1, faq2, faq3], status: 200, }), ); },src/test/javascript/spec/component/faq/faq-update.component.spec.ts (1)
Line range hint
1-195: Consider adding test cases for the full lifecycle of proposed FAQs.While the existing test cases have been updated to reflect the new "proposed" state for FAQs, there might be a need for additional test cases to cover the full lifecycle of a proposed FAQ.
Consider adding the following test cases:
- Verify that a newly created FAQ is not visible in the course overview.
- Test the process of an instructor accepting a proposed FAQ.
- Ensure that an accepted FAQ becomes visible in the course overview.
- Test the behavior when an instructor rejects a proposed FAQ.
These additional test cases would help ensure that the new functionality is thoroughly covered and behaves as expected throughout the FAQ lifecycle.
src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (1)
Line range hint
1-184: Overall, the test coverage is comprehensive and aligns with the PR objectives.The
FaqIntegrationTestclass provides good coverage for FAQ management operations, including the new functionality for proposing FAQs. It tests various scenarios and user roles, adhering to the coding guidelines for integration tests.To improve consistency, consider:
Standardizing the test method naming convention. Some methods use camelCase (e.g.,
testgetFaqByCourseId), while others use snake_case (e.g.,updateFaq_IdsDoNotMatch_shouldNotUpdateFaq). Choose one convention and apply it consistently across all test methods.Adding a test for the tutor role proposing an FAQ, as this is a key feature mentioned in the PR objectives.
Example:
@Test @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA") void testTutorProposeFaq() throws Exception { Faq newFaq = FaqFactory.generateFaq(course1, FaqState.PROPOSED, "Tutor Question", "Tutor Answer"); Faq returnedFaq = request.postWithResponseBody("/api/courses/" + course1.getId() + "/faqs", newFaq, Faq.class, HttpStatus.CREATED); assertThat(returnedFaq).isNotNull(); assertThat(returnedFaq.getFaqState()).isEqualTo(FaqState.PROPOSED); }These improvements will enhance the overall quality and consistency of the test class.
src/test/javascript/spec/service/faq.service.spec.ts (1)
Line range hint
1-256: Consider enhancing test specificity and maintainability.While the test file is well-structured, there are a couple of improvements that could be made:
Use more specific expectations:
Instead of usingtoEqualfor comparing entire objects, consider using more specific matchers for individual properties. This can make test failures more informative.Use constants for repeated values:
Values likecourseIdand color codes are repeated throughout the tests. Consider defining these as constants at the top of the describe block for better maintainability.Here's an example of how you could implement these suggestions:
describe('Faq Service', () => { // ... existing setup ... const COURSE_ID = 1; const CATEGORY_COLOR = '#6ae8ac'; const CATEGORY_NAME = 'category1'; // ... existing tests ... it('should find faqs by courseId and status', () => { const category = { color: CATEGORY_COLOR, category: CATEGORY_NAME, } as FaqCategory; const returnedFromService = [{ ...elemDefault, categories: [JSON.stringify(category)] }]; const expected = [{ ...elemDefault, categories: [new FaqCategory(CATEGORY_NAME, CATEGORY_COLOR)] }]; service .findAllByCourseIdAndState(COURSE_ID, FaqState.ACCEPTED) .pipe(take(1)) .subscribe((resp) => { expect(resp.body).toBeDefined(); expect(resp.body?.length).toBe(1); expect(resp.body?.[0].categories?.[0]).toEqual(expect.objectContaining({ category: CATEGORY_NAME, color: CATEGORY_COLOR })); }); const req = httpMock.expectOne({ url: `api/courses/${COURSE_ID}/faq-state/${FaqState.ACCEPTED}`, method: 'GET', }); req.flush(returnedFromService); }); // ... remaining tests ... });These changes would make the tests more robust and easier to maintain.
🧰 Tools
🪛 Biome
[error] 148-148: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (17)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (5 hunks)
- src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.html (1 hunks)
- src/main/webapp/app/course/manage/overview/course-management-card.component.html (1 hunks)
- src/main/webapp/app/entities/faq.model.ts (1 hunks)
- src/main/webapp/app/faq/faq-update.component.ts (5 hunks)
- src/main/webapp/app/faq/faq.component.html (3 hunks)
- src/main/webapp/app/faq/faq.component.ts (6 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (2 hunks)
- src/main/webapp/i18n/de/faq.json (2 hunks)
- src/main/webapp/i18n/en/faq.json (2 hunks)
- src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (4 hunks)
- src/test/javascript/spec/component/faq/faq-update.component.spec.ts (3 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (4 hunks)
- src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (3 hunks)
- src/test/javascript/spec/service/faq.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.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/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-management-tab-bar/course-management-tab-bar.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/course/manage/overview/course-management-card.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/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.ts (1)
src/main/webapp/i18n/de/faq.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/java/de/tum/cit/aet/artemis/communication/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}}src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/service/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] 35-35: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/main/webapp/app/faq/faq.component.ts
[error] 36-36: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 37-37: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/test/javascript/spec/service/faq.service.spec.ts
[error] 148-148: 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 (28)
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (2)
15-15: LGTM: Import statement added correctly.The import for
FaqStateis necessary for the new method parameter and follows the coding guideline of avoiding star imports.
34-34: LGTM: New method added correctly. Consider verifying query performance.The new method
findAllByCourseIdAndFaqStatefollows Spring Data JPA naming conventions and is consistent with the existing code style. It adheres to the single responsibility principle by providing a specific query method.To ensure optimal performance, please verify the generated query for this method:
If no custom query or index is found, consider adding an index to optimize this query, especially if it's expected to be called frequently.
src/main/webapp/i18n/en/faq.json (4)
7-9: LGTM: New labels for FAQ proposal workflow.The new labels "proposeLabel", "accept", and "reject" are clear, concise, and align well with the PR objectives. They accurately represent the new functionality allowing tutors to propose FAQs and instructors to review them.
16-17: LGTM: New messages for accepting and rejecting FAQs.The new messages for accepting and rejecting FAQs are well-structured and consistent with the existing messages. They include the FAQ ID for improved traceability and align perfectly with the new functionality described in the PR objectives.
26-27: LGTM: Addition of "state" label for FAQ table.The new "state" label in the table object is a good addition. It's consistent with the new FAQ proposal workflow and follows the existing structure and naming conventions.
Line range hint
1-32: Overall assessment: Excellent additions for FAQ proposal workflow.The changes to this translation file comprehensively cover the new FAQ proposal functionality described in the PR objectives. The new labels and messages are clear, consistent in style with existing content, and should effectively communicate the new workflow to users.
The additions include:
- Labels for proposing, accepting, and rejecting FAQs
- Updated messages for FAQ creation, update, and deletion (now including FAQ IDs)
- New messages for accepting and rejecting FAQs
- A new "state" label for the FAQ table
These changes align perfectly with the goal of allowing tutors to propose FAQs and instructors to review them. The translations are well-structured and should integrate seamlessly with the existing UI.
src/main/webapp/i18n/de/faq.json (2)
26-26: LGTM: Correct translation for "categories"The translation "Kategorien" for "categories" is correct and appropriate.
27-27: LGTM: Correct translation for "state"The translation "Status" for "state" is correct and appropriate in this context.
src/main/webapp/app/overview/course-faq/course-faq.component.ts (2)
11-11: LGTM: Import statement updated correctly.The import statement has been properly updated to include
FaqStatealongsideFaq. This change is consistent with its usage in theloadFaqsmethod and follows the coding guidelines for TypeScript files.
70-70: LGTM: Method call updated to filter accepted FAQs.The
loadFaqsmethod has been correctly updated to usefindAllByCourseIdAndStatewithFaqState.ACCEPTEDas a parameter. This change aligns with the PR objectives to show only accepted FAQs to students.To ensure consistency across the application, please verify that related components or services handling FAQ data are updated accordingly. Run the following script to check for any inconsistencies:
✅ Verification successful
LGTM: Verification successful for filtering accepted FAQs.
The update to the
loadFaqsmethod incourse-faq.component.tscorrectly usesfindAllByCourseIdAndStatewithFaqState.ACCEPTED. This change is appropriately scoped to FAQ-related components, ensuring no unintended impacts on other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential inconsistencies in FAQ-related components and services # Test: Search for occurrences of findAllByCourseId in other files echo "Checking for occurrences of findAllByCourseId:" rg --type typescript 'findAllByCourseId\(' src/main/webapp # Test: Search for occurrences of FaqState to ensure it's used consistently echo "Checking for occurrences of FaqState:" rg --type typescript 'FaqState\.' src/main/webappLength of output: 370
Script:
#!/bin/bash # Description: Check for potential inconsistencies in FAQ-related components and services # Test: Search for occurrences of findAllByCourseId in TypeScript files echo "Checking for occurrences of findAllByCourseId:" rg --glob "*.ts" 'findAllByCourseId\(' src/main/webapp # Test: Search for occurrences of FaqState to ensure it's used consistently in TypeScript files echo "Checking for occurrences of FaqState:" rg --glob "*.ts" 'FaqState\.' src/main/webappLength of output: 1884
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (2)
19-19: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
FaqState. This change aligns with the new usage ofFaqStatein the test file.
Line range hint
1-141: Overall assessment: Changes are well-implemented and maintain test coverage.The updates to this test file accurately reflect the changes made to the
CourseFaqComponentandFaqService. The introduction ofFaqStateand the updated method signature for fetching FAQs have been properly incorporated into the test suite. The changes maintain good test coverage and adhere to the provided coding guidelines.Key points:
- Import statements have been updated correctly.
- The FaqService mock has been modified to use the new method signature.
- Test cases have been updated to reflect the new functionality and parameters.
The suggested minor improvements (adding comments for clarity and enhancing test robustness) would further increase the quality of the test suite, but the current implementation is acceptable.
src/main/webapp/app/faq/faq-update.component.ts (3)
19-19: LGTM: Import statement added correctly.The import for
AccountServiceis properly placed and follows the coding guidelines. It's necessary for the new functionality in the component.
49-49: LGTM: AccountService injection added correctly.The
AccountServiceis properly injected using theinjectfunction, following the pattern used for other services in the component. This adheres to modern Angular dependency injection practices and the coding guidelines.
Line range hint
1-164: Overall approval with minor suggestions for improvement.The changes to the
FaqUpdateComponentsuccessfully implement the new functionality for setting FAQ state based on user role, aligning with the PR objectives. The code adheres to the Angular style guide and coding guidelines, with only minor improvements suggested:
- Correct the spelling of "at least" in the new property name.
- Remove the unnecessary type annotation for the new property.
- Consider using a more descriptive variable name for clarity.
These minor adjustments will enhance code readability and consistency. Great job on implementing this feature!
src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.html (1)
Line range hint
75-80: LGTM! Change aligns with PR objectives.The modification to broaden access to the FAQ tab from instructors to editors aligns well with the PR objective of allowing tutors to propose FAQs. The use of
@ifadheres to the coding guidelines for Angular templates.src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2)
18-18: LGTM: Import statement added correctly.The import for
FaqDTOis necessary for the new functionality and follows the coding guidelines.
35-36: LGTM: Field declaration added correctly.The
faqDTOfield is appropriately declared as private and will be used in the new test methods.src/test/javascript/spec/service/faq.service.spec.ts (1)
137-155: LGTM! The new test case is well-structured and consistent.The new test case for
findAllByCourseIdAndStatefollows the existing patterns in the file and correctly tests the new functionality. It properly sets up the test data, calls the service method, and verifies the response.🧰 Tools
🪛 Biome
[error] 148-148: 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/course/manage/overview/course-management-card.component.html (1)
Line range hint
342-352: LGTM! Change aligns with PR objectives and follows coding guidelines.The modification to allow course editors (including tutors) to access the FAQ section is in line with the PR objectives. The use of the new
@ifsyntax instead of*ngIfadheres to the provided coding guidelines for Angular templates. Thecourse.faqEnabledcheck is maintained, ensuring that the FAQ feature remains toggleable per course.src/main/webapp/app/faq/faq.component.ts (7)
2-2: Import statements are appropriateThe added imports for
FaqandFaqStateare correct and necessary for managing FAQ entities and states.
18-20: Added service and model imports are correctThe imports for
AccountService,Course, andTranslateServiceare appropriately added to support user role checks, course data access, and translations.
30-30: Assignment ofFaqStatefor template accessAssigning
FaqStateto a class property allows for enum access within the template, which is appropriate.
32-32: Declaration ofcoursepropertyThe
courseproperty is correctly declared and is necessary for course-specific operations within the component.
53-54: Initialization of additional iconsThe addition of
faCancelandfaCheckicons is appropriate for the new FAQ acceptance and rejection functionality.
60-61: Injection of additional servicesInjecting
AccountServiceandTranslateServiceis necessary for user role verification and internationalization support.
141-166: Implementation of FAQ state management methodsThe
rejectFaqandacceptProposedFaqmethods are well-implemented, handling state updates and error cases effectively.src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)
Line range hint
77-87: Access control update increateFaqmethodThe change to
@EnforceAtLeastEditorand the authorization check forRole.EDITORin thecreateFaqmethod correctly allows editors to create FAQs, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- src/main/webapp/app/faq/faq-update.component.ts (6 hunks)
- src/main/webapp/i18n/de/faq.json (2 hunks)
- src/main/webapp/i18n/en/faq.json (2 hunks)
- src/test/javascript/spec/component/faq/faq-update.component.spec.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/i18n/de/faq.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/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}}
🪛 Biome
src/main/webapp/app/faq/faq-update.component.ts
[error] 35-35: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/test/javascript/spec/component/faq/faq-update.component.spec.ts
[error] 145-145: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 172-172: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (13)
src/main/webapp/i18n/en/faq.json (4)
7-9: LGTM: New labels for FAQ management.The new labels "proposeLabel", "accept", and "reject" are clear and align well with the PR objectives of allowing editors to propose FAQs. These additions enhance the FAQ management functionality.
18-19: LGTM: New messages for accepting and rejecting FAQs.The new messages for accepted and rejected FAQs are clear, consistent, and include the FAQ ID. These additions support the new functionality of managing proposed FAQs.
28-29: LGTM: New "state" label for FAQ table.The addition of the "state" label in the table object is appropriate for the new FAQ management functionality. It will allow users to see the current state of each FAQ (e.g., proposed, accepted, rejected) in the UI.
Line range hint
1-33: Overall LGTM: Comprehensive enhancements to FAQ management localization.The changes in this file effectively support the new FAQ proposal and management features outlined in the PR objectives. The new labels and messages provide clear guidance for users interacting with the enhanced FAQ functionality. The inclusion of FAQ IDs in most messages improves traceability.
These localization updates will contribute to a more intuitive user experience for both tutors proposing FAQs and instructors managing them. Great job on maintaining consistency throughout the file, with only the minor suggestion for the "deleted" message noted earlier.
src/main/webapp/i18n/de/faq.json (3)
7-9: LGTM: New labels correctly translated and use informal language.The new labels for proposing, accepting, and rejecting FAQs are correctly translated into German and adhere to the informal language (dutzen) requirement as per the coding guidelines.
28-29: LGTM: New "state" entry correctly added.The new entry for "state" (Status) has been correctly added to the table section. The translation is appropriate and consistent with the existing style.
Line range hint
1-34: Summary of changes and recommendationsOverall, the changes to this German localization file for FAQ-related content are well-implemented and enhance the FAQ management functionality. The new entries and modifications adhere to the informal language (dutzen) requirement as per the coding guidelines.
A few minor issues were identified:
- A grammatical error in the "proposedChange" message.
- Inconsistent use of placeholders (
{{ param }}vs{{ id }}).- A typo in the "accepted" message.
Please address these issues as outlined in the previous comments. Once these corrections are made, the file will be in excellent shape and ready for use.
src/test/javascript/spec/component/faq/faq-update.component.spec.ts (2)
Line range hint
1-254: Overall LGTM! Changes align well with PR objectives.The modifications to this test file effectively support the new functionality for proposing FAQs and edits:
- The removal of the FaqState import and the use of string literals for FAQ states are consistent throughout the file.
- New test cases have been added to cover the scenarios where tutors propose new FAQs and edits to existing FAQs.
- The
isAtLeastInstructorproperty has been introduced to differentiate between instructor and tutor roles in the test cases.These changes align well with the PR objectives of allowing tutors to propose FAQs and edits for review by instructors.
To further improve the code:
- Consider using constants for FAQ states to improve maintainability.
- Refactor common setup code in similar test cases to reduce duplication and improve readability.
These suggestions will make the test suite more robust and easier to maintain in the future.
🧰 Tools
🪛 Biome
[error] 145-145: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
14-14: LGTM! Verify FaqState usage consistency.The removal of the FaqState import aligns with the shift to using string literals for FAQ states in the test cases. This change is consistent with the PR objectives.
To ensure consistency, let's verify that FaqState is not used elsewhere in this file:
✅ Verification successful
Verified Removal of FaqState Import
The
FaqStateimport has been successfully removed, and no remaining usages were found infaq-update.component.spec.ts. This confirms that the change is consistent and does not impact existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of FaqState in the file # Test: Search for FaqState usage echo "Searching for FaqState usage:" rg "FaqState" src/test/javascript/spec/component/faq/faq-update.component.spec.tsLength of output: 150
src/main/webapp/app/faq/faq-update.component.ts (4)
84-84: Approved: Logic for setting FAQ state is correctThe assignment of
faqStatebased on the user's role is correctly implemented. This ensures that FAQs are appropriately marked asACCEPTEDorPROPOSEDdepending on the user's permissions.
35-35: Approved: Variable naming follows coding standardsThe property
isAtLeastInstructoris correctly named using camelCase, adhering to the project's coding guidelines.🧰 Tools
🪛 Biome
[error] 35-35: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
49-49: Consistent use of theinjectfunction for dependency injectionUsing the
injectfunction to obtain instances of services likeAccountServiceis consistent with the rest of the component and aligns with Angular's recommended practices.
114-128: Ensure success messages are correctly localizedThe success messages use translation keys, which is good practice for localization. Verify that the corresponding entries exist in the localization files for both English and German, as per the project's requirements.
Run the following script to check for the existence of translation keys:
| "created": "The FAQ with ID {{ id }} was successfully created", | ||
| "updated": "The FAQ with ID {{ id }} was successfully updated", | ||
| "proposed": "The FAQ with ID {{ id }} was successfully proposed", | ||
| "proposedChange": "The changes to the FAQ with the ID {{ id }} have been successfully proposed", | ||
| "deleted": "The FAQ with ID {{ param }} was successfully deleted", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve with suggestion: Consistent parameter naming.
The updated messages now include the FAQ ID, which improves traceability and user feedback. The new messages for proposed FAQs align well with the new functionality. Great improvements overall!
However, for consistency, please update the "deleted" message:
- "deleted": "The FAQ with ID {{ param }} was successfully deleted",
+ "deleted": "The FAQ with ID {{ id }} was successfully deleted",This will maintain consistency with the other messages using "{{ id }}".
📝 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.
| "created": "The FAQ with ID {{ id }} was successfully created", | |
| "updated": "The FAQ with ID {{ id }} was successfully updated", | |
| "proposed": "The FAQ with ID {{ id }} was successfully proposed", | |
| "proposedChange": "The changes to the FAQ with the ID {{ id }} have been successfully proposed", | |
| "deleted": "The FAQ with ID {{ param }} was successfully deleted", | |
| "created": "The FAQ with ID {{ id }} was successfully created", | |
| "updated": "The FAQ with ID {{ id }} was successfully updated", | |
| "proposed": "The FAQ with ID {{ id }} was successfully proposed", | |
| "proposedChange": "The changes to the FAQ with the ID {{ id }} have been successfully proposed", | |
| "deleted": "The FAQ with ID {{ id }} was successfully deleted", |
| "created": "Das FAQ mit der ID {{ id }} wurde erfolgreich erstellt", | ||
| "updated": "Das FAQ mit der ID {{ id }} wurde erfolgreich aktualisiert", | ||
| "proposed": "Das FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen", | ||
| "proposedChange": "Die Änderungen am FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen", |
There was a problem hiding this comment.
Fix grammatical error in "proposedChange" message.
The translations are correct and use informal language as required. However, there's a grammatical error in the "proposedChange" message.
Please apply the following change:
- "proposedChange": "Die Änderungen am FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen",
+ "proposedChange": "Die Änderungen am FAQ mit der ID {{ id }} wurden erfolgreich vorgeschlagen",The rest of the changes in this segment look good.
📝 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.
| "created": "Das FAQ mit der ID {{ id }} wurde erfolgreich erstellt", | |
| "updated": "Das FAQ mit der ID {{ id }} wurde erfolgreich aktualisiert", | |
| "proposed": "Das FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen", | |
| "proposedChange": "Die Änderungen am FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen", | |
| "created": "Das FAQ mit der ID {{ id }} wurde erfolgreich erstellt", | |
| "updated": "Das FAQ mit der ID {{ id }} wurde erfolgreich aktualisiert", | |
| "proposed": "Das FAQ mit der ID {{ id }} wurde erfolgreich vorgeschlagen", | |
| "proposedChange": "Die Änderungen am FAQ mit der ID {{ id }} wurden erfolgreich vorgeschlagen", |
| "deleted": "Das FAQ mit der ID {{ param }} wurde erfolgreich gelöscht", | ||
| "accepted": "Das FAQ mit der ID {{ id }} wurde erfolgreich akzeptieren", | ||
| "rejected": "Das FAQ mit der ID {{ id }} wurde erfolgreich abgelehnt", |
There was a problem hiding this comment.
Fix inconsistent placeholder and typo.
There are two issues in this segment:
- Inconsistent placeholder usage
- Typo in the "accepted" message
Please apply the following changes:
- "deleted": "Das FAQ mit der ID {{ param }} wurde erfolgreich gelöscht",
+ "deleted": "Das FAQ mit der ID {{ id }} wurde erfolgreich gelöscht",
- "accepted": "Das FAQ mit der ID {{ id }} wurde erfolgreich akzeptieren",
+ "accepted": "Das FAQ mit der ID {{ id }} wurde erfolgreich akzeptiert",These changes will ensure consistency in placeholder usage and correct the typo in the "accepted" message.
📝 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.
| "deleted": "Das FAQ mit der ID {{ param }} wurde erfolgreich gelöscht", | |
| "accepted": "Das FAQ mit der ID {{ id }} wurde erfolgreich akzeptieren", | |
| "rejected": "Das FAQ mit der ID {{ id }} wurde erfolgreich abgelehnt", | |
| "deleted": "Das FAQ mit der ID {{ id }} wurde erfolgreich gelöscht", | |
| "accepted": "Das FAQ mit der ID {{ id }} wurde erfolgreich akzeptiert", | |
| "rejected": "Das FAQ mit der ID {{ id }} wurde erfolgreich abgelehnt", |
| @@ -93,6 +93,7 @@ describe('FaqUpdateComponent', () => { | |||
|
|
|||
| it('should create faq', fakeAsync(() => { | |||
| faqUpdateComponent.faq = { questionTitle: 'test1' } as Faq; | |||
| faqUpdateComponent.isAtLeastInstructor = true; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider using a constant for FAQ states.
The changes align with the PR objectives:
- The introduction of
isAtLeastInstructorcorrectly differentiates between instructor and tutor roles. - The use of the string literal 'ACCEPTED' is consistent with the removal of the FaqState import.
To improve maintainability, consider defining constants for FAQ states:
const ACCEPTED_FAQ_STATE = 'ACCEPTED';
const PROPOSED_FAQ_STATE = 'PROPOSED';
// Then use them in the test cases:
expect(createSpy).toHaveBeenCalledExactlyOnceWith(courseId, { questionTitle: 'test1', faqState: ACCEPTED_FAQ_STATE });This approach would make it easier to update state values across all test cases if needed in the future.
Also applies to: 115-117
| it('should propose faq', fakeAsync(() => { | ||
| faqUpdateComponent.faq = { questionTitle: 'test1' } as Faq; | ||
| faqUpdateComponent.isAtLeastInstructor = false; | ||
| const createSpy = jest.spyOn(faqService, 'create').mockReturnValue( | ||
| of( | ||
| new HttpResponse({ | ||
| body: { | ||
| id: 3, | ||
| questionTitle: 'test1', | ||
| course: { | ||
| id: 1, | ||
| }, | ||
| } as Faq, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| faqUpdateComponentFixture.detectChanges(); | ||
| faqUpdateComponent.save(); | ||
| tick(); | ||
|
|
||
| expect(createSpy).toHaveBeenCalledExactlyOnceWith(courseId, { questionTitle: 'test1', faqState: 'PROPOSED' }); | ||
| expect(faqUpdateComponent.isSaving).toBeFalse(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! New test case for proposing FAQs.
The new test case "should propose faq" aligns well with the PR objectives:
- It correctly tests the scenario where a tutor (non-instructor) proposes an FAQ.
- The test structure is consistent with the existing "should create faq" test case.
- The use of the string literal 'PROPOSED' is consistent with earlier changes.
To improve consistency and reduce duplication, consider extracting the common setup code for both FAQ creation test cases into a separate function:
function setupFaqCreationTest(isAtLeastInstructor: boolean, expectedState: string) {
faqUpdateComponent.faq = { questionTitle: 'test1' } as Faq;
faqUpdateComponent.isAtLeastInstructor = isAtLeastInstructor;
const createSpy = jest.spyOn(faqService, 'create').mockReturnValue(
of(
new HttpResponse({
body: {
id: 3,
questionTitle: 'test1',
course: { id: 1 },
} as Faq,
}),
),
);
faqUpdateComponentFixture.detectChanges();
faqUpdateComponent.save();
tick();
expect(createSpy).toHaveBeenCalledExactlyOnceWith(courseId, { questionTitle: 'test1', faqState: expectedState });
expect(faqUpdateComponent.isSaving).toBeFalse();
}
// Then use it in both test cases:
it('should create faq', fakeAsync(() => setupFaqCreationTest(true, 'ACCEPTED')));
it('should propose faq', fakeAsync(() => setupFaqCreationTest(false, 'PROPOSED')));This refactoring would reduce code duplication and make it easier to maintain these test cases in the future.
| expect(faqUpdateComponent.isSaving).toBeFalse(); | ||
| })); | ||
|
|
||
| it('should edit a faq', fakeAsync(() => { | ||
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } }); | ||
|
|
||
| faqUpdateComponent.isAtLeastInstructor = true; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider using a constant for FAQ states.
The changes in the "should edit a faq" test case are consistent with earlier modifications and align with the PR objectives:
- Setting
isAtLeastInstructorto true correctly represents the instructor role. - The use of the string literal 'ACCEPTED' for the faqState is consistent with previous changes.
As suggested earlier, consider using a constant for the FAQ state to improve maintainability:
const ACCEPTED_FAQ_STATE = 'ACCEPTED';
// Then use it in the test case:
expect(updateSpy).toHaveBeenCalledExactlyOnceWith(courseId, { id: 6, questionTitle: 'test1Updated', faqState: ACCEPTED_FAQ_STATE });This approach would make it easier to update state values across all test cases if needed in the future.
Also applies to: 168-169
| it('should propose to edit a faq', fakeAsync(() => { | ||
| activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } }); | ||
| faqUpdateComponent.isAtLeastInstructor = false; | ||
| faqUpdateComponentFixture.detectChanges(); | ||
| faqUpdateComponent.faq = { id: 6, questionTitle: 'test1Updated' } as Faq; | ||
|
|
||
| expect(updateSpy).toHaveBeenCalledExactlyOnceWith(courseId, { id: 6, questionTitle: 'test1Updated' }); | ||
| const updateSpy = jest.spyOn(faqService, 'update').mockReturnValue( | ||
| of<HttpResponse<Faq>>( | ||
| new HttpResponse({ | ||
| body: { | ||
| id: 6, | ||
| questionTitle: 'test1Updated', | ||
| questionAnswer: 'answer', | ||
| course: { | ||
| id: 1, | ||
| }, | ||
| } as Faq, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| faqUpdateComponent.save(); | ||
| tick(); | ||
| faqUpdateComponentFixture.detectChanges(); | ||
| expect(updateSpy).toHaveBeenCalledExactlyOnceWith(courseId, { id: 6, questionTitle: 'test1Updated', faqState: 'PROPOSED' }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! New test case for proposing FAQ edits.
The new test case "should propose to edit a faq" aligns well with the PR objectives:
- It correctly tests the scenario where a tutor (non-instructor) proposes an edit to an existing FAQ.
- The test structure is consistent with the existing "should edit a faq" test case.
- The use of the string literal 'PROPOSED' is consistent with earlier changes.
To improve consistency and reduce duplication, consider the following refactoring:
- Extract common setup code for both FAQ editing test cases into a separate function:
function setupFaqEditTest(isAtLeastInstructor: boolean, expectedState: string) {
activatedRoute.parent!.data = of({ course: { id: 1 }, faq: { id: 6 } });
faqUpdateComponent.isAtLeastInstructor = isAtLeastInstructor;
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,
questionTitle: 'test1Updated',
questionAnswer: 'answer',
course: { id: 1 },
} as Faq,
}),
),
);
faqUpdateComponent.save();
tick();
faqUpdateComponentFixture.detectChanges();
expect(updateSpy).toHaveBeenCalledExactlyOnceWith(courseId, { id: 6, questionTitle: 'test1Updated', faqState: expectedState });
}
// Then use it in both test cases:
it('should edit a faq', fakeAsync(() => setupFaqEditTest(true, 'ACCEPTED')));
it('should propose to edit a faq', fakeAsync(() => setupFaqEditTest(false, 'PROPOSED')));- Use constants for FAQ states as suggested in previous comments:
const ACCEPTED_FAQ_STATE = 'ACCEPTED';
const PROPOSED_FAQ_STATE = 'PROPOSED';
// Update the function call:
it('should edit a faq', fakeAsync(() => setupFaqEditTest(true, ACCEPTED_FAQ_STATE)));
it('should propose to edit a faq', fakeAsync(() => setupFaqEditTest(false, PROPOSED_FAQ_STATE)));These refactorings would significantly reduce code duplication, improve maintainability, and make the test cases more consistent and easier to understand.
🧰 Tools
🪛 Biome
[error] 172-172: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| if (this.isAtLeastInstructor) { | ||
| this.alertService.success(this.translateService.instant('artemisApp.faq.created', { id: faq.id })); | ||
| } else { | ||
| this.alertService.success(this.translateService.instant('artemisApp.faq.proposed', { id: faq.id })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated success message code to improve maintainability
The blocks of code displaying success messages in both the creation and update scenarios are similar, with differences only in the translation keys and messages. To reduce duplication and enhance maintainability, consider refactoring the success message logic into a separate method or variable.
Apply this refactor to consolidate the success message handling:
protected onSaveSuccess(faq: Faq) {
this.isSaving = false;
const isNewFaq = !this.faq.id;
const actionKey = isNewFaq ? (this.isAtLeastInstructor ? 'created' : 'proposed') : (this.isAtLeastInstructor ? 'updated' : 'proposedChange');
const translationKey = `artemisApp.faq.${actionKey}`;
+ this.alertService.success(this.translateService.instant(translationKey, { id: faq.id }));
this.router.navigate(['course-management', this.courseId, 'faqs']);
}Then, you can remove the duplicated code blocks from both if-else branches.
Also applies to: 124-128
| @@ -77,10 +81,10 @@ | |||
| */ | |||
| save() { | |||
| this.isSaving = true; | |||
| this.faq.faqState = this.isAtLeastInstructor ? FaqState.ACCEPTED : FaqState.PROPOSED; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider handling potential null values for this.faq
While assigning faqState, ensure that this.faq is not null or undefined to prevent potential runtime errors.
Apply this minor adjustment:
this.isSaving = true;
+if (this.faq) {
this.faq.faqState = this.isAtLeastInstructor ? FaqState.ACCEPTED : FaqState.PROPOSED;
+}📝 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.
| this.faq.faqState = this.isAtLeastInstructor ? FaqState.ACCEPTED : FaqState.PROPOSED; | |
| this.isSaving = true; | |
| if (this.faq) { | |
| this.faq.faqState = this.isAtLeastInstructor ? FaqState.ACCEPTED : FaqState.PROPOSED; | |
| } |
| @@ -56,6 +59,7 @@ | |||
| if (course) { | |||
| this.faq.course = course; | |||
| this.loadCourseFaqCategories(course.id); | |||
| this.isAtLeastInstructor = this.accountService.isAtLeastInstructorInCourse(course); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the subscription is properly unsubscribed to prevent memory leaks
The subscribe method used on this.activatedRoute.parent?.data in ngOnInit may lead to a memory leak if the component is destroyed before the observable completes. To prevent this, consider using the takeUntil operator with an ngOnDestroy lifecycle hook to unsubscribe when the component is destroyed.
Apply this refactor to manage the subscription:
import { Component, OnInit, OnDestroy, inject } from '@angular/core';
import { Subject } from 'rxjs';
+import { takeUntil } from 'rxjs/operators';
export class FaqUpdateComponent implements OnInit, OnDestroy {
private ngUnsubscribe = new Subject<void>();
ngOnInit() {
// ...
- this.activatedRoute.parent?.data.subscribe((data) => {
+ this.activatedRoute.parent?.data.pipe(takeUntil(this.ngUnsubscribe)).subscribe((data) => {
// existing code
});
}
+ ngOnDestroy() {
+ this.ngUnsubscribe.next();
+ this.ngUnsubscribe.complete();
+ }
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- src/main/webapp/app/faq/faq.component.html (3 hunks)
- src/main/webapp/app/faq/faq.component.ts (7 hunks)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (2 hunks)
- src/test/javascript/spec/component/faq/faq.component.spec.ts (4 hunks)
- src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (3 hunks)
- src/test/javascript/spec/service/faq.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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.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/component/overview/course-faq/course-faq.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/service/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.component.ts
[error] 37-37: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 38-38: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/test/javascript/spec/service/faq.service.spec.ts
[error] 148-148: 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 (16)
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (2)
19-19: LGTM: Import statement updated correctly.The import statement has been properly updated to include
FaqState. This change aligns with the modifications in the test case and suggests that the component now uses theFaqStateenum.
72-72: LGTM: Mock provider updated correctly.The mock provider for
FaqServicehas been properly updated to use the new method namefindAllByCourseIdAndState. This change reflects an update in theFaqServiceAPI and is consistent with the modifications in the test case.src/test/javascript/spec/service/faq.service.spec.ts (1)
Line range hint
1-255: Overall, excellent test coverage and structure.The entire test suite for FaqService is well-organized and comprehensive. It covers various scenarios and methods of the service, maintaining consistency in test structure and assertions. The new test case for
findAllByCourseIdAndStateintegrates seamlessly with the existing tests.Good job on:
- Consistent use of TestBed configuration
- Proper mocking of HTTP requests
- Thorough testing of different service methods
- Clear and descriptive test case names
Keep up the good work in maintaining high-quality tests!
🧰 Tools
🪛 Biome
[error] 148-148: 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/overview/course-faq/course-faq.component.ts (2)
10-10: Importing 'FaqState' is appropriate.The addition of
FaqStateto the import statement is necessary for filtering FAQs by their state in theloadFaqsmethod.
74-74: Filtering FAQs by 'ACCEPTED' state aligns with PR objectives.Updating the service call to
findAllByCourseIdAndState(this.courseId, FaqState.ACCEPTED)ensures that only accepted FAQs are loaded, which meets the goal of displaying only accepted FAQs to students.src/main/webapp/app/faq/faq.component.ts (5)
2-3: Imports are correctly addedThe necessary imports for
Faq,FaqState, and FontAwesome icons are appropriately included.
19-21: Services and models are properly importedThe imports for
AccountService,Course, andTranslateServiceare correctly added to support the new functionalities.
31-31: New propertiesFaqStateandcourseare correctly definedThe
FaqStateconstant is assigned for template usage, and thecourseproperty is appropriately declared.Also applies to: 33-33
55-56: FontAwesome icons are correctly initializedThe
faCancelandfaCheckicons are properly initialized for use in the template.
62-63: Services are properly injectedThe
accountServiceandtranslateServiceare correctly injected into the component.src/main/webapp/app/faq/faq.component.html (2)
77-80: Addition of 'State' column looks goodThe inclusion of the "State" column in the FAQ table enhances the functionality by allowing users to see the status of each FAQ and sort accordingly.
112-122: Accept/Reject functionality is correctly implementedThe conditional rendering of the accept and reject buttons for instructors when an FAQ is in the
PROPOSEDstate is appropriate and follows the desired access control.src/test/javascript/spec/component/faq/faq.component.spec.ts (4)
12-12: Importing 'FaqState' for FAQ state managementThe addition of
FaqStateto the import statement is appropriate for handling FAQ states within the tests.
21-22: Importing and mocking 'AccountService' correctlyThe imports for
MockAccountServiceandAccountServiceare correctly added to mock authentication services in the tests.
30-30: Initializing 'faqState' to 'PROPOSED'Setting the
faqStateproperty toFaqState.PROPOSEDensures that the FAQ is initialized with the correct state.
63-63: Providing 'MockAccountService' in TestBedMocking
AccountServicewithMockAccountServicein the testing module is appropriate for isolating the component during testing.
| findAllByCourseIdAndState(courseId: number, faqState: FaqState): Observable<EntityArrayResponseType> { | ||
| return this.http | ||
| .get<Faq[]>(`${this.resourceUrl}/${courseId}/faq-state/${faqState}`, { | ||
| observe: 'response', | ||
| }) | ||
| .pipe(map((res: EntityArrayResponseType) => FaqService.convertFaqCategoryArrayFromServer(res))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding error handling.
The new findAllByCourseIdAndState method is well-implemented and follows Angular best practices. It maintains consistency with other methods in the service, promotes code reuse, and adheres to type safety.
Consider adding error handling to improve robustness:
import { catchError, throwError } from 'rxjs';
import { HttpErrorResponse } from '@angular/common/http';
// ...
findAllByCourseIdAndState(courseId: number, faqState: FaqState): Observable<EntityArrayResponseType> {
return this.http
.get<Faq[]>(`${this.resourceUrl}/${courseId}/faq-state/${faqState}`, {
observe: 'response',
})
.pipe(
map((res: EntityArrayResponseType) => FaqService.convertFaqCategoryArrayFromServer(res)),
catchError((error: HttpErrorResponse) => {
console.error('Error fetching FAQs:', error);
return throwError(() => new Error('Failed to fetch FAQs. Please try again later.'));
})
);
}This addition will log any errors and provide a user-friendly error message.
| const findAllSpy = jest.spyOn(faqService, 'findAllByCourseIdAndState'); | ||
|
|
||
| courseFaqComponentFixture.detectChanges(); | ||
| expect(findAllSpy).toHaveBeenCalledExactlyOnceWith(1); | ||
| expect(findAllSpy).toHaveBeenCalledExactlyOnceWith(1, FaqState.ACCEPTED); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Test case updated correctly, with a suggestion for improvement.
The test case has been properly updated to use findAllByCourseIdAndState and includes the FaqState.ACCEPTED parameter. The use of toHaveBeenCalledExactlyOnceWith ensures that the method is called with the correct parameters.
To further improve the test's robustness, consider implementing the suggestion from the previous review:
expect(courseFaqComponent.faqs).toBeUndefined();
courseFaqComponentFixture.detectChanges();
expect(findAllSpy).toHaveBeenCalledExactlyOnceWith(1, FaqState.ACCEPTED);
expect(courseFaqComponent.faqs).toHaveLength(3);This addition would ensure that the component's state is updated only after the change detection.
| it('should find faqs by courseId and status', () => { | ||
| const category = { | ||
| color: '#6ae8ac', | ||
| category: 'category1', | ||
| } as FaqCategory; | ||
| const returnedFromService = [{ ...elemDefault, categories: [JSON.stringify(category)] }]; | ||
| const expected = [{ ...elemDefault, categories: [new FaqCategory('category1', '#6ae8ac')] }]; | ||
| const courseId = 1; | ||
| service | ||
| .findAllByCourseIdAndState(courseId, FaqState.ACCEPTED) | ||
| .pipe(take(1)) | ||
| .subscribe((resp) => (expectedResult = resp)); | ||
| const req = httpMock.expectOne({ | ||
| url: `api/courses/${courseId}/faq-state/${FaqState.ACCEPTED}`, | ||
| method: 'GET', | ||
| }); | ||
| req.flush(returnedFromService); | ||
| expect(expectedResult.body).toEqual(expected); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM with a minor suggestion.
The new test case for findAllByCourseIdAndState is well-structured and covers the essential aspects of the method's functionality. It correctly sets up the test data, mocks the HTTP request, and verifies the response.
However, there's a minor improvement we can make:
On line 148, let's adjust the subscribe callback to avoid the assignment in an expression:
- .subscribe((resp) => (expectedResult = resp));
+ .subscribe((resp) => { expectedResult = resp; });This change improves readability and addresses the static analysis warning about assignments in expressions.
The test case follows the coding guidelines for jest tests, using toEqual for object comparison and properly mocking the HTTP request. Good job on maintaining consistency with the existing test structure!
📝 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 find faqs by courseId and status', () => { | |
| const category = { | |
| color: '#6ae8ac', | |
| category: 'category1', | |
| } as FaqCategory; | |
| const returnedFromService = [{ ...elemDefault, categories: [JSON.stringify(category)] }]; | |
| const expected = [{ ...elemDefault, categories: [new FaqCategory('category1', '#6ae8ac')] }]; | |
| const courseId = 1; | |
| service | |
| .findAllByCourseIdAndState(courseId, FaqState.ACCEPTED) | |
| .pipe(take(1)) | |
| .subscribe((resp) => (expectedResult = resp)); | |
| const req = httpMock.expectOne({ | |
| url: `api/courses/${courseId}/faq-state/${FaqState.ACCEPTED}`, | |
| method: 'GET', | |
| }); | |
| req.flush(returnedFromService); | |
| expect(expectedResult.body).toEqual(expected); | |
| }); | |
| it('should find faqs by courseId and status', () => { | |
| const category = { | |
| color: '#6ae8ac', | |
| category: 'category1', | |
| } as FaqCategory; | |
| const returnedFromService = [{ ...elemDefault, categories: [JSON.stringify(category)] }]; | |
| const expected = [{ ...elemDefault, categories: [new FaqCategory('category1', '#6ae8ac')] }]; | |
| const courseId = 1; | |
| service | |
| .findAllByCourseIdAndState(courseId, FaqState.ACCEPTED) | |
| .pipe(take(1)) | |
| .subscribe((resp) => { expectedResult = resp; }); | |
| const req = httpMock.expectOne({ | |
| url: `api/courses/${courseId}/faq-state/${FaqState.ACCEPTED}`, | |
| method: 'GET', | |
| }); | |
| req.flush(returnedFromService); | |
| expect(expectedResult.body).toEqual(expected); | |
| }); |
🧰 Tools
🪛 Biome
[error] 148-148: 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)
| rejectFaq(courseId: number, faq: Faq) { | ||
| const previousState = faq.faqState; | ||
| faq.faqState = FaqState.REJECTED; | ||
| faq.course = this.course; | ||
| this.faqService.update(courseId, faq).subscribe({ | ||
| next: () => this.alertService.success(this.translateService.instant('artemisApp.faq.rejected', { id: faq.id })), | ||
| error: (error: HttpErrorResponse) => { | ||
| this.dialogErrorSource.next(error.message); | ||
| faq.faqState = previousState; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| acceptProposedFaq(courseId: number, faq: Faq) { | ||
| const previousState = faq.faqState; | ||
| faq.faqState = FaqState.ACCEPTED; | ||
| faq.course = this.course; | ||
| this.faqService.update(courseId, faq).subscribe({ | ||
| next: () => this.alertService.success(this.translateService.instant('artemisApp.faq.accepted', { id: faq.id })), | ||
| error: (error: HttpErrorResponse) => { | ||
| this.dialogErrorSource.next(error.message); | ||
| faq.faqState = previousState; | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor rejectFaq and acceptProposedFaq to reduce code duplication
The methods rejectFaq and acceptProposedFaq have similar logic. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring them into a single reusable method.
You can create a generic method to handle FAQ state updates:
private updateFaqState(courseId: number, faq: Faq, newState: FaqState, translationKey: string) {
const previousState = faq.faqState;
faq.faqState = newState;
faq.course = this.course;
this.faqService.update(courseId, faq).subscribe({
next: () => this.alertService.success(this.translateService.instant(translationKey, { id: faq.id })),
error: (error: HttpErrorResponse) => {
this.dialogErrorSource.next(error.message);
faq.faqState = previousState;
},
});
}Then, refactor the original methods to utilize this new function:
rejectFaq(courseId: number, faq: Faq) {
this.updateFaqState(courseId, faq, FaqState.REJECTED, 'artemisApp.faq.rejected');
}
acceptProposedFaq(courseId: number, faq: Faq) {
this.updateFaqState(courseId, faq, FaqState.ACCEPTED, 'artemisApp.faq.accepted');
}This refactoring enhances maintainability and simplifies future updates.
| <td> | ||
| <p class="markdown-preview" [innerHTML]="faq.faqState | htmlForMarkdown"></p> | ||
| </td> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Display user-friendly labels for FAQ states
Currently, the FAQ state is displayed using faq.faqState, which may show the raw enum value (e.g., PROPOSED, APPROVED). To improve user experience, consider displaying a user-friendly label or translating the state.
You can apply this change to use the Angular translate pipe with a translation key:
- <p class="markdown-preview" [innerHTML]="faq.faqState | htmlForMarkdown"></p>
+ <p>{{ 'artemisApp.faq.state.' + faq.faqState | translate }}</p>Ensure that you have the corresponding entries in your translation files, such as:
"artemisApp": {
"faq": {
"state": {
"PROPOSED": "Proposed",
"APPROVED": "Approved",
"REJECTED": "Rejected"
}
}
}Let me know if you need assistance in implementing this change or updating the translation files.
| @if (isAtleastInstrucor) { | ||
| <button | ||
| class="mt-1" | ||
| jhiDeleteButton | ||
| id="delete-faq-{{ faq.id }}" | ||
| [entityTitle]="faq.questionTitle || ''" | ||
| deleteQuestion="artemisApp.faq.delete.question" | ||
| deleteConfirmationText="artemisApp.faq.delete.typeNameToConfirm" | ||
| (delete)="deleteFaq(courseId, faq.id!)" | ||
| [dialogError]="dialogError$" | ||
| > | ||
| <fa-icon [icon]="faTrash" /> | ||
| </button> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider allowing tutors to delete their own proposed FAQs
Currently, only instructors can delete FAQs. To enhance usability and allow tutors to manage their own content, consider permitting tutors to delete their own proposed FAQs before they are reviewed by an instructor.
| faqComponentFixture.detectChanges(); | ||
| faqComponent.rejectFaq(courseId, faq1); | ||
| expect(faqService.update).toHaveBeenCalledExactlyOnceWith(courseId, faq1); | ||
| expect(faq1.faqState).toEqual(FaqState.REJECTED); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use 'toBe' instead of 'toEqual' for enum comparisons
When comparing enum values, it's recommended to use the toBe matcher for strict equality instead of toEqual.
Apply the following changes:
- expect(faq1.faqState).toEqual(FaqState.REJECTED);
+ expect(faq1.faqState).toBe(FaqState.REJECTED);
...
- expect(faq1.faqState).toEqual(FaqState.PROPOSED);
+ expect(faq1.faqState).toBe(FaqState.PROPOSED);
...
- expect(faq1.faqState).toEqual(FaqState.ACCEPTED);
+ expect(faq1.faqState).toBe(FaqState.ACCEPTED);
...
- expect(faq1.faqState).toEqual(FaqState.PROPOSED);
+ expect(faq1.faqState).toBe(FaqState.PROPOSED);Also applies to: 208-208, 216-216, 225-225
…von 454) Co-authored-by: Kaan Çaylı <38523756+kaancayli@users.noreply.github.com>
…ogramming exercises (#9217)
…ns in account settings for students (#9397)
…q' into feature/communication/propose-faq # Conflicts: # src/main/webapp/app/faq/faq.component.ts
github-actions
bot
added
database
Checklist
General
Server
Client
Motivation and Context
Tutors are mainly answering questions of the students, so they know what is feaquently asked. Consequently, they should be able to propose FAQs
Description
I allowed editors of the course to propose FAQ's
Steps for Testing
Prerequisites:
Log in to Artemis as Tutor
Navigate to Course Management
Select FAQ
Propse atleast 2 FAQ's
Check, that the proposed FAQ is not shown in the Course Overview
Log in as Instructor
Navigate to Course Management
See newly proposed FAQs
Accept / Reject FAQ
Go to Course Overview and verify, only accepted FAQ is shown
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
Documentation