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: Update typescript #9051

Merged
merged 124 commits into from
Aug 10, 2024
Merged

Development: Update typescript #9051

merged 124 commits into from
Aug 10, 2024

Conversation

krusche
Copy link
Member

@krusche krusche commented Jul 14, 2024

Checklist

General

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.
  • 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 documented the TypeScript code using JSDoc style.

Motivation and Context

Use the latest version of typescript

Description

This PR includes an upgrade prompted by the updated version of TypeScript, which no longer supports "suppressImplicitAnyIndexErrors": true. As a result, all implicit any assertions have been revised or rewritten.

Steps for Testing

Since this PR only modifies the typing of some variables, the behavior of the code should remain unchanged. However, it is crucial to verify that the affected areas function as expected, with a particular focus on testing modeling exercises and their assessment.

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

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

unchanged

Summary by CodeRabbit

  • New Features

    • Updated Angular and related packages to improve performance and introduce new features.
    • Enhanced type safety across various components and services.
  • Bug Fixes

    • Improved error handling and data structure management in multiple components.
  • Refactor

    • Streamlined initialization logic and improved property access methods in several components.
    • Transitioned from plain objects to Map objects for better data management.
    • Centralized search initialization logic in the CompetencySearchComponent tests for improved maintainability.
  • Chores

    • Removed outdated configurations and properties to enforce stricter type checks.

@github-actions github-actions bot added tests client Pull requests that update TypeScript code. (Added Automatically!) cypress Pull requests that update cypress tests. (Added Automatically!) labels Jul 14, 2024
Jan-Thurner
Jan-Thurner previously approved these changes Aug 9, 2024
Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

Tested these features on TS1:
Modeling Exercise Creation ✅
Modeling Exercise Participation ✅
Competency import ✅
Tutorial group session creation ✅
tutorial Group session student view ✅
short answer questions ✅
Lecture Unit management ✅

coolchock
coolchock previously approved these changes Aug 9, 2024
Copy link
Contributor

@coolchock coolchock left a comment

Choose a reason for hiding this comment

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

Tested in testing session on ts1. Tested components for which the types where changed, no issues found.

florian-glombik
florian-glombik previously approved these changes Aug 9, 2024
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.

Tested the changes on ts1 in testing session

  • Tested modeling exercise (creation, participation, assessment, review of assessment)
  • Tested sidebar and course switching
  • Tested programming exercise grading configuration
  • Participated in programming exercise
  • Tested text exercise (creation, participation, assessment

Found an unrelated issue in the assessment Assessment: Tutor note input field available if not submission is selected #9203

edkaya
edkaya previously approved these changes Aug 9, 2024
Copy link
Contributor

@edkaya edkaya left a comment

Choose a reason for hiding this comment

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

Code changes lgtm

@@ -12,4 +12,6 @@ export class ProgrammingExerciseTask extends ProgrammingExerciseServerSideTask {
resultingPoints?: number;
resultingPointsPercent?: number;
stats: TestCaseStats | undefined;

[key: string]: number | string | undefined | TestCaseStats | ProgrammingExerciseTestCase[];
Copy link
Contributor

Choose a reason for hiding this comment

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

This also sticks out a bit. Why do we need to define for arbitrary strings that a programming exercise task might have attributes of type number | string | undefined | TestCaseStats | ProgrammingExerciseTestCase[]?

Copy link
Contributor

@florian-glombik florian-glombik Aug 9, 2024

Choose a reason for hiding this comment

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

I do not understand why we need this here, can you explain it to us @SimonEntholzer ?

When removing this type declaration I do not seem to encounter errors in the files that use the ProgrammingExerciseTask

I checked the following files:

  • programming-exercise-grading-tasks-table.component.ts
  • programming-exercise-task.component.ts
  • programming-exercise-task.service.ts
  • programming-exercise-grading-tasks-table.component.spec.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

I remove the type and the client and e2e tests are still running through, also double checked the programming grading table and it worked, I am quite confident that the type was not necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Might have been an IDE related issue on my side. It looked like without this additional attribute accessing some of the ProgrammingExerciseTask with the ....["..."] notation wouldn't work. Seems like it does work without the additional key attribute 👍

coderabbitai[bot]
coderabbitai bot previously approved these changes Aug 9, 2024
Copy link
Contributor

@edkaya edkaya left a comment

Choose a reason for hiding this comment

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

Reapprove after changing types

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 after re-testing grading configuration for programming exercises after last two commits

@krusche krusche merged commit edbd614 into develop Aug 10, 2024
33 of 36 checks passed
@krusche krusche deleted the chore/update-typescript branch August 10, 2024 06:00
JohannesWt pushed a commit that referenced this pull request Sep 23, 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!) cypress Pull requests that update cypress tests. (Added Automatically!) ready for review tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.