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

Development: Migrate exam and exam exercise import to signals #10106

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

coolchock
Copy link
Contributor

@coolchock coolchock commented Jan 5, 2025

Checklist

General

Client

Description

This PR updates the Exam and Exam Exercise Import components to use Signals.

Steps for Testing

Prerequisites:

  • 1 Instructor
  1. Log in to Artemis
  2. Navigate to Course Administration -> Exams
  3. Try to import an exam and check if it works
  4. Try to import an exam exercise to an exam and check if it works

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

Summary by CodeRabbit

  • Refactor
    • Transitioned component input handling to a function-based approach for improved consistency in data retrieval and UI evaluation.
    • Updated template conditional logic to use function calls for clearer state management.
  • Chores
    • Streamlined initialization by removing redundant constructors in certain services.
  • Tests
    • Aligned test cases with the new input-binding mechanism by updating the way component inputs are set.

@github-actions github-actions bot added tests client Pull requests that update TypeScript code. (Added Automatically!) labels Jan 5, 2025
Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 13, 2025
@coolchock coolchock removed the stale label Jan 26, 2025
# Conflicts:
#	src/main/webapp/app/exam/manage/exam-management.module.ts
#	src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.ts
#	src/main/webapp/app/exam/manage/exams/exam-import/exam-import-paging.service.ts
#	src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.ts
#	src/test/javascript/spec/component/exam/exam-update.component.spec.ts
Copy link

github-actions bot commented Feb 3, 2025

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 3, 2025
# Conflicts:
#	src/main/webapp/app/exam/manage/exam-management.module.ts
#	src/test/javascript/spec/component/exam/manage/exam-import.component.spec.ts
@github-actions github-actions bot removed the stale label Feb 14, 2025
@coolchock coolchock changed the title General: Migrate exam and exam exercise import to signals, inject and standalone General: Migrate exam and exam exercise import to signals Feb 15, 2025
@coolchock coolchock marked this pull request as ready for review February 15, 2025 09:38
@coolchock coolchock requested a review from a team as a code owner February 15, 2025 09:38
Copy link

coderabbitai bot commented Feb 15, 2025

Walkthrough

This pull request refactors how component inputs are accessed by transitioning from direct property access to function calls. In several Angular components and their corresponding tests, the use of the @Input() and @ViewChild() decorators has been replaced with function-based approaches using input() and viewChild.required(). The changes update both the HTML templates and TypeScript files to call properties as functions (e.g., exam() instead of exam) and adjust the test suites to use fixture.componentRef.setInput(). Additionally, a redundant constructor was removed from a paging service.

Changes

File(s) Change Summary
src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.html
src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.ts
Refactored the Exam Exercise Import Component by replacing direct property accesses (e.g., exam.exerciseGroups) with function calls (e.g., exam().exerciseGroups), removing @Input() decorators and initializing inputs using input() methods.
src/main/webapp/app/exam/manage/exams/exam-import/exam-import-paging.service.ts
src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.html
src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.ts
Updated the Exam Import Component by removing the redundant constructor in the paging service, replacing traditional @Input() and @ViewChild() usage with input() and viewChild.required(), and converting property accesses to function calls for inputs and internal component references.
src/test/javascript/spec/component/exam/exam-update.component.spec.ts
src/test/javascript/spec/component/exam/manage/exam-exercise-import.component.spec.ts
src/test/javascript/spec/component/exam/manage/exam-import.component.spec.ts
Adjusted test suites to set component inputs using fixture.componentRef.setInput(), and updated import statements to include the new input decorator from Angular’s core package.

Sequence Diagram(s)

sequenceDiagram
    participant P as Parent Component/Test
    participant E as ExamExerciseImportComponent
    P->>E: Set input using setInput('exam', examObject)
    E->>E: Initialize exam via input.required<Exam>()
    E->>E: Invoke exam() to retrieve exam data
    E->>Template: Bind exam().exerciseGroups in HTML
Loading
sequenceDiagram
    participant P as Parent Component/Test
    participant I as ExamImportComponent
    P->>I: Set inputs using setInput (e.g., subsequentExerciseGroupSelection, targetExamId)
    I->>I: Initialize inputs via input() functions
    I->>I: Call input functions (e.g., subsequentExerciseGroupSelection()) during processing
    I->>Child: Access child component via viewChild.required()
Loading

Possibly related PRs

Suggested labels

small, ready to merge, refactoring

Suggested reviewers

  • SimonEntholzer
  • JohannesStoehr
  • HawKhiem
  • EneaGore
  • BBesrour
  • az108
  • Feras797

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bbe907c and 7ff7f3f.

📒 Files selected for processing (8)
  • src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.html (1 hunks)
  • src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.ts (4 hunks)
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import-paging.service.ts (0 hunks)
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.html (3 hunks)
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.ts (6 hunks)
  • src/test/javascript/spec/component/exam/exam-update.component.spec.ts (2 hunks)
  • src/test/javascript/spec/component/exam/manage/exam-exercise-import.component.spec.ts (9 hunks)
  • src/test/javascript/spec/component/exam/manage/exam-import.component.spec.ts (3 hunks)
💤 Files with no reviewable changes (1)
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import-paging.service.ts
✅ Files skipped from review due to trivial changes (1)
  • src/test/javascript/spec/component/exam/exam-update.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...

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/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.html
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...

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/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.ts
  • src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...

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/exam/manage/exam-exercise-import.component.spec.ts
  • src/test/javascript/spec/component/exam/manage/exam-import.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.ts (4)

2-2: LGTM! Import statement updated for new input handling.

The import statement correctly includes the new Angular APIs (inject, input, viewChild) needed for the signal-based input handling.


29-32: LGTM! Input properties migrated to signal-based approach.

The migration from @Input() decorators to the input() function follows Angular's new input handling pattern, improving reactivity and type safety.


34-34: LGTM! ViewChild access updated to use required pattern.

The migration from @ViewChild() decorator to viewChild.required() ensures type safety and proper null checking.


53-53:

❓ Verification inconclusive

Verify the function call syntax in template.

The property access has been changed to function calls. Ensure that the corresponding template file has been updated to use the function call syntax (e.g., targetCourseId() instead of targetCourseId).

Also applies to: 65-65, 73-74


🏁 Script executed:

#!/bin/bash
# Search for property access in the template
rg -A 2 "targetCourseId|targetExamId|subsequentExerciseGroupSelection" --type html

Length of output: 233


Action Required: Manually Verify Template Function Call Usage

The changes in the TypeScript file now use function call syntax (e.g., targetCourseId()) as expected. However, since the initial ripgrep command applied a filter that skipped files, we haven't automatically confirmed that the corresponding template (likely exam-import.component.html) has been updated accordingly. Please verify that all template references (for targetCourseId, targetExamId, and subsequentExerciseGroupSelection) use function call syntax (e.g., targetCourseId()) instead of property accesses.

  • Confirm that exam-import.component.html (or any related template files) uses function calls.
  • Also verify similar updates in the templates related to lines 65 and 73-74.

Use the revised shell command below to help narrow your search:

#!/bin/bash
# Verify if the exam-import.component.html template uses function call syntax for targetCourseId
FILE=$(fd -e html | rg "exam-import.component.html")
if [ -n "$FILE" ]; then
  echo "Found file: $FILE"
  rg -n "targetCourseId\(" "$FILE"
else
  echo "exam-import.component.html not found. Please verify manually."
fi
src/test/javascript/spec/component/exam/manage/exam-import.component.spec.ts (2)

99-100: LGTM! Test input setting updated to use fixture.componentRef.setInput().

The test cases correctly use fixture.componentRef.setInput() to set component inputs, aligning with Angular's new input handling pattern.

Also applies to: 102-103, 107-111


149-156: LGTM! Test setup properly handles multiple inputs.

The test correctly sets up multiple inputs using fixture.componentRef.setInput() and includes fixture.detectChanges() to trigger change detection.

src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.ts (3)

1-1: LGTM! Import statement updated for new input handling.

The import statement correctly includes the new Angular APIs needed for signal-based input handling.


21-22: LGTM! Input properties properly configured.

The input properties are correctly configured:

  • exam uses input.required() to enforce mandatory input
  • importInSameCourse uses input() with a default value of false

54-54:

❓ Verification inconclusive

Verify the function call syntax in template.

The property access has been changed to function calls. Ensure that the corresponding template file has been updated to use the function call syntax (e.g., exam() instead of exam).

Also applies to: 87-87, 92-92


🏁 Script executed:

#!/bin/bash
# Search for property access in the template
rg -A 2 "exam\.|importInSameCourse" --type html

Length of output: 198


Action Required: Verify HTML Template Syntax for Function Calls

The TypeScript file now correctly uses a function call (e.g., this.importInSameCourse()). However, the verification script intended to search HTML templates (using ripgrep with the --type html flag) didn’t return any results. This likely indicates that the filter may have excluded files (possibly because Angular component templates often have extensions like .component.html).

Please ensure that the corresponding template file(s) (for example, src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.html) have been updated to use function call syntax (e.g., exam()) rather than property access. Also, double-check the similar changes at lines 87 and 92.

To help with verification, consider running a revised search such as:

#!/bin/bash
# List all HTML files and search for function call syntax
fd -t f -e html | xargs rg "exam\("
fd -t f -e html | xargs rg "importInSameCourse\("

Then, review the matched files to confirm that all references use function calls.

src/test/javascript/spec/component/exam/manage/exam-exercise-import.component.spec.ts (1)

83-84: LGTM! Test input setting consistently uses fixture.componentRef.setInput().

The test cases consistently use fixture.componentRef.setInput() to set component inputs, properly testing the new input handling pattern.

Also applies to: 106-107, 180-180, 202-202, 260-260, 328-329

src/main/webapp/app/exam/manage/exams/exam-import/exam-import.component.html (3)

3-6: Updated Conditional Rendering for Modal Titles
The conditional expressions for rendering the modal titles now correctly invoke subsequentExerciseGroupSelection() instead of checking a variable directly. This change aligns with the signal‐based approach and ensures that the return value of the function is used for conditional rendering.


64-67: Button Rendering with Function Call
The logic for displaying the import button has been updated to use a function call (subsequentExerciseGroupSelection()) for both the true and false conditions. This helps maintain consistency with the signal approach. Ensure that the function is optimized (i.e. memoized if needed) since it is invoked multiple times within the template.


81-81: Consistent State Check for Import Transition
At line 81, the combined condition @if (exam && subsequentExerciseGroupSelection()) { correctly checks for the presence of an exam while leveraging the signal-based function call. Verify that the exam property remains stable and that this logical combination accurately reflects the desired UI state.

src/main/webapp/app/exam/manage/exams/exam-exercise-import/exam-exercise-import.component.html (1)

18-18: Migrated Access of Exam Property via Signal
The change from exam.exerciseGroups to exam().exerciseGroups in the @for loop correctly implements the new function-based approach for accessing the exam signal. This ensures that any updates to the exam signal propagate as expected to the list of exercise groups. Confirm that exam() is properly defined and returns an object with an exerciseGroups property.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@krusche krusche changed the title General: Migrate exam and exam exercise import to signals Development: Migrate exam and exam exercise import to signals Feb 15, 2025
@helios-aet helios-aet bot temporarily deployed to artemis-test4.artemis.cit.tum.de February 15, 2025 12:42 Inactive
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!) ready for review small tests
Projects
Status: Work In Progress
Development

Successfully merging this pull request may close these issues.

2 participants