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

Programming exercises: Add error categories and categorize feedback in grading analysis #9622

Merged

Conversation

az108
Copy link
Contributor

@az108 az108 commented Oct 28, 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

This is a follow-up to the feature I’m working on: #9315. The goal is to provide instructors with clearer insights into the feedback given to students. In this PR, error categories have been introduced, and a message is displayed when the table is empty. Additionally, minor adjustments were made: pagination now remains within bounds, and the previously introduced taskNumber was replaced with taskName, as it is more useful.

Description

The query was updated to accept filterErrorCategorie for more accurate filtering. Additionally, the taskNumber logic was completely removed and replaced by taskName, which is now stored directly in the database. All columns, except for Error Category, are now sortable.
Additionally a new shared component was introduced which allows sorting columns with a double arrow icon and depending on how we sort the column the remaining column arrows will be put as inactive and set to grey.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • Student Sumbissions
  • 1 Programming Exercise
  1. Log in to Artemis
  2. Navigate to a Course with Instructor rights
  3. Create a Programming Exercise or use an existing one
  4. Modify the Tests by adding two tests which fail per default and have a error Message starting with "Ares Security Error" and "Unwanted Statement Found"
  5. Solve the Programming Exercise as Students, ideally with different kinds of Progress (You can use artemis test users for this)
  6. Navigate as Instructor to the Grading of an Exercise
  7. Navigate to the Feedback Analysis section
  8. Verify that the table displays data correctly, allowing you to search and sort and filter as expected. Ensure that different error Categories are displayed for the different Error Categories, and confirm that the modals displays all information accurately.
  9. Verify if there is no feedback in the table or if the filter is too strong that a message, stating that the table is empty is displayed.

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

  • 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

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (expect)
feedback-analysis.component.ts 98%
feedback-analysis.service.ts 100%
feedback-filter-modal.component.ts 93%
feedback-modal.component.ts 100%
Methods in ResultService 91%
Methods in ResultResource 100%
FeedbackDetailDTO 100%
FeedbackAnalysisResponseDTO 100%

Screenshots

Before:
image
image
image

After:
image
image
image

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced feedback analysis with new filtering options for error categories.
    • Introduced sorting functionality with visual indicators for sorting direction.
    • Added support for displaying task names instead of task numbers in feedback details.
    • Improved filtering capabilities in the feedback filter modal, including error category selection.
    • Updated user interface texts for better clarity in feedback analysis.
  • Bug Fixes

    • Improved handling of empty states in feedback tables.
  • Documentation

    • Updated method documentation for clarity on feedback retrieval and filtering capabilities.
  • Localization

    • Added new messages and tooltips for improved user experience in feedback analysis.
  • Tests

    • Expanded test coverage for feedback retrieval and filtering functionalities, including error categories.

@az108 az108 self-assigned this Oct 28, 2024
@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!) assessment Pull requests that affect the corresponding module core Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module labels Oct 28, 2024
@az108 az108 added this to the 7.6.5 milestone Oct 28, 2024
@az108 az108 marked this pull request as ready for review October 28, 2024 21:37
@az108 az108 requested a review from a team as a code owner October 28, 2024 21:37
Copy link

coderabbitai bot commented Oct 28, 2024

Walkthrough

The pull request introduces significant modifications across multiple files, primarily focusing on enhancing the feedback analysis functionality in the application. Key changes include the restructuring of data transfer objects, particularly the FeedbackAnalysisResponseDTO, to include new parameters for task names and error categories. The FeedbackDetailDTO has also been updated for clarity. Additionally, enhancements to filtering capabilities have been implemented in various services and components, alongside updates to the user interface for better interaction with feedback data.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackAnalysisResponseDTO.java Updated record to remove totalAmountOfTasks and add taskNames (Set) and errorCategories (List).
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackDetailDTO.java Renamed taskNumber to taskName.
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackPageableDTO.java Added filterErrorCategories (List) with corresponding getter and setter methods.
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java Enhanced getFeedbackDetailsOnPage method documentation and logic to include filtering by error categories and task names.
src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java Updated documentation for getFeedbackDetailsPaged method to clarify its functionality and response structure.
src/main/java/de/tum/cit/aet/artemis/core/util/PageUtil.java Added new entry for taskName in ColumnMapping enum for SQL subquery.
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java Updated findFilteredFeedbackByExerciseId method to include filterErrorCategories for enhanced feedback filtering.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.html Added new section for filtering by error categories with checkboxes.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.ts Updated FilterData interface to include errorCategories, modified state management and filter methods accordingly.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.html Updated table structure and logic for displaying task names and handling empty states.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.ts Introduced new signals for errorCategories and taskNames, updated loadData method to manage new state.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.service.ts Modified FeedbackAnalysisResponse and FeedbackDetail interfaces to replace totalAmountOfTasks with taskNames and taskNumber with taskName.
src/main/webapp/app/shared/sort/sort-icon.component.html Introduced new HTML structure for sorting icons.
src/main/webapp/app/shared/sort/sort-icon.component.scss Added new CSS class .sort-icon.
src/main/webapp/app/shared/sort/sort-icon.component.ts Introduced SortIconComponent with input property direction to manage sorting icons.
src/main/webapp/i18n/de/programmingExercise.json Added new error messages and tooltips for feedback analysis.
src/main/webapp/i18n/en/programmingExercise.json Added new error messages and tooltips for feedback analysis.
src/test/java/de/tum/cit/aet/artemis/assessment/ResultServiceIntegrationTest.java Updated tests to include filterErrorCategories in feedback retrieval.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts Modified mocks and assertions to reflect new data structure and error categories.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts Updated mocks to reflect changes in property names and added errorCategories to filters.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-filter-modal.component.spec.ts Added tests for new errorCategories functionality in filter modal.
src/test/javascript/spec/util/shared/sort-icon.component.spec.ts Introduced new test suite for SortIconComponent with assertions for sorting direction.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-modal.component.spec.ts Updated mock data to reflect renaming of taskNumber to taskName.

Possibly related PRs

Suggested labels

programming, bugfix

Suggested reviewers

  • egekurt123
  • pzdr7
  • JohannesStoehr
  • FelixTJDietrich

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

🧹 Outside diff range and nitpick comments (23)
src/main/webapp/app/shared/sort/sort-icon.component.html (1)

1-4: The sort icon implementation looks good but could be enhanced.

The implementation is clean and functional. However, there are a few improvements that could make it more maintainable and accessible:

Consider applying these enhancements:

-<div class="sort-icon">
-    <fa-icon [icon]="faSortUp" [style.color]="isAscending() ? 'black' : 'grey'"></fa-icon>
-    <fa-icon [icon]="faSortDown" [style.color]="isDescending() ? 'black' : 'grey'"></fa-icon>
+<div class="sort-icon" role="img" aria-label="Sort direction indicator">
+    <fa-icon 
+        [icon]="faSortUp" 
+        [class.active]="isAscending()"
+        aria-hidden="true">
+    </fa-icon>
+    <fa-icon 
+        [icon]="faSortDown" 
+        [class.active]="isDescending()"
+        aria-hidden="true">
+    </fa-icon>
 </div>

Add these styles to your component's CSS:

.sort-icon {
    fa-icon {
        color: var(--inactive-sort-color, grey);
        
        &.active {
            color: var(--active-sort-color, black);
        }
    }
}

These changes:

  1. Move inline styles to CSS classes for better maintainability
  2. Use CSS variables for colors, making theme changes easier
  3. Add ARIA attributes for better accessibility
  4. Improve code readability with proper formatting
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackAnalysisResponseDTO.java (1)

11-12: Consider adding JavaDoc documentation.

The record structure looks good and follows DTO best practices. Consider adding JavaDoc to document the purpose of each field, especially the new taskNames and errorCategories fields, to improve maintainability.

Example:

/**
 * DTO for feedback analysis response containing paginated feedback details and filtering options.
 *
 * @param feedbackDetails Paginated feedback details
 * @param totalItems Total number of items across all pages
 * @param taskNames Set of unique task names available for filtering
 * @param testCaseNames List of test case names available for filtering
 * @param errorCategories List of error categories available for filtering
 */
src/main/webapp/app/shared/sort/sort-icon.component.ts (1)

13-21: LGTM! Clean implementation with room for optimization.

The component implementation follows best practices with proper typing and computed properties. Consider using signals for better change detection performance.

Optional optimization:

-    isAscending = computed(() => this.direction() === 'asc');
-    isDescending = computed(() => this.direction() === 'desc');
+    readonly isAscending = computed(() => this.direction() === 'asc');
+    readonly isDescending = computed(() => this.direction() === 'desc');
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackPageableDTO.java (2)

51-53: Consider adding null check for consistency.

The getSearchTerm() method includes null handling, but getFilterErrorCategories() doesn't. Consider adding similar null handling for consistency across the DTO.

 public List<String> getFilterErrorCategories() {
-    return filterErrorCategories;
+    return filterErrorCategories != null ? filterErrorCategories : List.of();
 }

Line range hint 1-58: Consider converting to a Java record and adding validation.

Since this is a DTO class used for transferring search parameters:

  1. Consider converting it to a Java record for immutability and automatic getter generation:
public record FeedbackPageableDTO(
    List<String> filterTasks,
    List<String> filterTestCases,
    String[] filterOccurrence,
    String searchTerm,
    List<String> filterErrorCategories
) extends PageableSearchDTO<String> {
    public FeedbackPageableDTO {
        // Defensive copying for mutable fields
        filterTasks = filterTasks != null ? List.copyOf(filterTasks) : List.of();
        filterTestCases = filterTestCases != null ? List.copyOf(filterTestCases) : List.of();
        filterErrorCategories = filterErrorCategories != null ? List.copyOf(filterErrorCategories) : List.of();
        filterOccurrence = filterOccurrence != null ? filterOccurrence.clone() : new String[0];
        searchTerm = searchTerm != null ? searchTerm : "";
    }
}
  1. If keeping the current class structure, consider adding constructor validation and defensive copying to prevent potential NPEs and mutability issues.
src/test/javascript/spec/util/shared/sort-icon.component.spec.ts (1)

18-37: Add test case for invalid direction value.

While the existing test cases cover the valid scenarios well, we should also test how the component handles invalid direction values.

Add this test case:

it('should handle invalid direction value by setting both flags to false', () => {
    fixture.componentRef.setInput('direction', 'invalid');
    fixture.detectChanges();
    expect(component.isAscending()).toBeFalse();
    expect(component.isDescending()).toBeFalse();
});
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.service.ts (1)

33-34: Consider explicit empty array handling for filters.

While the implementation is correct, consider adding explicit handling for empty filter arrays to improve clarity and prevent potential issues with backend interpretation.

Consider this enhancement:

-            .set('filterErrorCategories', options.filters.errorCategories.join(','));
+            .set('filterErrorCategories', options.filters.errorCategories?.length ? options.filters.errorCategories.join(',') : '');
src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts (1)

12-13: Consider adding more diverse test cases.

While the current mock data is valid, consider adding test cases with:

  • Different error categories to test filtering
  • Edge cases like empty taskName or errorCategory
  • Special characters in detailText
 const feedbackDetailsMock: FeedbackDetail[] = [
     { detailText: 'Feedback 1', testCaseName: 'test1', count: 5, relativeCount: 25.0, taskName: '1', errorCategory: 'StudentError' },
     { detailText: 'Feedback 2', testCaseName: 'test2', count: 3, relativeCount: 15.0, taskName: '2', errorCategory: 'StudentError' },
+    { detailText: 'Feedback 3', testCaseName: 'test3', count: 1, relativeCount: 5.0, taskName: '3', errorCategory: 'CompilationError' },
+    { detailText: '', testCaseName: 'test4', count: 0, relativeCount: 0.0, taskName: '', errorCategory: '' }
 ];
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.ts (1)

31-37: Consider adding JSDoc comments for the new signals

The new constant and signals are well-implemented, but could benefit from documentation explaining their purpose.

Consider adding JSDoc comments:

/** Key for storing error categories filter in local storage */
readonly FILTER_ERROR_CATEGORIES_KEY = 'feedbackAnalysis.errorCategories';

/** Array of available task names */
readonly taskArray = signal<string[]>([]);

/** Available error categories for filtering */
readonly errorCategories = signal<string[]>([]);
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.html (1)

46-65: LGTM! Consider adding aria-label for better accessibility.

The implementation follows best practices, uses the new Angular syntax correctly, and maintains consistency with other filter sections.

Consider adding aria-label attributes to improve accessibility:

 <input
     type="checkbox"
     class="form-check-input"
     id="errorCategory-{{ errorCategory }}"
     [value]="errorCategory"
     (change)="onCheckboxChange($event, 'errorCategories')"
     [checked]="filters.errorCategories.includes(errorCategory)"
+    [attr.aria-label]="'Filter by error category: ' + errorCategory"
 />
src/main/java/de/tum/cit/aet/artemis/core/util/PageUtil.java (1)

78-84: Consider optimizing the taskName query for performance and maintainability.

The current implementation has several potential concerns:

  1. The subquery with JOIN and MAX could impact performance for large datasets
  2. Complex SQL logic embedded in an enum reduces maintainability

Consider these improvements:

  1. Move the SQL query to a dedicated repository method:
@Query("""
    SELECT COALESCE(MAX(t.taskName), '')
    FROM ProgrammingExerciseTask t
    JOIN t.testCases tct
    WHERE t.exercise.id = :exerciseId 
    AND tct.testName = :testName
""")
String findTaskNameByExerciseAndTestCase(
    @Param("exerciseId") Long exerciseId,
    @Param("testName") String testName
);
  1. Add an index to improve JOIN performance:
CREATE INDEX idx_test_cases_test_name ON programming_exercise_test_cases(test_name);

Would you like me to help create a separate issue for implementing these improvements?

🧰 Tools
🪛 ast-grep

[warning] 73-84: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 73-84: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-filter-modal.component.spec.ts (2)

52-52: Consider using consistent test data across test cases.

The test correctly verifies error category storage, but consider extracting the test values 'Student Error' and 'Ares Error' into constants at the top of the file for reuse across test cases.

const TEST_ERROR_CATEGORIES = ['Student Error', 'Ares Error'];

Also applies to: 58-58


85-110: Consider improving event mocking and test structure.

While the test cases provide good coverage, consider these improvements:

  1. Use proper TypeScript types for event mocking instead of unknown:
interface MockCheckboxEvent {
    target: {
        checked: boolean;
        value: string;
    };
}
  1. Extract repeated test setup into beforeEach:
beforeEach(() => {
    component.filters = {
        tasks: [],
        testCases: [],
        occurrence: [0, 10],
        errorCategories: []
    };
});
  1. Use toHaveBeenCalledExactlyOnceWith for more specific assertions as per coding guidelines.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.html (3)

3-8: LGTM! Consider adding aria-sort for accessibility.

The flex container and new @if syntax look good. The sort icon implementation is clean and modular.

Consider adding aria-sort attribute to improve accessibility:

-    <th scope="col" [class.clickable]="isSortableColumn(column)" (click)="isSortableColumn(column) && setSortedColumn(column)">
+    <th scope="col" [class.clickable]="isSortableColumn(column)" (click)="isSortableColumn(column) && setSortedColumn(column)"
+        [attr.aria-sort]="isSortableColumn(column) ? getSortDirection(column) : null">

37-44: LGTM! Consider min-width for better responsive behavior.

The column width distribution looks good, but consider adding min-width to prevent content squishing on smaller screens.

Consider adding min-width to maintain readability:

-            <col style="width: 62.5%" />
+            <col style="width: 62.5%; min-width: 300px" />

68-88: LGTM! Consider trackBy for better performance.

The table body implementation is clean and follows the new Angular syntax guidelines.

Consider using a trackBy function instead of tracking the entire item for better performance:

-                @for (item of content().resultsOnPage; track item) {
+                @for (item of content().resultsOnPage; track trackByFn(item)) {

Add to your component:

trackByFn(item: any): string {
  return `${item.taskName}-${item.testCaseName}-${item.detailText}`;
}
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.ts (2)

122-122: LGTM: Sorting implementation is clean and type-safe.

The sorting functionality is well-implemented. Consider extracting the sortable columns into a constant for better maintainability.

+  private readonly SORTABLE_COLUMNS = ['count', 'detailText', 'testCaseName', 'taskName'] as const;
+  type SortableColumn = typeof SORTABLE_COLUMNS[number];
+
   isSortableColumn(column: string): boolean {
-    return ['count', 'detailText', 'testCaseName', 'taskName'].includes(column);
+    return this.SORTABLE_COLUMNS.includes(column as SortableColumn);
   }

Also applies to: 135-141


Line range hint 63-67: Consider implementing memory leak prevention.

While the component is well-structured, consider implementing the following improvements:

  1. Implement ngOnDestroy to clean up subscriptions
  2. Use takeUntilDestroyed operator for the debounced load data
private readonly destroy$ = new Subject<void>();

constructor() {
  effect(() => {
    untracked(async () => {
      await this.loadData();
    });
  }, { injector: inject(DestroyRef) });  // Auto-cleanup of effect
}

ngOnDestroy() {
  this.destroy$.next();
  this.destroy$.complete();
}
src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts (2)

Line range hint 118-135: Enhance test setup with explicit type annotations.

While the test setup is correct, consider adding explicit type annotations for the filter object to improve code maintainability.

- const filters = {
+ const filters: FilterData = {
    tasks: ['task1'],
    testCases: ['testCase1'],
    occurrence: [component.minCount(), 5],
    errorCategories: ['Student Error'],
};

Based on the gathered information, I can now provide a final response.

Test coverage needs improvement in specific areas

The test suite requires additional test cases for comprehensive coverage:

  • No tests found for empty feedback table message display
  • Pagination tests only cover basic page changes but miss edge cases and bounds handling
  • Error category testing is limited to basic cases ('Student Error', 'AST Error') and lacks comprehensive filter combination tests
  • Service tests only cover 'StudentError' category, missing other categories like 'AST Error' and 'Ares Error'
🔗 Analysis chain

Line range hint 1-240: Verify test coverage for all PR objectives.

The test suite covers the core functionality well, but consider using a coverage tool to ensure complete coverage of:

  • Empty feedback table message display
  • Pagination bounds handling
  • All error category filter combinations

Let me gather more information about the test coverage by examining the test files found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check test coverage for feedback analysis component
jest --coverage src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts

# Verify all error categories are tested
rg -l "errorCategory" src/test/javascript/spec/

Length of output: 579


Script:

#!/bin/bash
# Check the content of test files for error category testing
echo "=== Feedback Analysis Component Tests ==="
rg "errorCategory" -B 2 -A 2 src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts

echo -e "\n=== Feedback Analysis Service Tests ==="
rg "errorCategory" -B 2 -A 2 src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts

echo -e "\n=== Feedback Modal Tests ==="
rg "errorCategory" -B 2 -A 2 src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-modal.component.spec.ts

# Check for empty table message tests
echo -e "\n=== Empty Table Message Tests ==="
rg -i "empty|no.*results" src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts

# Check for pagination tests
echo -e "\n=== Pagination Tests ==="
rg "page|setPage" -B 2 -A 2 src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts

Length of output: 2503

src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java (1)

286-328: Documentation improvements suggested.

The documentation is comprehensive and well-structured. However, consider these refinements for better consistency and maintainability:

  1. Use consistent HTML formatting (prefer <ul> over <br> for lists).
  2. Align response structure documentation with Java records pattern.

Apply these changes to improve the documentation:

     * GET /exercises/{exerciseId}/feedback-details : Retrieves paginated and filtered aggregated feedback details for a specified exercise.
-     * <br>
+     *
     * This endpoint provides detailed feedback analytics, including:
-     * - The count and relative count (percentage) of each unique feedback entry.
-     * - Associated test case names.
-     * - Task names, mapped from test cases.
-     * <br>
+     * <ul>
+     * <li>The count and relative count (percentage) of each unique feedback entry</li>
+     * <li>Associated test case names</li>
+     * <li>Task names, mapped from test cases</li>
+     * </ul>
     *
     * Pagination, sorting, and filtering options allow flexible data retrieval:
     * <ul>
     * <li><b>Pagination:</b> Based on page number and page size, as specified in the request.</li>
     * <li><b>Sorting:</b> By column (e.g., "count" or "detailText") and sorting order (ASCENDING or DESCENDING).
     * If the specified column is not valid for sorting, the default sorting column is "count".</li>
     * <li><b>Filtering:</b>
     * <ul>
     * <li><b>Task names:</b> Filters feedback entries by specific task names, including "Not assigned to task" if unassigned feedback is requested.</li>
     * <li><b>Test case names:</b> Filters by specified test cases, using only active test cases from the exercise.</li>
     * <li><b>Occurrence range:</b> Filters by the minimum and maximum number of occurrences (inclusive).</li>
     * <li><b>Search term:</b> Case-insensitive filter applied to feedback detail text.</li>
     * <li><b>Error categories:</b> Filters feedback entries by specified error categories (e.g., "Student Error," "Ares Error," and "AST Error").</li>
     * </ul>
     * </li>
     * </ul>
     *
     * @param exerciseId The unique identifier of the exercise for which feedback details are requested.
     * @param data       A {@link FeedbackPageableDTO} object containing pagination, sorting, and filtering parameters, including:
-     *                       <ul>
-     *                       <li>Page number and page size</li>
-     *                       <li>Search term (optional)</li>
-     *                       <li>Sorting order (ASCENDING or DESCENDING)</li>
-     *                       <li>Sorted column</li>
-     *                       <li>Filter task names (optional)</li>
-     *                       <li>Filter test case names (optional)</li>
-     *                       <li>Occurrence range (optional)</li>
-     *                       <li>Error categories (optional)</li>
-     *                       </ul>
+     *                  See {@link FeedbackPageableDTO} for detailed parameter descriptions.
     * @return A {@link ResponseEntity} containing a {@link FeedbackAnalysisResponseDTO}, which includes:
     *         <ul>
     *         <li>{@link SearchResultPageDTO < FeedbackDetailDTO >} feedbackDetails: Paginated and filtered feedback details for the exercise.</li>
     *         <li>long totalItems: The total count of feedback entries (for pagination).</li>
     *         <li>Set<String> taskNames: A set of task names relevant to the feedback items, including "Not assigned to task" if applicable.</li>
     *         <li>List<String> testCaseNames: A list of active test case names used in the feedback.</li>
     *         <li>List<String> errorCategories: The list of error categories included in the feedback details, such as "Student Error," "Ares Error," and "AST Error".</li>
     *         </ul>
src/test/java/de/tum/cit/aet/artemis/assessment/ResultServiceIntegrationTest.java (1)

756-756: Consider using constants for error categories.

The error categories are hardcoded in the test. Consider defining these as constants to maintain consistency and make updates easier across tests.

+ private static final Set<String> EXPECTED_ERROR_CATEGORIES = Set.of("Student Error", "Ares Error", "AST Error");
...
- assertThat(response.errorCategories()).containsExactlyInAnyOrder("Student Error", "Ares Error", "AST Error");
+ assertThat(response.errorCategories()).containsExactlyInAnyOrderElementsOf(EXPECTED_ERROR_CATEGORIES);
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)

620-621: Consider defining ERROR_CATEGORIES as a static final class constant

To improve reusability and maintainability, consider defining ERROR_CATEGORIES as a static final constant at the class level.

Suggested change:

+private static final List<String> ERROR_CATEGORIES = List.of("Student Error", "Ares Error", "AST Error");

...

-    final List<String> ERROR_CATEGORIES = List.of("Student Error", "Ares Error", "AST Error");
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7503f9c and 9a7674f.

📒 Files selected for processing (23)
  • src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackAnalysisResponseDTO.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackDetailDTO.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackPageableDTO.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/core/util/PageUtil.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.html (1 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.ts (4 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-modal.component.html (1 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.html (4 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.ts (8 hunks)
  • src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.service.ts (2 hunks)
  • src/main/webapp/app/shared/sort/sort-icon.component.html (1 hunks)
  • src/main/webapp/app/shared/sort/sort-icon.component.scss (1 hunks)
  • src/main/webapp/app/shared/sort/sort-icon.component.ts (1 hunks)
  • src/main/webapp/i18n/de/programmingExercise.json (2 hunks)
  • src/main/webapp/i18n/en/programmingExercise.json (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/assessment/ResultServiceIntegrationTest.java (4 hunks)
  • src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts (7 hunks)
  • src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts (2 hunks)
  • src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-filter-modal.component.spec.ts (3 hunks)
  • src/test/javascript/spec/util/shared/sort-icon.component.spec.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackDetailDTO.java
  • src/main/webapp/app/shared/sort/sort-icon.component.scss
🧰 Additional context used
📓 Path-based instructions (20)
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackAnalysisResponseDTO.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/assessment/dto/FeedbackPageableDTO.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/assessment/service/ResultService.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/assessment/web/ResultResource.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/core/util/PageUtil.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/exercise/repository/StudentParticipationRepository.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/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.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/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.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/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-modal.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/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.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/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.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/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.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/shared/sort/sort-icon.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/shared/sort/sort-icon.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/main/webapp/i18n/de/programmingExercise.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/assessment/ResultServiceIntegrationTest.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/programming-exercise/feedback-analysis/feedback-analysis.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/programming-exercise/feedback-analysis/feedback-analysis.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}}

src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-filter-modal.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/util/shared/sort-icon.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}}

🪛 ast-grep
src/main/java/de/tum/cit/aet/artemis/core/util/PageUtil.java

[warning] 73-84: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 73-84: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

🪛 GitHub Check: client-style
src/main/webapp/app/shared/sort/sort-icon.component.ts

[warning] 7-7:
The selector should start with one of these prefixes: "jhi" (https://angular.dev/style-guide#style-02-07)

🔇 Additional comments (29)
src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackAnalysisResponseDTO.java (1)

4-4: LGTM! Clean import statement.

The specific import for Set follows best practices by avoiding wildcard imports.

src/main/webapp/app/shared/sort/sort-icon.component.ts (2)

1-4: LGTM! Clean and efficient imports.

The imports are well-organized and follow best practices by importing only the necessary modules and specific icons.


1-21: Verify integration with feedback analysis feature.

Since this component is part of the feedback analysis enhancement, let's verify its integration.

✅ Verification successful

Integration with feedback analysis feature is properly implemented

The sort-icon component is correctly integrated within the feedback analysis feature:

  • Used in table headers for sortable columns via <app-sort-icon [direction]="getSortDirection(column)"></app-sort-icon>
  • Properly imported and declared in the component's imports
  • Sorting direction is correctly managed through the getSortDirection() method which returns the expected 'asc' | 'desc' | 'none' values
  • Sorting functionality is fully implemented with column sorting and order switching
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of the sort-icon component in feedback analysis
rg -l "app-sort-icon" --type html | grep -i "feedback"

# Verify sorting functionality implementation
rg -A 5 "direction=" --type html | grep -i "feedback"

Length of output: 212


Script:

#!/bin/bash
# Let's check the actual implementation in the feedback analysis component
cat src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.html

# Also check the corresponding TypeScript file for sorting logic
cat src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.ts

Length of output: 14478

🧰 Tools
🪛 GitHub Check: client-style

[warning] 7-7:
The selector should start with one of these prefixes: "jhi" (https://angular.dev/style-guide#style-02-07)

src/main/java/de/tum/cit/aet/artemis/assessment/dto/FeedbackPageableDTO.java (2)

17-18: LGTM! Field declaration follows established patterns.

The new field follows consistent naming and type patterns with other filter fields in the class.


55-57: LGTM! Setter follows established patterns.

The setter method is consistent with other setters in the class and follows Java bean conventions.

src/test/javascript/spec/util/shared/sort-icon.component.spec.ts (1)

18-23: LGTM! Test cases follow best practices.

The test cases:

  • Properly set input values using componentRef.setInput
  • Call detectChanges at appropriate times
  • Use correct assertion methods (toBeTrue/toBeFalse)
  • Cover all valid scenarios

Also applies to: 25-30, 32-37

src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts (1)

45-45: LGTM!

The filters object correctly includes the new errorCategories array, maintaining consistency with the service implementation.

src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-filter-modal.component.ts (3)

12-12: LGTM: Interface property addition follows conventions

The new errorCategories property is well-typed and follows TypeScript naming conventions.


43-43: LGTM: Consistent filter initialization

The errorCategories filter is properly initialized following the same pattern as other filters.


1-1: Verify lazy loading configuration

The component is properly configured as standalone, but we should verify it's properly set up for lazy loading in the routing configuration.

src/main/java/de/tum/cit/aet/artemis/core/util/PageUtil.java (1)

78-84: Verify NULL handling in the taskName query.

The COALESCE with empty string might mask NULL values, making it harder to distinguish between "no task name exists" and "task name is intentionally empty".

Let's verify the NULL handling requirements:

✅ Verification successful

Based on the search results, we can see that taskName is used in a consistent way across the codebase, particularly in test cases and repositories. The COALESCE with empty string in the query appears to be an intentional design choice to handle cases where no task name exists, as evidenced by:

  1. The same pattern is used in multiple repository queries (StudentParticipationRepository)
  2. Test cases show that taskName is a meaningful identifier that's explicitly set and checked
  3. No validation constraints or special NULL handling requirements were found in the codebase

The current NULL handling with COALESCE is appropriate.

The empty string fallback provides a consistent interface for UI/client code while maintaining the ability to distinguish between "no task exists" (query returns empty string) and "task exists but has no name" (task exists in join but has NULL name) through the original database records.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for NULL handling patterns in similar contexts
rg -g '*.java' -A 5 'COALESCE.*taskName'

# Check if empty strings are used elsewhere for task names
rg -g '*.java' 'taskName\s*=\s*""'

Length of output: 79


Script:

#!/bin/bash
# Let's search for broader patterns related to taskName and NULL handling
rg -g '*.java' -B 2 -A 2 'taskName.*NULL|NULL.*taskName'

# Search for ProgrammingExerciseTask usage patterns
ast-grep --pattern 'class $_ {
  $$$
  taskName
  $$$
}'

# Look for any SQL queries or JPA operations involving taskName
rg -g '*.java' -B 2 -A 2 'SELECT.*taskName|FROM.*ProgrammingExerciseTask'

# Check repository tests for taskName handling
rg -g '*Test.java' 'taskName'

Length of output: 8333


Script:

#!/bin/bash
# Let's check the entity class to understand taskName's constraints and usage
ast-grep --pattern 'class ProgrammingExerciseTask {
  $$$
}'

# Look for service layer handling of taskName
rg -g '*.java' -B 3 -A 3 'ProgrammingExerciseTask.*service|service.*ProgrammingExerciseTask'

# Check for any validation or business logic around taskName
rg -g '*.java' 'taskName.*validate|validate.*taskName'

Length of output: 11427

🧰 Tools
🪛 ast-grep

[warning] 73-84: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 73-84: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Map.of(
"count", "COUNT(f.id)",
"detailText", "f.detailText",
"testCaseName", "f.testCase.testName",
"taskName", """
COALESCE((
SELECT MAX(t.taskName)
FROM ProgrammingExerciseTask t
JOIN t.testCases tct
WHERE t.exercise.id = :exerciseId AND tct.testName = f.testCase.testName
), '')"""
))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-filter-modal.component.spec.ts (3)

35-35: LGTM: Filter initialization properly includes error categories.

The addition of errorCategories array to the filters object follows the existing pattern and matches the PR objectives.

Also applies to: 42-42


73-73: LGTM: Filter clearing properly handles error categories.

The test correctly verifies both the localStorage clearing and filter reset for error categories.

Also applies to: 79-79


Line range hint 1-116: Verify test coverage for edge cases.

While the test coverage is good for the main functionality, consider adding these test cases:

  1. Handling of invalid error category values
  2. Behavior when maximum number of filters is selected
  3. Case sensitivity in error category matching
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.html (3)

11-17: LGTM! Well-structured empty state handling.

The empty state template provides clear user feedback with appropriate messages for different scenarios (filtered vs. no data).


54-54: LGTM! Column headers align with PR objectives.

The replacement of taskNumber with taskName and addition of errorCategory align with the PR's goal of enhancing feedback analysis.

Also applies to: 62-62


99-100: LGTM! Pagination limit improves UI clarity.

Setting maxSize to 5 helps maintain a clean pagination interface without overwhelming the user.

src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/feedback-analysis.component.ts (5)

12-12: LGTM: Import and component configuration changes are correct.

The addition of SortIconComponent aligns with the PR objective of making columns sortable and follows Angular's style guide.

Also applies to: 19-19


74-74: LGTM: Data loading implementation is robust.

The integration of error categories into the data loading flow is well-implemented with proper error handling and state management.

Also applies to: 82-82, 92-92, 97-97, 99-99


146-146: LGTM: Filter modal integration is complete.

The error categories are properly integrated into the filter modal with consistent patterns for state management and data passing.

Also applies to: 153-153, 156-156, 161-161


181-183: LGTM: Filter counting logic is consistent.

The error categories are properly integrated into the filter counting mechanism.


32-32: Verify the impact of increased page size and text length.

While the changes support the new features, please verify:

  1. The increased page size (25 items) doesn't impact performance or load times
  2. The increased feedback text length (200 chars) doesn't cause UI layout issues

Also applies to: 47-47, 52-52, 54-58

✅ Verification successful

Let me gather more information about the pagination and performance aspects.


Based on the analysis of the search results, I can now provide a final response:

The page size and text length changes are safe to implement

  1. The increased page size (25) is well within normal range:

    • Other components in the codebase use similar or larger page sizes (e.g., conversation components use 50, learning paths table uses 100)
    • The feedback analysis component handles pagination through standard mechanisms used across the application
  2. The text length handling (200 chars) is properly managed:

    • The template (feedback-analysis.component.html) already implements text truncation with ellipsis
    • The UI safely handles longer texts by showing only first 100 characters with "..." when exceeding the limit
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for pagination-related performance tests
rg -l "pageSize.*performance|load.*test.*page" --type ts

# Search for feedback text length handling in templates
rg "MAX_FEEDBACK_DETAIL_TEXT_LENGTH|feedbackDetail.*text" --type html

Length of output: 392


Script:

#!/bin/bash
# Search for pageSize usage in components and services
rg "pageSize" -A 3 --type ts

# Look for any performance or load testing related files
fd "performance|load" --type f --extension ts

# Search for feedback text display related code
ast-grep --pattern 'detailText.length > $_'

Length of output: 45139

src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.component.spec.ts (1)

20-43: LGTM! Mock data structure aligns with the new requirements.

The mock data correctly implements the new error categories and task name changes, providing good test coverage for different error types.

src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java (1)

Line range hint 331-334: LGTM! Clean implementation following best practices.

The method implementation:

  • Uses proper REST and security annotations
  • Follows single responsibility principle by delegating to service layer
  • Uses appropriate parameter binding
src/test/java/de/tum/cit/aet/artemis/assessment/ResultServiceIntegrationTest.java (2)

746-746: LGTM: URL parameter addition for error categories.

The addition of filterErrorCategories parameter aligns with the PR objectives and follows the existing URL parameter pattern.


812-812: Duplicate error categories assertion found.

This assertion duplicates the error categories from the previous test. Using the suggested constants would help avoid this duplication and ensure consistency.

src/main/webapp/i18n/en/programmingExercise.json (2)

357-358: LGTM! Clear and helpful empty state messages.

The messages effectively communicate two distinct scenarios to users:

  1. When no feedback entries exist for the exercise
  2. When no entries match the current filter/search criteria

368-368: LGTM! Error category filter option added.

The new filter option aligns with the PR objectives to enhance feedback analysis with error categories.

src/main/webapp/i18n/de/programmingExercise.json (1)

368-368: LGTM!

The translation "Fehlerkategorien" is correct and appropriate for this context.

raffifasaro
raffifasaro previously approved these changes Oct 28, 2024
Copy link
Contributor

@raffifasaro raffifasaro left a comment

Choose a reason for hiding this comment

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

code LGTM

az108 added 2 commits October 28, 2024 22:59
…tegorize-feedback-in-table' into feature/programming-exercises/categorize-feedback-in-table
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 TS1. Everything works as expected
Screenshot 2024-10-29 190351
Screenshot 2024-10-29 190441

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 TS1 and everything worked as expected. 👍

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

@krusche krusche modified the milestones: 7.6.5, 7.7.0 Nov 3, 2024
Copy link
Contributor

@rabeatwork rabeatwork left a comment

Choose a reason for hiding this comment

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

We talked about a few small things that will be implemented/improved in the next PR.

@az108 az108 added ready to merge maintainer-approved The feature maintainer has approved the PR and removed ready for review labels Nov 5, 2024
@krusche krusche changed the title Programming exercises: Add Error Categories and Categorize Feedback Programming exercises: Add error categories and categorize feedback in grading analysis Nov 10, 2024
@krusche krusche merged commit 395e8df into develop Nov 10, 2024
115 of 122 checks passed
@krusche krusche deleted the feature/programming-exercises/categorize-feedback-in-table branch November 10, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assessment Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module maintainer-approved The feature maintainer has approved the PR ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.