-
Notifications
You must be signed in to change notification settings - Fork 301
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
: Remove guided mode exercise creation shortcut
#10022
Lectures
: Remove guided mode exercise creation shortcut
#10022
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of navigation and wizard mode functionality across multiple components in an Angular application. The changes primarily focus on removing back-to-wizard navigation options, simplifying component logic, and streamlining user interfaces. Key modifications include eliminating Changes
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.ts (1)
1-2
: LGTM: Simplified navigation handlingThe removal of the Router dependency and consolidation of navigation logic through
navigationUtilService
improves code consistency and maintainability. This change aligns with the PR objective to remove guided mode functionality.This architectural improvement:
- Reduces component complexity
- Centralizes navigation logic
- Makes the codebase more maintainable
src/main/webapp/app/course/manage/course-management-exercises.component.ts (1)
Line range hint
40-45
: Consider safer null handling in route.parent access.The non-null assertion operator (
!
) inthis.route.parent!.data
could lead to runtime errors if the component is used outside the expected routing context.Consider using optional chaining or adding a null check:
-this.route.parent!.data.subscribe(({ course }) => { +this.route.parent?.data.subscribe(({ course }) => { if (course) { this.course = course; } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/webapp/app/course/manage/course-management-exercises.component.html
(0 hunks)src/main/webapp/app/course/manage/course-management-exercises.component.ts
(2 hunks)src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.component.ts
(2 hunks)src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.ts
(1 hunks)src/main/webapp/app/exercises/quiz/manage/quiz-exercise-update.component.ts
(0 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts
(0 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
(1 hunks)src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.html
(0 hunks)src/main/webapp/i18n/de/lecture.json
(0 hunks)src/main/webapp/i18n/en/lecture.json
(0 hunks)src/test/javascript/spec/component/text-exercise/text-exercise-update.component.spec.ts
(0 hunks)
💤 Files with no reviewable changes (7)
- src/main/webapp/app/course/manage/course-management-exercises.component.html
- src/main/webapp/i18n/en/lecture.json
- src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.html
- src/main/webapp/i18n/de/lecture.json
- src/main/webapp/app/exercises/quiz/manage/quiz-exercise-update.component.ts
- src/test/javascript/spec/component/text-exercise/text-exercise-update.component.spec.ts
- src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.component.ts (1)
src/main/webapp/app/course/manage/course-management-exercises.component.ts (1)
src/main/webapp/app/exercises/modeling/manage/modeling-exercise-update.component.ts (1)
🔇 Additional comments (6)
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise-update.component.ts (2)
36-36
: LGTM: Icon property follows Angular best practices
The addition of the faQuestionCircle
icon as a protected readonly property follows Angular's recommended practices for icon management.
65-68
: LGTM: Improved comment accuracy and subscription management
The comment correction from "Subcriptions" to "Subscriptions" improves code documentation quality. The subscription properties are well-organized and properly typed.
src/main/webapp/app/course/manage/course-management-exercises.component.ts (2)
1-1
: LGTM! Good use of modern Angular patterns.
The refactoring to use inject()
instead of constructor injection follows current Angular best practices and makes the code more maintainable.
Also applies to: 3-3, 35-35
Line range hint 8-91
: LGTM! Well-structured component implementation.
The component demonstrates:
- Clear separation of concerns
- Proper TypeScript typing
- Efficient exercise counting and filtering logic
- Good use of extension points for flexibility
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (2)
Line range hint 1-24
: LGTM! Well-structured component using modern Angular features.
The component follows Angular best practices with proper typing, null safety, and the new input() syntax.
128-130
: Verify navigation parameter changes across dependent components.
The removal of wizard-related query parameters aligns with the PR objective to remove the guided mode exercise creation shortcut. However, we should verify that components handling these navigation parameters are updated accordingly.
Let's check for any components that might expect the removed parameters:
✅ Verification successful
Let me search for more specific usage of navigation parameters in the target component and its related components.
Let me check one final aspect - the history of this component to understand what parameters were removed.
Based on the comprehensive search results, I can now provide the final response:
Navigation parameter changes are safe to proceed
The removal of wizard-related query parameters appears to be isolated to this component. Our analysis shows:
- No other components in the codebase are reading the removed wizard-related query parameters (
shouldHaveBackButtonToWizard
orstep
) - The only query parameter usage in the lecture unit management is the
lectureId
, which is being preserved - No dependent components were found that expect the removed parameters for exercise creation navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for components that might expect the removed query parameters
# Look for components that read 'shouldHaveBackButtonToWizard' or 'step' from query parameters
# Search for query parameter usage in TypeScript files
echo "Searching for query parameter usage in components:"
rg -t ts "queryParamMap.*(?:shouldHaveBackButtonToWizard|step)" src/main/webapp/
# Search for template bindings that might use these parameters
echo "Searching for template bindings:"
rg -t html "(?:\[.*\]|\{\{).*(?:shouldHaveBackButtonToWizard|step)" src/main/webapp/
Length of output: 10649
Script:
#!/bin/bash
# Search for components that handle navigation to/from exercise creation
# Look for router navigation and parameter handling related to exercise creation
# Search for components that navigate to exercise creation
echo "Searching for navigation to exercise creation:"
rg -t ts "router.navigate.*exercise.*create" src/main/webapp/
# Search for components that read exercise-related query parameters
echo "Searching for exercise-related query parameter handling:"
rg -t ts "queryParamMap.*exercise" src/main/webapp/
# Search specifically in the exercise unit directory
echo "Searching in exercise unit components:"
rg -t ts "(queryParams|queryParamMap)" src/main/webapp/app/lecture/lecture-unit/
Length of output: 846
Script:
#!/bin/bash
# Look for the specific component in the PR changes
echo "Searching for the original parameters in the component:"
git diff HEAD^ -- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
# Look for any related test files that might verify navigation
echo "Searching for related test files:"
fd -t f "create-exercise-unit.component.spec.ts" src/main/webapp/
Length of output: 405
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3, the "Create new exercise" button is removed from the guided mode. Also, tested the "back to lecture" with the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3. No "Create new exercise" is displayed in guided mode. No Back to lecture button displayed with the added URL. I created the following exercises:
- A programming exercise
- A quiz in synchronized mode
- A modelling exercise
- A text exercise
- A file upload exercise
The save button works for all the above exercises
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS1. No button "Create new exercise" was shown. Added parameters to url and no back to lecture button was displayed.
Checklist
General
Client
Motivation and Context
It is unlikely that editors / instructors will start creating exercises while they are creating a lecture. This step will usually have been completed before the lecture creation and is not something that will be done within a matter of minutes.
While this feature, of navigating from the lecture creation to the exercise creation and back, is only available in the guided mode and the guided mode will be removed in Lectures: Add status bar to lecture creation and edit mode #9655, this PR already removes the "exercise creation shortcut" to keep #9655 at a manageable size.
Description
Steps for Testing
Prerequisites:
?shouldHaveBackButtonToWizard=true&lectureId=83&step=4
Back to lecture
button displayedTestserver 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
Manual Tests
Test Coverage
Screenshots
Old Version
New Version
Create new exercise
shortcut removed from guided modeBack to lecture
button removed from exercise overview (can be tested by adding parameters to URL)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests