Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Communication: Only show accepted categories of accepted FAQs #9591

Merged
merged 12 commits into from
Nov 2, 2024

Conversation

cremertim
Copy link
Contributor

@cremertim cremertim commented Oct 25, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

Currently, all categories are shown in the course overview. This is bad, cause a tutor can propose a new category for FAQs and students could filter for it without having an accepted FAQ --> they see nothing

Description

I added a endpoint for retrieving the FAQ categories per state and adapted the client side to retrieve accepted FAQs

Steps for Testing

Prerequisites:

  • 1 Tutor
  • 1 Student
  • 1 Course with FAQ enabled
  1. Log in to Artemis as tutor
  2. Navigate to Course Management
  3. Create a FAQ with a category, that is completly new
  4. Go to course overview / faqs
  5. See that this category is not in the filter

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.







Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced FAQ querying capabilities with new methods to retrieve categories and specific FAQs based on course ID and state.
    • Role-based access control for creating and updating FAQs, ensuring only authorized users can modify accepted FAQs.
    • Introduction of a dynamic faqState property in the Course FAQ component to improve FAQ management flexibility.
  • Bug Fixes

    • Improved test coverage for FAQ retrieval based on user roles and states, ensuring accurate access control.
  • Documentation

    • Updated method names and signatures for clarity and consistency across the FAQ service and resources.

@cremertim cremertim requested a review from a team as a code owner October 25, 2024 09:10
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) communication Pull requests that affect the corresponding module labels Oct 25, 2024
Copy link

coderabbitai bot commented Oct 25, 2024

Walkthrough

The pull request introduces enhancements to the FAQ functionality by adding new query methods in the FaqRepository, allowing retrieval of FAQ categories and specific FAQs based on their state. The FaqResource class is updated to enforce role checks for creating and updating FAQs, with new methods for retrieving FAQs based on user roles. The FaqService and utility functions are modified to support these changes, while integration tests are updated to reflect the new role-based access logic.

Changes

File Path Change Summary
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java - Added method findAllCategoriesByCourseIdAndState for retrieving categories by course ID and state.
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java - Renamed method getFaqForCourse to getFaqsForCourse.
- Added method getFaqCategoriesForCourseByState for fetching categories by state.
- Added role check methods.
src/main/webapp/app/faq/faq.service.ts - Added method findAllCategoriesByCourseIdAndCategory for fetching categories by course ID and state.
src/main/webapp/app/faq/faq.utils.ts - Updated loadCourseFaqCategories to include an optional faqState parameter for conditional fetching.
src/main/webapp/app/overview/course-faq/course-faq.component.ts - Added property faqState and updated methods to utilize this property for dynamic FAQ state handling.
src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java - Renamed and modified existing test methods to reflect role-based access changes.
- Added new test methods to validate access based on user roles and FAQ states.
src/test/javascript/spec/service/faq.service.spec.ts - Added a test case for the new findAllCategoriesByCourseIdAndCategory method in FaqService.
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts - Updated test case to use findAllCategoriesByCourseIdAndCategory for error handling when no categories are found.

Possibly related PRs

  • Communication: Add FAQs to Artemis #9325: The FaqRepository was modified to include a method for retrieving FAQs by course ID and state, which relates directly to the changes made in the main PR that added a method to enhance querying capabilities in the same repository.
  • Communication: Add FAQ search bar #9423: This PR introduces a search bar for FAQs, which is directly related to the main PR's changes in the FaqResource and FaqService that enhance the management and retrieval of FAQs.
  • Communication: Allow tutors to propose FAQ #9477: The changes in this PR allow tutors to propose FAQs, which connects to the main PR's modifications in the FaqRepository and FaqResource that involve managing FAQs based on their state and course ID.
  • Communication: Remove announcements from unresolved filter #9561: This PR modifies the filtering logic for conversations, which is relevant to the overall communication features that include FAQs, as both involve managing user interactions with course-related content.
  • Communication: Reduce FAQ size #9568: The changes in this PR focus on reducing the size of the FAQ display, which complements the main PR's enhancements to the FAQ management system by improving user experience and interface design.

Suggested labels

ready to merge, bugfix

Suggested reviewers

  • JohannesStoehr
  • florian-glombik
  • az108
  • sarpsahinalp
  • rabeatwork
  • Malekos74

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Outside diff range and nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1)

34-39: Consider adding database indexes and documentation.

For optimal query performance:

  1. Consider adding database indexes on frequently queried columns (course.id, faqState).
  2. Add Javadoc to document the query's purpose and expected behavior, especially for complex queries like this one.
src/main/webapp/app/faq/faq.service.ts (2)

157-161: Add return type annotation and documentation.

Consider adding TypeScript return type annotation and JSDoc documentation for consistency with other methods:

+    /**
+     * Retrieves FAQ categories for a specific course filtered by FAQ state
+     * @param courseId - The ID of the course
+     * @param faqState - The state to filter categories by
+     * @returns Observable of FAQ categories response
+     */
-    findAllCategoriesByCourseIdAndCategory(courseId: number, faqState: FaqState) {
+    findAllCategoriesByCourseIdAndCategory(courseId: number, faqState: FaqState): Observable<HttpResponse<string[]>> {

157-161: Consider subscription management.

When consuming this Observable in components, ensure proper subscription management to prevent memory leaks. Consider using either:

  • The async pipe in templates
  • Implementing ngOnDestroy to unsubscribe
  • Using takeUntil or other cleanup operators
src/test/javascript/spec/service/faq.service.spec.ts (2)

178-183: Enhance test coverage with more comprehensive test data.

The test only validates a single category scenario. Consider adding test cases for:

  • Multiple categories
  • Empty categories array
  • Categories with different states
         it('should find all categories by courseId and faqState', () => {
-            const category = {
-                color: '#6ae8ac',
-                category: 'category1',
-            } as FaqCategory;
-            const returnedFromService = { categories: [JSON.stringify(category)] };
+            const categories = [
+                { color: '#6ae8ac', category: 'category1' } as FaqCategory,
+                { color: '#ff0000', category: 'category2' } as FaqCategory
+            ];
+            const returnedFromService = { 
+                categories: categories.map(cat => JSON.stringify(cat))
+            };

194-195: Use more specific assertions.

The current assertion only checks for equality. Consider adding more specific assertions following the coding guidelines.

             req.flush(returnedFromService);
-            expect(expectedResult.body).toEqual(expected);
+            expect(expectedResult.body).toContainEntries(expected);
+            expect(req.request.method).toBe('GET');
+            expect(req.request.url).toBe(`api/courses/${courseId}/faq-categories/${faqState}`);
src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2)

183-187: Enhance assertion coverage and remove redundant test.

The test verifies only the size of returned FAQs. Consider:

  1. Adding assertions for content equality
  2. Removing the redundant test method getFaq_ShouldGetFaqByCourseId (lines 199-202) as it tests the same functionality
 @Test
 @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
 void getFaq_InstructorsShouldGetAllFaqByCourseId() throws Exception {
     Set<Faq> faqs = faqRepository.findAllByCourseId(this.course1.getId());
     Set<FaqDTO> returnedFaqs = request.get("/api/courses/" + course1.getId() + "/faqs", HttpStatus.OK, Set.class);
     assertThat(returnedFaqs).hasSize(faqs.size());
+    assertThat(returnedFaqs).containsExactlyInAnyOrderElementsOf(faqs);
 }

Line range hint 183-228: Enhance test coverage with additional scenarios.

Consider adding the following test cases to improve coverage:

  1. Test behavior when no FAQs exist
  2. Test with empty categories
  3. Test with special characters in FAQ titles/answers
  4. Test pagination if supported
  5. Test error cases (e.g., invalid course ID)

Also, consider organizing related tests using nested test classes (JUnit 5 @Nested) to group:

  • Student access tests
  • Instructor access tests
  • State-specific tests
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (3)

208-208: Rename variable faqs to categories for clarity

The variable faqs holds a set of FAQ categories (Set<String>). Renaming it to categories improves readability and accurately reflects its content.

Apply this change:

- Set<String> faqs = faqRepository.findAllCategoriesByCourseId(courseId);
+ Set<String> categories = faqRepository.findAllCategoriesByCourseId(courseId);

Also, update the variable usage in the return statement:

- return ResponseEntity.ok().body(faqs);
+ return ResponseEntity.ok().body(categories);

216-216: Rename variable faqs to categories for clarity

Similar to the previous method, the variable faqs here represents a set of category names. Renaming it to categories enhances clarity.

Apply this change:

- Set<String> faqs = faqRepository.findAllCategoriesByCourseIdAndState(courseId, faqState);
+ Set<String> categories = faqRepository.findAllCategoriesByCourseIdAndState(courseId, faqState);

Also, update the variable usage in the return statement:

- return ResponseEntity.ok().body(faqs);
+ return ResponseEntity.ok().body(categories);

224-224: Fix typo in JavaDoc comment

In the JavaDoc comment for checkRoleForCourse, correct the typo "atleast" to "at least".

Apply this change:

-  * @throws AccessForbiddenException if the user is not atleast role
+  * @throws AccessForbiddenException if the user is not at least role
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c200372 and 05b3f9b.

📒 Files selected for processing (7)
  • src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (6 hunks)
  • src/main/webapp/app/faq/faq.service.ts (1 hunks)
  • src/main/webapp/app/faq/faq.utils.ts (1 hunks)
  • src/main/webapp/app/overview/course-faq/course-faq.component.ts (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2 hunks)
  • src/test/javascript/spec/service/faq.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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_imports

src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/webapp/app/faq/faq.service.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/faq/faq.utils.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

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: true

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/test/javascript/spec/service/faq.service.spec.ts

[error] 189-189: 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 (13)
src/main/webapp/app/faq/faq.utils.ts (3)

7-7: LGTM!

The import of FaqState is correctly placed and necessary for the new functionality.


9-9: LGTM!

The function signature change correctly introduces the optional faqState parameter while maintaining backward compatibility.


9-25: Verify the usage of FaqState parameter.

The implementation looks correct, but let's verify that it's being used with the correct state to show only accepted categories.

✅ Verification successful

Usage of FaqState.ACCEPTED is correctly implemented

The verification confirms that:

  • In course-faq.component.ts, the component correctly sets faqState = FaqState.ACCEPTED and passes it to loadCourseFaqCategories
  • The faq.component.ts uses FaqState.ACCEPTED appropriately when accepting FAQs
  • The faq-update.component.ts sets the state to ACCEPTED for instructors
  • Test files verify the behavior with FaqState.ACCEPTED
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the function is called with ACCEPTED state
# where appropriate in the codebase

# Check for imports and usage of loadCourseFaqCategories
rg -A 5 "loadCourseFaqCategories" --type ts

# Check for FaqState.ACCEPTED usage
rg "FaqState\.ACCEPTED" --type ts

Length of output: 7597

src/main/java/de/tum/cit/aet/artemis/communication/repository/FaqRepository.java (1)

34-39: LGTM! Well-structured JPQL query implementation.

The query is correctly implemented with proper parameter annotations and follows SQL naming conventions. The use of Set<String> for distinct categories is appropriate for preventing duplicates.

src/main/webapp/app/overview/course-faq/course-faq.component.ts (3)

35-35: LGTM! Property addition aligns with PR objective.

The faqState property is correctly initialized with FaqState.ACCEPTED, ensuring that only accepted FAQs are displayed by default.


Line range hint 76-84: LGTM! Robust implementation with proper error handling.

The modification correctly uses faqState to filter FAQs while maintaining proper error handling and following Angular best practices.


35-35: Verify consistent FAQ state handling across the application.

Let's verify that the FaqState.ACCEPTED is consistently used across the application and that there are no hardcoded state values elsewhere.

Also applies to: 76-76

✅ Verification successful

FAQ state handling is consistently implemented across the application

The verification shows that FaqState enum is used consistently throughout the codebase:

  • Frontend components (course-faq.component.ts, faq.component.ts, faq-update.component.ts) properly use the enum
  • Backend (FaqResource.java) correctly handles state transitions and permissions
  • No hardcoded state values were found
  • Test files demonstrate proper usage of the enum for different states (ACCEPTED, PROPOSED, REJECTED)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for FaqState usage to ensure consistency
echo "Checking for FaqState usage patterns:"
rg "FaqState\." -A 2

# Search for potential hardcoded state values
echo "\nChecking for potential hardcoded state values:"
rg -i "state.*=.*['\"]accepted['\"]"

Length of output: 12068

src/main/webapp/app/faq/faq.service.ts (1)

157-161: Verify the new endpoint integration.

The method implements the PR objective to filter FAQ categories by state. Let's verify the endpoint integration:

✅ Verification successful

Let me gather more information to verify the endpoint integration completely.


Let me try one more verification with modified commands:


Endpoint integration is properly implemented and tested

The verification shows:

  • The endpoint is correctly implemented in the service with proper typing (FaqState enum)
  • The service is covered by unit tests in faq.service.spec.ts
  • The endpoint is utilized in faq.utils.ts with proper response handling
  • The FaqState enum is properly defined in the backend with matching values (ACCEPTED, REJECTED, PROPOSED)
  • The integration follows the same pattern as other endpoints in the codebase

The implementation aligns with the PR objective to filter FAQ categories by state.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the endpoint implementation and usage
# Test 1: Check if the endpoint is properly defined in the backend
rg -t java "faq-categories/.*FaqState" 

# Test 2: Check for proper error handling of the endpoint in components
rg -t typescript "findAllCategoriesByCourseIdAndCategory.*catch"

# Test 3: Verify consistent usage of FaqState enum
ast-grep --pattern 'FaqState.$$$'

Length of output: 2058


Script:

#!/bin/bash
# Check the backend endpoint implementation
rg -t java "courses/.*faq-categories.*FaqState" -A 5

# Check the service test coverage
rg -t typescript "findAllCategoriesByCourseIdAndCategory" src/test/javascript/spec/service/faq.service.spec.ts -A 5

# Check the component usage
rg -t typescript "findAllCategoriesByCourseIdAndCategory" src/main/webapp/app/ -A 2

Length of output: 324


Script:

#!/bin/bash
# Check the backend endpoint implementation
rg "courses/.*faq-categories.*FaqState" -A 5

# Check for the service test coverage and component usage
rg "findAllCategoriesByCourseIdAndCategory" -A 3

# Check the FaqState enum definition
fd -e ts -e java FaqState --exec cat {}

Length of output: 1530

src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (5)

139-139: Role check for accessing non-accepted FAQs is appropriate

The call to checkShouldAccessNotAccepted(faq.getFaqState(), courseId); ensures that users have the required role when accessing FAQs that are not in the ACCEPTED state. This enforces proper access control based on FAQ state.


172-172: Method name updated to reflect plurality

Renaming the method to getFaqsForCourse improves clarity by indicating that it returns a collection of FAQs rather than a single FAQ. This enhances readability and aligns with naming conventions.


174-175: Properly restrict FAQs based on user role

The logic correctly ensures that users who are at least teaching assistants receive all FAQs, while other users receive only FAQs with the state ACCEPTED. This implementation aligns with the access control requirements.


191-191: Role validation when accessing FAQs by state

The call to checkShouldAccessNotAccepted(faqState, courseId); in getAllFaqsForCourseByStatus enforces role checks when users attempt to access FAQs that are not ACCEPTED. This ensures proper access control based on user roles.


232-236: Method checkShouldAccessNotAccepted implements correct access control

The checkShouldAccessNotAccepted method correctly enforces that users must have at least the TEACHING_ASSISTANT role to access FAQs that are not in the ACCEPTED state.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (2)

212-219: Add Javadoc for the new endpoint

The implementation looks good, but please add Javadoc documentation similar to other endpoints in the class.

+/**
+ * GET /courses/:courseId/faq-categories/:faqState : get all faq categories of a course for the specified state
+ *
+ * @param courseId the courseId of the course for which faq categories should be returned
+ * @param faqState the state of FAQs whose categories should be returned
+ * @return the ResponseEntity with status 200 (OK) and the list of categories in body
+ */
 @GetMapping("courses/{courseId}/faq-categories/{faqState}")

232-236: Add Javadoc for the helper method

Please add Javadoc documentation to maintain consistency with other methods in the class.

+/**
+ * Checks if the user should be allowed to access FAQs with non-accepted state
+ *
+ * @param faqState the state of the FAQ being accessed
+ * @param courseId the id of the course the FAQ belongs to
+ * @throws AccessForbiddenException if the user does not have required role for accessing non-accepted FAQs
+ */
 private void checkShouldAccessNotAccepted(FaqState faqState, Long courseId) {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 05b3f9b and 4bfbc7e.

📒 Files selected for processing (3)
  • src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (6 hunks)
  • src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2 hunks)
  • src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

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: true

src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (6)
src/test/javascript/spec/component/overview/course-faq/course-faq.component.spec.ts (1)

155-155: LGTM: Method name updated correctly.

The update to use findAllCategoriesByCourseIdAndCategory aligns with the PR's objective to filter FAQ categories based on their acceptance status.

src/test/java/de/tum/cit/aet/artemis/communication/FaqIntegrationTest.java (2)

189-197: LGTM! Well-structured test with comprehensive state verification.

The test properly verifies that students can only access accepted FAQs and explicitly checks that proposed and rejected FAQs are not included.


215-222: Refer to previous review comment.

src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (3)

140-140: LGTM! Access control properly implemented

The access check ensures that only users with appropriate roles can access non-accepted FAQs.


173-176: LGTM! Improved method naming and role-based filtering

The changes enhance clarity through:

  1. More accurate method name (plural form for collection return type)
  2. Clear role-based filtering logic using the ternary operator

221-230: LGTM! Well-documented helper method

The helper method is properly implemented with clear documentation and good reuse of existing functionality.

sachmii
sachmii previously approved these changes Oct 25, 2024
Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS2. Created a FAQ as a instructor with category "new category" and proposed a FAQ as a tutor with another new category. Could not filter for it, works fine.

Bildschirmfoto 2024-10-25 um 14 36 08

HawKhiem
HawKhiem previously approved these changes Oct 25, 2024
Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS2. I created created faq with the new category (super new cat). Indeed it doesn't appear in the filter
Screenshot 2024-10-25 205235
Screenshot 2024-10-25 205304

flbrgit
flbrgit previously approved these changes Oct 28, 2024
Copy link

@flbrgit flbrgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS6. Everything works as expected. After creating new FAQs with new Categories, those were not shown in the filter
Screenshot 2024-10-28 200509
Screenshot 2024-10-28 200530

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 28, 2024
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)

206-213: Add Javadoc for the new endpoint.

The new getFaqCategoriesForCourseByState endpoint should have proper Javadoc documentation like other endpoints in the class.

Add this documentation:

+    /**
+     * GET /courses/:courseId/faq-categories/:faqState : get all the faq categories of a course with the specified state
+     *
+     * @param courseId the courseId of the course for which all faq categories should be returned
+     * @param faqState the state of FAQs whose categories should be returned
+     * @return the ResponseEntity with status 200 (OK) and the list of faq categories in body
+     */
     @GetMapping("courses/{courseId}/faq-categories/{faqState}")
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c2f4924 and b15d8d3.

📒 Files selected for processing (1)
  • src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/web/FaqResource.java (2)

221-236: LGTM! Well-structured role check methods.

The implementation follows good practices:

  • Clear separation of concerns
  • Descriptive method names
  • Proper error handling through exceptions

169-170: 🛠️ Refactor suggestion

Improve readability by avoiding ternary operator.

The current implementation using a ternary operator makes the access control logic less readable.

Consider using a more explicit if-else structure:

-        Set<Faq> faqs = authCheckService.isAtLeastTeachingAssistantInCourse(courseId) ? faqRepository.findAllByCourseId(courseId)
-                : faqRepository.findAllByCourseIdAndFaqState(courseId, FaqState.ACCEPTED);
+        Set<Faq> faqs;
+        if (authCheckService.isAtLeastTeachingAssistantInCourse(courseId)) {
+            faqs = faqRepository.findAllByCourseId(courseId);
+        } else {
+            faqs = faqRepository.findAllByCourseIdAndFaqState(courseId, FaqState.ACCEPTED);
+        }

Likely invalid or redundant comment.

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Copy link
Contributor

@florian-glombik florian-glombik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Approve, failing tests seem unrelated

Copy link

@SindiBuklaji SindiBuklaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS6. Everything works as expected. 👍

Copy link

@Feras797 Feras797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested on TS6, only the accepted FAQ categories are visible in the filter.
image

@krusche krusche merged commit a4a56db into develop Nov 2, 2024
32 of 35 checks passed
@krusche krusche deleted the bugfix/communication/show-only-accepted-categories branch November 2, 2024 06:46
@krusche krusche added this to the 7.6.5 milestone Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) communication Pull requests that affect the corresponding module ready for review ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.