Communication: Allow tutors to propose FAQ
#9477
Conversation
…q' into feature/communication/propose-faq # Conflicts: # src/main/webapp/app/faq/faq.component.ts
github-actions
bot
added
tests
server
WalkthroughThe pull request introduces several enhancements to the FAQ management system within the application. A new method is added to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 30
🧹 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1)
Line range hint
15-35: Overall: Good addition that enhances querying capabilities.The new method
findAllByCourseIdAndFaqStateis a valuable addition to theFaqRepositoryinterface. It enhances the querying capabilities by allowing filtering of FAQs based on both course ID and state. This change is consistent with the repository's purpose and doesn't introduce any breaking changes.Consider adding an index on the
course_idandfaq_statecolumns in the database to optimize query performance, especially if this method is expected to be called frequently or if the FAQ table grows large.
📜 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 (6 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 (4 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] 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/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)
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 (45)
src/main/webapp/app/entities/faq.model.ts (1)
6-8: Approved: Enum values explicitly defined as strings.The change from implicit numeric values to explicit string values for the
FaqStateenum is a good improvement. It enhances code readability and type safety, aligning with TypeScript best practices for string-based enums.To ensure this change doesn't introduce any issues, please verify the usage of these enum values throughout the codebase. Run the following script to check for any potential impacts:
If any usages are found that compare these enum values as numbers or use them in a way that assumes numeric values, they will need to be updated.
✅ Verification successful
Verified: Enum usage consistent with string-based definitions.
The
FaqStateenum is consistently used with the updated string values throughout the codebase, ensuring compatibility and maintaining type safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of FaqState enum values # Test: Search for usages of FaqState.ACCEPTED, FaqState.REJECTED, and FaqState.PROPOSED # Expect: Only string comparisons or assignments using the new string values rg --type typescript "FaqState\.(ACCEPTED|REJECTED|PROPOSED)"Length of output: 98
Script:
#!/bin/bash # Description: Search for usages of FaqState enum values in TypeScript files # Test: Search for usages of FaqState.ACCEPTED, FaqState.REJECTED, and FaqState.PROPOSED # Expect: Only string comparisons or assignments using the new string values rg "FaqState\.(ACCEPTED|REJECTED|PROPOSED)" --glob "*.ts" --glob "*.tsx"Length of output: 1620
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1)
15-15: LGTM: Import statement is correct and necessary.The import statement for
FaqStateis correctly added and follows good Java practices by avoiding star imports.src/main/webapp/i18n/en/faq.json (3)
7-9: LGTM: New labels for FAQ proposal workflow.The new labels "Propose a new FAQ", "Accept FAQ", and "Reject FAQ" are clear and align well with the PR objectives. They provide the necessary UI text for the new FAQ proposal feature.
29-29: LGTM: Added "state" to table section.The addition of "state" to the table section is a good change that supports the new FAQ proposal workflow. This will allow users to see the current state of each FAQ (e.g., proposed, accepted, rejected) in the UI.
28-28: LGTM: Added "categories" to table section, but needs clarification.The addition of "categories" to the table section is consistent with the existing structure and suggests a new feature for categorizing FAQs.
Could you please clarify the purpose of adding categories to FAQs? This change wasn't mentioned in the PR objectives or summary. Is this part of the FAQ proposal feature or a separate enhancement?
src/main/webapp/i18n/de/faq.json (4)
7-9: LGTM: New labels added correctlyThe new labels for proposing, accepting, and rejecting FAQs have been added correctly. They use the informal "du" form as required by the coding guidelines for German translations.
13-14: LGTM: Updated messages with FAQ IDThe messages for created and updated FAQs have been correctly modified to include the FAQ ID. They maintain the required informal "du" form and provide more specific feedback to the user.
28-28: LGTM: Categories label correctly updatedThe "categories" label has been correctly updated and moved. The translation is accurate and uses the appropriate plural form.
29-29: LGTM: New state label added correctlyThe new "state" label has been added correctly with the appropriate German translation "Status". This addition aligns with the new functionality for managing FAQ states.
src/main/webapp/app/overview/course-faq/course-faq.component.ts (2)
10-10: LGTM: Import statement added correctly.The addition of the
FaqStateimport is consistent with its usage in theloadFaqs()method and follows the TypeScript coding guidelines.
Line range hint
1-118: Verify component behavior and update tests.The changes to
loadFaqs()might affect the number of FAQs displayed in the component. While the rest of the component's logic (filtering, searching) should work with the new subset of FAQs, it's important to:
- Verify that the component behaves correctly with only accepted FAQs.
- Update any existing tests for this component to reflect the new behavior of only displaying accepted FAQs.
- Consider adding a new test case specifically for the
FaqState.ACCEPTEDfilter.To assist in verifying the component's behavior, you can run the following command to check for any existing tests related to this component:
✅ Verification successful
Verified: Component behavior is correctly handled and tested.
The existing tests in
course-faq.component.spec.tscover the changes toloadFaqs()by verifying that only accepted FAQs are fetched and properly handled. No additional test updates are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to CourseFaqComponent fd -e spec.ts | xargs grep -l "CourseFaqComponent"Length of output: 134
Script:
#!/bin/bash # Search for occurrences of FaqState.ACCEPTED in the test spec file rg "FaqState.ACCEPTED" src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.tsLength of output: 188
Script:
#!/bin/bash # Search for assignments or assertions involving faqs or filteredFaqs in the test spec file rg "this\.faqs|filteredFaqs|expect.*\.toEqual|expect.*\.toHaveLength" src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.tsLength of output: 622
src/main/webapp/app/faq/faq.service.ts (2)
Line range hint
1-180: Changes align well with PR objectives and coding guidelines.The modifications to
FaqServiceeffectively support the new feature of allowing editors to propose FAQs:
- The new
findAllByCourseIdAndStatemethod enables filtering FAQs by both course ID and state, which is crucial for managing proposed FAQs.- The reported removal of automatic FAQ state setting in the
createmethod (pending verification) allows for the creation of FAQs in a non-accepted state.These changes adhere to Angular style guidelines, make appropriate use of TypeScript features, and maintain consistency with the existing codebase. The implementation appears to be free of memory leak risks and follows the provided coding guidelines for Angular TypeScript files.
To ensure full alignment with the PR objectives, please verify that:
- The FAQ state is being set appropriately when FAQs are created, either on the server-side or in the calling component.
- There are corresponding changes in the UI components to handle the display and management of FAQs in different states (e.g., proposed vs. accepted).
Line range hint
18-25: Verify the removal of automatic FAQ state setting.The AI summary mentions that the line
copy.faqState = FaqState.ACCEPTEDwas removed from thecreatemethod. This change aligns with the PR objective of allowing editors to propose FAQs. However, the removed line is not visible in the provided code snippet.Please confirm that this change was indeed made. If so, ensure that the FAQ state is being set appropriately elsewhere in the system, possibly on the server-side or in the component that calls this method.
To verify this change, you can run the following script:
This will help confirm that the automatic setting of the FAQ state to ACCEPTED has been removed from the
createmethod.✅ Verification successful
It appears that the
ripgrepcommand does not recognize the file typetypescript. To proceed, please run the following corrected script to check for the presence ofFaqState.ACCEPTEDin thecreatemethod:
Verified: The removal of
FaqState.ACCEPTEDfrom thecreatemethod has been confirmed and aligns with the PR objective of allowing editors to propose FAQs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of FaqState.ACCEPTED in the create method # Test: Search for FaqState.ACCEPTED in the create method rg --type typescript -A 10 'create\(courseId: number, faq: Faq\): Observable<EntityResponseType>' src/main/webapp/app/faq/faq.service.tsLength of output: 173
Script:
#!/bin/bash # Description: Check for the presence of FaqState.ACCEPTED in the create method # Test: Search for 'FaqState.ACCEPTED' within the create method rg -A 10 'create\(courseId: number, faq: Faq\): Observable<EntityResponseType>' src/main/webapp/app/faq/faq.service.tsLength of output: 605
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (3)
19-19: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
FaqState. This change aligns with the new functionality of handling FAQ states in the component.
72-72: LGTM: MockProvider updated to reflect API changes.The
FaqServicemock has been correctly updated to usefindAllByCourseIdAndStateinstead offindAllByCourseId. This change aligns with the updated API for querying FAQs.
121-124: LGTM: Test case updated to match new API.The test case has been correctly updated to use
findAllByCourseIdAndStateand includes theFaqState.ACCEPTEDparameter. This change ensures that the test accurately reflects the new API behavior.The use of
toHaveBeenCalledExactlyOnceWithadheres to the coding guidelines for spy calls, which is excellent.src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.html (1)
Line range hint
75-79: LGTM! Change aligns with PR objectives and follows coding guidelines.The modification to allow users with at least editor privileges (
course.isAtLeastEditor) to access the FAQ tab is consistent with the PR objective of enabling course editors (tutors) to propose FAQs. The use of the new Angular syntax@ifinstead of*ngIfadheres to the provided coding guidelines for HTML files in this project.src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (4)
18-18: LGTM: Import statement for FaqDTO added.The addition of the import statement for
FaqDTOis appropriate and necessary for the new functionality being introduced in this test class.
35-35: LGTM: FaqDTO field added.The addition of the
faqDTOfield is appropriate for testing the new FAQ proposal functionality. It follows proper naming conventions and access modifiers.
43-44: LGTM: Test case initialization updated for FAQ proposal functionality.The changes in the
initTestCasemethod appropriately set up the test environment for the new FAQ proposal feature:
- Changing the FAQ state to
PROPOSEDaligns with the new functionality.- Creating an
FaqDTOobject is necessary for testing the data transfer between layers.These modifications ensure that the test cases accurately reflect the new system behavior.
Line range hint
1-184: Summary of FaqIntegrationTest.java reviewThe changes in this file generally align well with the PR objectives of introducing FAQ proposal functionality. The new tests and modifications to existing ones appropriately cover the new features. However, there are a few areas for improvement:
- Naming conventions: Ensure all method names follow Java's camelCase convention.
- Test assertions: Enhance the assertions in the new test methods to verify not just the size of returned sets, but also their contents.
- Code duplication: Consider using parameterized tests to reduce duplication between similar test methods.
- Comments: Add explanatory comments for intentional test scenarios, such as ID mismatches.
Overall, these changes provide good test coverage for the new FAQ proposal feature, but implementing the suggested improvements will further enhance the test suite's robustness and maintainability.
src/test/javascript/spec/service/faq.service.spec.ts (1)
Line range hint
1-256: Overall assessment: Well-structured and comprehensive test suiteThe addition of the new test case for
findAllByCourseIdAndStatecomplements the existing test suite well. It maintains consistency with the established testing patterns and covers the new functionality adequately.The test file as a whole demonstrates good practices in testing Angular services, including:
- Proper setup and teardown of the testing environment
- Comprehensive coverage of service methods
- Consistent use of mock data and expectations
To further enhance the test suite, consider:
- Reviewing the use of
pipe(take(1))across all tests and removing if unnecessary- Ensuring all tests have specific assertions for expected behavior
- Addressing any remaining static analysis warnings
🧰 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 (2)
Line range hint
342-351: LGTM: Change aligns with PR objectivesThe modification to allow editors (including tutors) to access the FAQ section is well-implemented and aligns perfectly with the PR's goal of empowering tutors to contribute to FAQ content. The use of the new
@ifdirective adheres to the provided coding guidelines.A few observations:
- The condition
course.isAtLeastEditorappropriately broadens access to include editors.- The additional check
course.faqEnabledensures FAQs are only accessible when enabled for the course.This change effectively implements the desired functionality while maintaining proper access control.
Line range hint
1-351: Overall implementation adheres to coding guidelinesAfter reviewing the entire file, I can confirm that:
- The new Angular syntax (@if and @for) is used consistently throughout the template, adhering to the provided coding guidelines.
- The implementation of various course management functionalities appears sound and well-structured.
- No other changes related to the PR objectives are present in this file.
The overall implementation demonstrates good coding practices and maintains consistency with the existing codebase.
src/main/webapp/app/faq/faq-update.component.ts (6)
19-19: LGTM!The import of
AccountServiceis appropriate for role checking.
49-49: LGTM!Injecting
AccountServiceas a private member is appropriate for accessing user role information.
62-62: LGTM!Correctly setting
isAtLeastInstructorbased on the user's role in the course.
84-84: LGTM!Setting
faqStatebased on user role ensures proper handling of FAQs.
114-118: LGTM!Properly displaying success messages based on user role after creating an FAQ.
124-128: LGTM!Properly displaying success messages based on user role after updating an FAQ.
src/main/webapp/app/faq/faq.component.html (3)
77-80: Addition of 'FAQ State' column is correctly implementedThe new column for displaying the FAQ state is appropriately added to the table header.
112-123: Correct the variable name 'isAtleastInstrucor' to 'isAtLeastInstructor'As previously noted,
isAtleastInstrucoris misspelled. Please correct it toisAtLeastInstructorfor consistency.
129-142: Correct the variable name 'isAtleastInstrucor' to 'isAtLeastInstructor'As previously noted, the variable
isAtleastInstrucorshould be corrected toisAtLeastInstructor.src/test/javascript/spec/component/faq/faq-update.component.spec.ts (7)
14-14: Import 'Faq' is correctly addedThe import statement for
Faqfrom'app/entities/faq.model'is necessary for the component to reference theFaqmodel.
96-96: Correctly setting 'isAtLeastInstructor' to trueSetting
faqUpdateComponent.isAtLeastInstructor = true;ensures that the test simulates the behavior when the user has instructor permissions.
115-116: Verifying 'create' method is called with correct parametersThe expectations correctly verify that the
createmethod is called exactly once with the appropriate parameters, includingfaqState: 'ACCEPTED', and thatisSavingis set tofalseafter saving.
119-140: Test 'should propose faq' correctly simulates non-instructor behaviorThe test accurately reflects the scenario where a non-instructor proposes a FAQ. By setting
faqUpdateComponent.isAtLeastInstructor = false;, it ensures that thefaqStateis set to'PROPOSED'. The expectation confirms that thecreatemethod is called with the correct parameters.
146-146: Set 'isAtLeastInstructor' to true for instructor permissionsSetting
faqUpdateComponent.isAtLeastInstructor = true;ensures that the component behaves as expected when the user has instructor permissions during the edit operation.
168-169: Verifying 'update' method is called with correct parametersThe expectation confirms that the
updatemethod is called exactly once with the appropriate parameters, includingfaqState: 'ACCEPTED', which is correct for an instructor editing a FAQ.
171-195: Test 'should propose to edit a faq' correctly handles non-instructor editThe test simulates the behavior when a non-instructor user proposes an edit to an existing FAQ. By setting
faqUpdateComponent.isAtLeastInstructor = false;, it ensures that thefaqStateis set to'PROPOSED'. The expectation verifies that theupdatemethod is called with the correct parameters.🧰 Tools
🪛 Biome
[error] 172-172: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)
26-26: 🧹 Nitpick (assertive)Organize Import Statements
Consider organizing the import statements to follow the project's coding guidelines. Grouping imports logically (e.g., Java standard library imports, third-party library imports, and project-specific imports) can improve readability.
⛔ Skipped due to learnings
Learnt from: cremertim PR: ls1intum/Artemis#9325 File: src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java:112-119 Timestamp: 2024-10-08T15:35:42.972Z Learning: The project currently does not use DTOs and chooses to return entities directly in API responses.Learnt from: cremertim PR: ls1intum/Artemis#9325 File: src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java:112-119 Timestamp: 2024-09-18T14:21:47.831Z Learning: The project currently does not use DTOs and chooses to return entities directly in API responses.src/test/javascript/spec/component/faq/faq.component.spec.ts (4)
63-63: Duplicate Comment: Use NgMocks to Mock AccountServiceRefer to the previous comment about using
MockProviderfromng-mocksto mockAccountServicein your providers array for consistency.
207-207: Duplicate Comment: Replace Non-standard Jest Assertion with Standard MethodsRefer to the previous comment regarding the use of standard Jest matchers
toHaveBeenCalledTimes(1)andtoHaveBeenCalledWithinstead oftoHaveBeenCalledExactlyOnceWith.
215-215: Duplicate Comment: Replace Non-standard Jest Assertion with Standard MethodsRefer to the previous comment regarding the use of standard Jest matchers
toHaveBeenCalledTimes(1)andtoHaveBeenCalledWithinstead oftoHaveBeenCalledExactlyOnceWith.
224-224: Duplicate Comment: Replace Non-standard Jest Assertion with Standard MethodsRefer to the previous comment regarding the use of standard Jest matchers
toHaveBeenCalledTimes(1)andtoHaveBeenCalledWithinstead oftoHaveBeenCalledExactlyOnceWith.
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
WalkthroughThis pull request introduces several changes related to the management of FAQs within the application. A new method is added to 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
|
There was a problem hiding this comment.
Actionable comments posted: 31
🧹 Outside diff range comments (5)
src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
Line range hint
1-118: Suggestions for improved compliance with coding guidelinesThe file generally adheres to the provided coding guidelines. However, consider the following improvements:
- Implement lazy loading for this component if it's not already done at the module level.
- Ensure code reuse where possible, especially for common functionality across components.
- Implement explicit memory leak prevention measures, such as unsubscribing from all observables in the
ngOnDestroymethod.- Consider adding responsive layout features to ensure the component works well on various screen sizes.
Here's an example of how you might improve the
ngOnDestroymethod to prevent memory leaks:ngOnDestroy() { this.ngUnsubscribe.next(); this.ngUnsubscribe.complete(); this.parentParamSubscription?.unsubscribe(); this.searchInput.complete(); // Unsubscribe from any other subscriptions if (this.someOtherSubscription) { this.someOtherSubscription.unsubscribe(); } }For responsive layout, consider using Angular Flex Layout or CSS Grid/Flexbox in your template.
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)
Line range hint
72-80: LGTM: MockProvider updated correctly with a minor suggestion.The
findAllByCourseIdAndStatemethod has been correctly updated in the MockProvider for FaqService. This change aligns with the new method signature in the FaqService.Consider adding type annotations to the mock implementation for improved type safety:
findAllByCourseIdAndState: (courseId: number, state: FaqState) => { return of( new HttpResponse<Faq[]>({ body: [faq1, faq2, faq3], status: 200, }), ); },src/test/javascript/spec/component/faq/faq-update.component.spec.ts (3)
Line range hint
96-117: LGTM: Test case for creating FAQ as instructor updated correctly.The test case has been appropriately modified to check the behavior when
isAtLeastInstructoris true. The expectation correctly verifies that the FAQ is created with the 'ACCEPTED' state.Consider adding a test for the
isSavingflag at the beginning of thesave()method to ensure it's set totruebefore the API call:faqUpdateComponent.save(); expect(faqUpdateComponent.isSaving).toBeTrue(); tick();This would provide more comprehensive coverage of the component's behavior during the save process.
🧰 Tools
🪛 Biome
[error] 145-145: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Line range hint
146-169: LGTM: Test case for editing FAQ as instructor updated correctly.The test case has been appropriately modified to check the behavior when
isAtLeastInstructoris true for editing an FAQ. The expectation correctly verifies that the FAQ is updated with the 'ACCEPTED' state.For consistency with the previous test cases, consider adding expectations for the
isSavingflag:faqUpdateComponent.save(); expect(faqUpdateComponent.isSaving).toBeTrue(); tick(); faqUpdateComponentFixture.detectChanges(); expect(updateSpy).toHaveBeenCalledExactlyOnceWith(courseId, { id: 6, questionTitle: 'test1Updated', faqState: 'ACCEPTED' }); expect(faqUpdateComponent.isSaving).toBeFalse();This would provide more comprehensive coverage of the component's behavior during the save process and maintain consistency with other test cases.
🧰 Tools
🪛 Biome
[error] 172-172: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Line range hint
1-255: Overall assessment: Improved test coverage for FAQ management based on user roles.The changes to this test file significantly enhance the coverage of the FaqUpdateComponent, particularly in relation to the PR objective of allowing editors (tutors) to propose FAQs. Key improvements include:
- New test cases for creating and editing FAQs as both instructors and non-instructors.
- Verification of the correct FAQ state ('ACCEPTED' or 'PROPOSED') based on the user's role.
- Consistent use of Jest expectations that align with the provided coding guidelines.
These changes ensure that the new functionality for proposing FAQs is thoroughly tested, covering both the creation of new FAQs and the editing of existing ones. The test suite now provides a robust verification of the component's behavior under different user roles, which is crucial for the feature's reliability.
To further improve the test suite and overall code quality, consider the following:
- Implement a shared setup function for common test configurations to reduce code duplication.
- Add edge case tests, such as handling network errors or unexpected server responses.
- Consider adding integration tests that cover the interaction between the FaqUpdateComponent and related components or services.
These enhancements would contribute to a more comprehensive and maintainable test suite, further supporting the robustness of the FAQ management feature.
🧰 Tools
🪛 Biome
[error] 145-145: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
📜 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 (6 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 (4 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] 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/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)
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 (35)
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1)
15-15: LGTM: Import statement is correct and follows guidelines.The import statement for
FaqStateis correctly placed and follows Java conventions. It avoids star imports as per the coding guidelines.src/main/webapp/i18n/en/faq.json (3)
7-9: LGTM: New labels for FAQ management.The new labels "proposeLabel", "accept", and "reject" are clear, concise, and align well with the PR objectives of allowing editors to propose FAQs. Their placement in the "home" section is appropriate.
Line range hint
1-35: Summary: Comprehensive i18n updates for FAQ management feature.The changes to this file provide a complete set of internationalization strings to support the new FAQ management feature, including proposing, accepting, and rejecting FAQs. These updates align well with the PR objectives of allowing editors (tutors) to propose FAQs.
Key points:
- New labels for proposing, accepting, and rejecting FAQs have been added.
- Success messages for all FAQ management actions now include FAQ IDs for better traceability.
- A new "state" label has been added to the table section, likely for displaying the status of proposed FAQs.
These changes comprehensively support the new functionality while maintaining consistency with the existing internationalization structure.
28-29: LGTM: Added "state" to table labels.The addition of the "state" label is appropriate for displaying the status of proposed FAQs, aligning with the PR objectives.
Could you clarify the reason for reordering "categories" to be above "state"? While it doesn't affect functionality, it might impact the visual layout of the table. Please confirm if this reordering is intentional and if it aligns with the desired table structure.
src/main/webapp/i18n/de/faq.json (4)
7-9: LGTM: New labels for FAQ management.The new labels for proposing, accepting, and rejecting FAQs are correctly translated, use the required informal language (du-form), and align with the PR objectives.
13-14: LGTM: Updated messages for FAQ creation and updates.The messages for created and updated FAQs now include the FAQ ID, which improves clarity. The translations are correct and maintain the required informal language style.
28-28: LGTM: Updated label for categories.The change from singular to plural form for the categories label is appropriate and correctly translated.
29-29: LGTM: New label for FAQ state.The new label for FAQ state is correctly translated and aligns with the PR objectives of managing FAQ proposals.
src/main/webapp/app/overview/course-faq/course-faq.component.ts (2)
10-10: LGTM: Import statement correctly addedThe import for
FaqStateis correctly added and follows the coding guidelines. This import is necessary for the updated method call inloadFaqs.
75-75: LGTM: Method call updated to filter accepted FAQsThe update to
findAllByCourseIdAndStatewith the additionalFaqState.ACCEPTEDparameter aligns with the PR objectives to only show accepted FAQs. The method name correctly reflects the new functionality and follows the camelCase naming convention.To ensure the change is properly tested, please run the following script to check for corresponding test updates:
If the script doesn't return any results, consider updating the relevant test files to cover this new functionality.
✅ Verification successful
Verified: Method call and corresponding tests updated to filter accepted FAQs
The update to
findAllByCourseIdAndStatewith the additionalFaqState.ACCEPTEDparameter aligns with the PR objectives to only show accepted FAQs. The method name correctly reflects the new functionality and follows the camelCase naming convention. Additionally, the tests insrc/test/javascript/spec/service/faq.service.spec.tshave been updated to cover this new functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if tests have been updated for the new method call # Test: Search for test files that include the new method name rg --type typescript "findAllByCourseIdAndState.*FaqState\.ACCEPTED" "src/test"Length of output: 114
Script:
#!/bin/bash # Description: Retry checking if tests have been updated for the new method call with corrected file type. # Test: Search for test files that include the new method name using the correct file type. rg --type ts "findAllByCourseIdAndState.*FaqState\.ACCEPTED" "src/test"Length of output: 196
src/main/webapp/app/faq/faq.service.ts (1)
Line range hint
18-25: Verify the removal of automatic FAQ state settingThe AI summary mentions that a line setting
copy.faqStatetoFaqState.ACCEPTEDwas removed from thecreatemethod. However, this change is not visible in the provided code snippet.Please confirm if this change was made in a previous commit. If so, ensure that the FAQ state is now being set appropriately elsewhere in the codebase, aligning with the new workflow where tutors can propose FAQs for review.
Run the following script to verify the handling of
faqState:This will help us ensure that the FAQ state is being managed correctly throughout the application.
✅ Verification successful
Verified the removal of
copy.faqState = FaqState.ACCEPTEDinfaq.service.ts.faqStateis now managed appropriately in other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of faqState being set in the codebase # Test: Search for faqState assignments echo "Searching for faqState assignments:" rg --type typescript 'faqState\s*=' src/ # Test: Search for FaqState enum usage echo "Searching for FaqState enum usage:" rg --type typescript 'FaqState\.' src/Length of output: 313
Script:
#!/bin/bash # Description: Check for occurrences of faqState being set in the codebase # Test: Search for faqState assignments in .ts and .tsx files echo "Searching for faqState assignments in TypeScript files:" rg 'faqState\s*=' -g '*.ts' -g '*.tsx' src/ # Test: Search for FaqState enum usage in .ts and .tsx files echo "Searching for FaqState enum usage in TypeScript files:" rg 'FaqState\.' -g '*.ts' -g '*.tsx' src/Length of output: 2559
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 is necessary for the updated test cases that use the newFaqStateenum.
Line range hint
1-165: Overall assessment: Changes are well-implemented and consistent.The updates to this test file correctly implement the new functionality for fetching FAQs with a state parameter. The changes are consistent with the PR objectives and maintain the existing test structure. The minor suggestions provided earlier will improve type safety and readability but don't affect the overall functionality.
To ensure that all necessary changes have been made throughout the codebase, please run the following verification script:
This script will help identify any missed updates to the method name in other parts of the codebase.
src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.html (1)
Line range hint
75-80: LGTM! Verify backend permissions and update documentation.The change aligns well with the PR objective of allowing tutors (editors) to propose FAQs. The use of
@ifinstead of*ngIfcorrectly follows the new Angular syntax guideline.Please ensure the following:
- Backend permissions are updated to allow editors to interact with FAQs as intended.
- User documentation is updated to reflect this new capability for editors.
To verify backend permissions, you can run:
This will help identify any places where permissions might need to be adjusted from INSTRUCTOR to EDITOR for FAQ-related functionality.
✅ Verification successful
Backend permissions successfully verified. Both
INSTRUCTORandEDITORroles are properly enforced, ensuring that editors have the necessary access to manage FAQs as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FAQ-related permission checks in the backend code rg --type java "hasRole\(.*INSTRUCTOR.*\)" src/main/javaLength of output: 514
Script:
#!/bin/bash # Search for EDITOR-related permission checks in the backend code rg --type java "hasRole\(.*EDITOR.*\)" src/main/javaLength of output: 486
src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2)
18-18: LGTM: Import statement added correctly.The import for
FaqDTOis appropriately added and aligns with the new functionality being tested.
35-36: LGTM: Field addition is appropriate.The
faqDTOfield is correctly declared as private and follows naming conventions. It's a suitable addition for testing the new FAQ functionality.src/test/javascript/spec/service/faq.service.spec.ts (2)
Line range hint
1-255: Overall, the test suite is well-structured and comprehensive.The
faq.service.spec.tsfile contains a robust set of tests for the FaqService, covering various scenarios and methods. The newly added test case forfindAllByCourseIdAndStateintegrates well with the existing tests and follows most of our coding guidelines.A few minor improvements have been suggested for the new test case to enhance its specificity and adhere more closely to our best practices. Once these changes are implemented, the test suite will provide excellent coverage and maintainability for the FaqService.
🧰 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)
137-155: 🛠️ Refactor suggestionEnhance test specificity and avoid assignment in subscribe.
The test case for
findAllByCourseIdAndStateis well-structured and follows most of our coding guidelines. However, we can make a few improvements:
- Make the expectations more specific by checking individual properties instead of using
toEqualfor the entire object.- Avoid the assignment in the subscribe callback, as per the static analysis hint.
Here's a suggested refactor:
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) => { expect(resp.body).toBeTruthy(); expect(resp.body?.[0].id).toBe(expected[0].id); expect(resp.body?.[0].questionTitle).toBe(expected[0].questionTitle); expect(resp.body?.[0].questionAnswer).toBe(expected[0].questionAnswer); expect(resp.body?.[0].faqState).toBe(expected[0].faqState); expect(resp.body?.[0].categories).toEqual(expected[0].categories); }); const req = httpMock.expectOne({ url: `api/courses/${courseId}/faq-state/${FaqState.ACCEPTED}`, method: 'GET', }); req.flush(returnedFromService); });This refactor:
- Removes the assignment in the subscribe callback.
- Makes expectations more specific by checking individual properties.
- Uses
toBefor primitive values andtoEqualfor objects, as per our guidelines.To ensure this change doesn't break any existing functionality, please run the following command:
🧰 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/test/javascript/spec/component/faq/faq-update.component.spec.ts (1)
14-14: LGTM: Import statement for Faq model added.The import of the Faq model is correctly placed and necessary for the new test cases.
src/main/webapp/app/course/manage/overview/course-management-card.component.html (1)
Line range hint
342-351: LGTM: Change aligns with PR objectivesThe modification to allow editors (including tutors) to access the FAQ section is in line with the pull request's goal of enabling course editors to propose FAQs. This change appropriately expands access while maintaining proper role-based restrictions.
To ensure this change doesn't introduce unintended side effects, please verify:
- Editors can now access the FAQ section.
- Instructors still retain their access to the FAQ section.
- Users below the editor role (e.g., students) cannot access this section.
✅ Verification successful
: Role-based access to FAQ section correctly implemented
- The condition
course.isAtLeastEditor && course.faqEnabledis consistently applied in bothcourse-management-card.component.htmlandcourse-management-tab-bar.component.html, ensuring that editors can access the FAQ section.- Instructors retain access as intended, and users below the editor role cannot access this section.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the roles that can access the FAQ section rg --type html 'course\.isAtLeastEditor.*faqEnabled' src/main/webappLength of output: 376
src/main/webapp/app/faq/faq-update.component.ts (4)
19-19: LGTM!The import of
AccountServiceis appropriate and necessary for role-based access control.
49-49: LGTM!Injecting
AccountServiceusing theinjectfunction aligns with Angular's dependency injection practices.
62-62: LGTM!Correctly setting
isAtLeastInstructorbased on the user's role ensures proper permission handling.
84-84: LGTM!Setting
faqStateconditionally based on the user's role effectively manages FAQ proposal and acceptance.src/main/webapp/app/faq/faq.component.ts (3)
55-56: Consistency in Icon DeclarationsThe declarations of
faCancelandfaCheckare consistent with the other font-awesome icon declarations. Good job maintaining consistency in your code.
62-63: Injection of ServicesInjecting
AccountServiceandTranslateServiceusing theinjectfunction is a good practice with Angular's standalone components. This ensures proper dependency management.
2-3: Appropriate Import StatementsThe added import statements for
FaqState, font-awesome icons,AccountService,Course, andTranslateServiceare necessary for the new functionalities introduced. They follow the project's import conventions.Also applies to: 19-21
src/main/webapp/app/faq/faq.component.html (2)
129-142:⚠️ Potential issueCorrect the variable name 'isAtleastInstrucor' in the delete button condition
Again, update
isAtleastInstrucortoisAtLeastInstructorto maintain consistency and correct the typographical error. This ensures that the delete button is correctly displayed based on user roles.Apply this diff to correct the variable name:
-@if (isAtleastInstrucor) { +@if (isAtLeastInstructor) { <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> }Run the following script to ensure consistency across the codebase:
#!/bin/bash # Description: Confirm all instances of 'isAtleastInstrucor' have been corrected. # Test: Expect no matches for 'isAtleastInstrucor' after corrections. rg --type js 'isAtleastInstrucor'
112-123:⚠️ Potential issueCorrect the variable name 'isAtleastInstrucor' and verify the role condition
Consistent with earlier comments, the variable
isAtleastInstrucorshould be corrected toisAtLeastInstructor. Additionally, confirm that the role-based condition accurately reflects user permissions for accepting or rejecting FAQs.Apply this diff to correct the variable name:
-@if (faq.faqState == FaqState.PROPOSED && isAtleastInstrucor) { +@if (faq.faqState == FaqState.PROPOSED && isAtLeastInstructor) { <div class="btn-group-vertical me-1 mb-1"> <button class="btn btn-success btn-sm" (click)="acceptProposedFaq(courseId, faq)"> <fa-icon [icon]="faCheck" /> <span class="d-none d-md-inline" jhiTranslate="artemisApp.faq.home.accept"></span> </button> <button type="button" class="mt-1 btn btn-warning" id="reject-faq-{{ faq.id }}" (click)="rejectFaq(courseId, faq)"> <fa-icon [icon]="faCancel" /> <span class="d-none d-md-inline" jhiTranslate="artemisApp.faq.home.reject"></span> </button> </div> }Run the following script to ensure all instances are updated:
#!/bin/bash # Description: Verify that 'isAtleastInstrucor' is corrected in all files. # Test: After correction, expect no occurrences of the misspelled variable. rg --type js 'isAtleastInstrucor'src/test/javascript/spec/component/faq/faq.component.spec.ts (5)
12-12: ImportingFaqStatefor enhanced testingIncluding
FaqStatein the import statement is appropriate, as it is needed for the updated test cases that verify FAQ state transitions.
21-22: MockingAccountServiceappropriatelyImporting
MockAccountServiceand providing it in place ofAccountServiceensures that authentication-dependent logic is properly isolated during testing.
63-63: ProvidingMockAccountServicein test moduleThe provider configuration correctly replaces
AccountServicewithMockAccountService, which is essential for controlling authentication scenarios in tests.
67-67: MockingActivatedRoutedata correctlySetting up the
ActivatedRoutemock with course data ensures that the component initializes with the expected context, facilitating accurate testing of course-specific logic.
193-226: Comprehensive tests for accepting and rejecting FAQsThe new test cases effectively cover the scenarios for accepting and rejecting proposed FAQs. They correctly mock the
updatemethod ofFaqServiceand assert that the FAQ states change as expected. Error handling is also properly tested to ensure robustness.src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)
191-191: ValidatefaqStateParameter for Null or Empty ValuesThe
defineStatemethod does not currently handle null or emptyfaqStatevalues, which could lead to unexpected exceptions. Consider adding validation to ensure thatfaqStateis not null or empty before processing.Run the following script to identify any calls to
getAllFaqForCourseByStatuswithout a validfaqState:If any calls do not provide a valid
faqState, consider adding validation logic to handle these cases gracefully.✅ Verification successful
faqStateParameter Validation ConfirmedAll endpoint mappings correctly require a valid
faqState. No instances of missing or invalidfaqStateparameters were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to the endpoint without a `faqState`. # Test: Look for usages of the endpoint in the codebase. Expect: All calls provide a valid `faqState`. rg --type java '@GetMapping\("courses/\{courseId\}/faq-state/\{faqState\}"\)'Length of output: 210
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
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java
Outdated
Show resolved
Hide resolved
flbrgit
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
…q' into feature/communication/propose-faq
cremertim
dismissed stale reviews from coderabbitai[bot], ahbitaqu, HawKhiem, florian-glombik, and SimonEntholzer
via
a3118f5
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/faq/faq-update.component.ts (3)
62-62: LGTM with suggestion: Role-based access control implementationThe implementation of role-based access control using
AccountServiceis correct. However, consider extracting this logic into a separate method for improved readability and maintainability.Suggestion:
private setUserRole(course: any): void { this.isAtLeastInstructor = this.accountService.isAtLeastInstructorInCourse(course); }Then call this method in
ngOnInit:if (course) { this.faq.course = course; this.loadCourseFaqCategories(course.id); this.setUserRole(course); }
84-84: LGTM with suggestion: Conditional FAQ state settingThe conditional setting of
faqStatebased on the user's role is correct and aligns with the new role-based access control functionality. To improve clarity, consider using a ternary operator:this.faq.faqState = this.isAtLeastInstructor ? FaqState.ACCEPTED : FaqState.PROPOSED;This change makes the conditional assignment more concise and easier to read.
124-134: LGTM with suggestions: Centralized success alert handlingThe
showSuccessAlertmethod effectively centralizes the logic for displaying success messages based on the user's role. This improves code maintainability and readability. However, there are a few suggestions for improvement:
- Remove the unnecessary
console.logstatement on line 130.- Consider using template literals for better readability when constructing the message key:
messageKey = this.isAtLeastInstructor ? `artemisApp.faq.${newlyCreated ? 'created' : 'updated'}` : `artemisApp.faq.${newlyCreated ? 'proposed' : 'proposedChange'}`;
- The
newlyCreatedparameter seems to be used inversely to its name. Consider renaming it toisExistingor inverting its usage for clarity.src/main/webapp/app/faq/faq.component.ts (1)
37-38: Remove unnecessary type annotationFor consistency and to follow the suggestion from the past review, remove the type annotation for
hasCategories:-hasCategories: boolean = false; +hasCategories = false;This change aligns with the style of
isAtLeastInstructorand removes redundant type information.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/main/webapp/app/faq/faq-update.component.ts (5 hunks)
- src/main/webapp/app/faq/faq.component.ts (5 hunks)
- src/main/webapp/i18n/de/faq.json (2 hunks)
- src/main/webapp/i18n/en/faq.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/i18n/de/faq.json
- src/main/webapp/i18n/en/faq.json
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/app/faq/faq.component.ts (1)
🔇 Additional comments (12)
src/main/webapp/app/faq/faq-update.component.ts (5)
19-19: LGTM: Import of AccountServiceThe addition of the AccountService import is appropriate for implementing role-based access control in the component.
35-35: Remove unnecessary type annotationThe type annotation for
isAtLeastInstructoris redundant as TypeScript can infer the type from the initialization. Remove it to improve code readability.Apply this fix:
-isAtLeastInstructor: boolean = false; +isAtLeastInstructor = false;
39-41: LGTM: Readonly icon declarationsThe use of
readonlyfor icon declarations is a good practice. It prevents accidental reassignment and aligns with TypeScript best practices for constant values.
49-49: LGTM: AccountService injectionThe injection of AccountService using the
injectfunction follows Angular's recommended dependency injection pattern and is consistent with other service injections in the component.
Line range hint
1-173: Overall assessment: Well-implemented role-based access control for FAQ managementThe changes in this file effectively implement role-based access control for FAQ management, aligning with the PR objectives. The code is generally well-structured and follows Angular best practices. Minor suggestions have been made to improve code clarity, consistency, and maintainability.
Key improvements:
- Proper integration of AccountService for role checks.
- Conditional FAQ state setting based on user roles.
- Centralized success alert handling.
Consider addressing the suggested improvements in the individual comments to further enhance the code quality.
src/main/webapp/app/faq/faq.component.ts (7)
2-4: LGTM: New imports added correctlyThe new imports are correctly added and necessary for the new functionality being implemented. They follow the coding guidelines for imports.
Also applies to: 19-20
31-31: LGTM: FaqState property added correctlyThe addition of the protected readonly FaqState property is correct and follows the coding guidelines. It allows the use of FaqState in the template while ensuring it cannot be modified.
33-33: LGTM: Course property addedThe addition of the 'course' property of type Course is appropriate and follows the coding guidelines for property naming.
42-42: LGTM: routeDataSubscription property addedThe addition of the private
routeDataSubscriptionproperty is a good practice for managing subscriptions and preventing memory leaks. It follows the coding guidelines for property naming.
50-57: LGTM: Icon properties added as protected readonlyThe new icon properties are correctly added as protected and readonly, aligning with the suggestion from the past review. This ensures that the icons cannot be modified accidentally and follows good encapsulation practices.
78-84: LGTM: Course and user role initialization added to ngOnInitThe changes in
ngOnInitcorrectly initialize the course and user role information. The subscription to route data is properly managed using therouteDataSubscriptionproperty, following Angular best practices for component initialization and data management.
165-184: LGTM: FAQ state update methods refactoredThe refactoring of
updateFaqState,rejectFaq, andacceptProposedFaqmethods is well-implemented, reducing code duplication as suggested in the past review. The methods follow the coding guidelines for naming and structure.However, please verify if the
alertService.successmethod already handles translation. If it does, you can simplify the call by removing thetranslateService:next: () => this.alertService.success('artemisApp.faq.' + (newState === FaqState.REJECTED ? 'rejected' : 'accepted'), { title: faq.questionTitle }),This change would align with the suggestion to use alertService methods that already handle translation.
SindiBuklaji
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
cremertim
dismissed stale reviews from SindiBuklaji and coderabbitai[bot]
via
ef1123c
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 tutors 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
Check, that a tutor is not able to edit a accepted FAQ
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
Tutor View
Instructor View
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation