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: Change the build plan editors to Monaco #8978

Merged

Conversation

pzdr7
Copy link
Contributor

@pzdr7 pzdr7 commented Jul 6, 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.
  • I added multiple screenshots/screencasts of my UI changes.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

We want to replace Ace with Monaco. As of now, all custom build plan/script editors in Artemis use Ace.

Description

  • Replaced Ace with Monaco in three components:
    • Custom build plan editor
    • Custom Aeolus build plan editor
    • Build plan editor

Steps for Testing

Caution

One E2E test is failing because of the branch name...

Caution

Due to the issue #8980, you may encounter situations where the build script is not updated on the first attempt. Any subsequent attempts should work, though.

Prerequisites:

  • 1 Instructor
  • 1 Programming Exercise
  • 1 Test Server with LocalVC/CI enabled (e.g. TS1-3)
  • 1 Test Server with Gitlab/Jenkins (e.g. TS6)
  • 1 Local setup with Aeolus and LocalVC/CI

Aeolus setup:

  1. Start Aeolus:
    docker compose -f docker/aeolus.yml up -d

  2. Add to application-local.yml

aeolus:
    url: http://localhost:8090
  1. Adjust your server run config to include the following profiles:
    dev,localci,localvc,artemis,scheduling,buildagent,aeolus,core,ldap-only,local

  2. Run the server and the client.


On your local setup (set up Aeolus as described above):

  1. Navigate to course management > your course > exercises > click the title of your programming exercise.
  2. Click "Edit"
  3. Scroll down and tick the box "Customize build script"
  4. Verify that you can add / remove actions and edit the corresponding build scripts.
  5. Save the exercise and verify that the changes were saved.

On the Test Server with LocalVC

  1. Navigate to course management > your course > exercises > click the title of your programming exercise.
  2. Click "Edit"
  3. Scroll down and tick the box "Customize build script"
  4. Verify that you can edit the build script.
  5. Save the exercise and verify that the changes were saved.

On the Test Server without LocalVC

  1. Navigate to course management > your course > exercises > click the title of your programming exercise.
  2. Click "Edit build plan"
  3. Verify that you can edit the build plan
  4. Click "Submit" and verify that the changes were saved.

Testserver States







Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
build-plan-editor.component.ts 100%
programming-exercise-custom-aeolus-build-plan.component.ts 95.29%
programming-exercise-custom-build-plan.component.ts 100%

Screenshots

Build script editor

image

Build script editor (Aeolus)

image

Build plan editor

image

Summary by CodeRabbit

  • New Features

    • Replaced Ace Editor with Monaco Editor across various programming exercise components for an enhanced editing experience.
  • Improvements

    • Updated layouts and styling in script editing sections to improve user interaction and visual presentation.
  • Bug Fixes

    • Adjusted editor configurations to ensure better performance and layout handling.
  • Tests

    • Updated test configurations to include Monaco Editor and removed dependencies on Ace Editor.

@github-actions github-actions bot added tests client Pull requests that update TypeScript code. (Added Automatically!) labels Jul 6, 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.

code

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.

Reapprove code with tab size settings

Copy link
Contributor

@sarpsahinalp sarpsahinalp 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

@Strohgelaender
Copy link
Contributor

Is it intended that the checkbox "Customize Build Script" is also disabled if the script is already customized?

Yes this was intentional. The checkbox was not really meant as a setting but more as a 'hide details' button. I see why this is confusing, but in the future we want to move away from this position anyways (see #8994) so this is nothing we need to fix.

@pzdr7 pzdr7 added this to the 7.4.4 milestone Jul 17, 2024
Copy link
Contributor

@egekurt123 egekurt123 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 TS4(LocalVC/CI) and on TS6 (Gitlab) works as expected on both

@krusche krusche merged commit 50bcebf into develop Jul 20, 2024
54 of 58 checks passed
@krusche krusche deleted the feature/programming-exercises/monaco-build-plan-editors branch July 20, 2024 08:19
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 tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants