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

Lectures: Add dismiss modal for unsaved changes to title or period section #10023

Conversation

florian-glombik
Copy link
Contributor

@florian-glombik florian-glombik commented Dec 14, 2024

Checklist

General

Client

  • I strictly followed the client coding and design guidelines.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

With Lectures: Add status bar to lecture creation and edit mode #9655 the guided mode will be removed and a "Status-bar-edit-view" will be introduced, with the sections title, period, attachments, and units.

As attachments and units are saved directly when being edited, but for title and period a save button will explicitly need to be pressed the user shall not be able to leave the edit page mistakenly without saving the changes to the title and period section.
To keep the scope of #9655 at a manageable size, we introduce the warning modal beforehand.

Description

  • Introduced signals in touched components
  • Added a new modal that is displayed when the lecture edit view (not in guided mode, as it will be removed soon) is closed without saving changes to the title or period section
  • Added isPeriodSectionValid computed signal to period section component, also renamed and moved the component as the wizard will be removed soon
  • Removed duplicated datepicker code in lecture-update.component.html and using lecture-period.component.ts instead
  • Disabled save button for lecture if no changes were made to title / period section
  • Removed unused translations competenciesStepTitle and competenciesStepMessage
  • Adjusted and extended client tests

Steps for Testing

Prerequisites:

  • 1 Instructor
  1. Create a Lecture (in normal mode)
  2. Verify all entries are saved properly (title, shortname, description, dates)
  3. Edit the Lecture
  4. Verify the edited information was saved properly
  5. Verify that the Save button is only enabled if you made a change to the lecture
  6. Edit the lecture and navigate somewhere else without saving
  7. See that modal appears which asks if the changes shall really be dismissed
  8. Verify that with cancel you stay on the edit page
  9. Verify that with save the changes are dismissed

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

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
exercise-title-channel-name.component.ts 96.77%
close-edit-lecture-modal.component.ts 88.88% (no significant logic in component) ❌
hasLectureUnsavedChanges.guard.ts 100%
lecture-period.component.ts 60% (no significant logic in component) ❌
lecture-title-channel-name.component.ts 100%
lecture-update.component.ts 90.09%
lecture.route.ts 0%
title-channel-name.component.ts 100%

Screenshots

saveButtonDisabledAndDismissModal-10023

image

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a modal for handling unsaved changes when editing lectures.
    • Added a guard to prevent navigation away from the lecture update component if there are unsaved changes.
    • Added a new component for updating lecture periods with real-time validation of date inputs.
    • Enhanced the TitleChannelNameComponent and ExerciseTitleChannelNameComponent for streamlined output event handling.
    • Updated the lecture update wizard to simplify user interactions.
  • Bug Fixes

    • Updated input and output handling in various components to ensure proper event emission and data binding.
  • Documentation

    • Updated localization files to enhance user prompts regarding unsaved changes and removed outdated competency-related messages.
  • Tests

    • Added new test specifications for the modal and guard functionalities, ensuring comprehensive coverage of their behaviors.

@florian-glombik florian-glombik added feature client Pull requests that update TypeScript code. (Added Automatically!) small lecture Pull requests that affect the corresponding module labels Dec 14, 2024
@florian-glombik florian-glombik added this to the 7.8.0 milestone Dec 14, 2024
@florian-glombik florian-glombik self-assigned this Dec 14, 2024
@github-actions github-actions bot added tests and removed lecture Pull requests that affect the corresponding module labels Dec 14, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between c249e87 and 4afc64f.

📒 Files selected for processing (3)
  • src/main/webapp/app/lecture/close-edit-lecture-dialog/close-edit-lecture-modal.component.html (1 hunks)
  • src/main/webapp/i18n/de/lecture.json (2 hunks)
  • src/main/webapp/i18n/en/lecture.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/webapp/app/lecture/close-edit-lecture-dialog/close-edit-lecture-modal.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/i18n/de/lecture.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".

🔇 Additional comments (3)
src/main/webapp/i18n/en/lecture.json (2)

83-83: LGTM: Clear and concise message

The updated message clearly communicates the purpose of the units step.


93-97: LGTM: Well-structured modal translations

The new dismissChangesModal section provides clear and consistent messaging with proper handling of singular/plural cases.

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

83-83: LGTM: Proper informal German translation

The message correctly uses the informal form and maintains good German grammar structure.

src/main/webapp/i18n/de/lecture.json Outdated Show resolved Hide resolved
Copy link
Contributor

@ole-ve ole-ve 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 changing translations

Copy link
Contributor

@az108 az108 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 after changes 👍

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

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

Tested on TS3. Everything works as described

Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

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

Tested on TS2, everything works perfectly.

@florian-glombik florian-glombik added ready to merge maintainer-approved The feature maintainer has approved the PR labels Dec 18, 2024
@krusche krusche merged commit 92b2aad into develop Dec 18, 2024
18 of 25 checks passed
@krusche krusche deleted the feature/lectures/add-dismiss-modal-for-unsaved-changes-to-title-or-period branch December 18, 2024 14:06
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!) feature maintainer-approved The feature maintainer has approved the PR ready for review ready to merge small tests
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

7 participants