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

Learning path: Refactor learning path navigation overview #8816

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Jun 17, 2024

Checklist (+900/-300)

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

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 added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

The navigation overview (where all the learning objects of the learning path are shown) should be grouped by competency.

Description

The navigation overview now shows a list of the competencies of the learning path. Each competency can be expanded to show its corresponding learning objects. These objects can be selected to navigate to that specific learning objects.
To achieve these client tests two additional endpoints have been added on the server.
Stacked on #8733

Testing this PR is straightforward on TS5 (Course Johannes Wiest) because the necessary competencies and other elements are already set up.

  • one to fetch all competencies of a learning path
  • one to fetch all learning objects of a competency

Steps for Testing

Prerequisites:

  • 1 Student
  • 1 Course
  • at least 1 competency with linked learning objects (LectureUnit or Exercise)
  1. Log in to Artemis
  2. Navigate to Course
  3. Navigate to Learning path
  4. Navigate back and forth in learning path (Previous and Next-Button)
  5. Open Competency overview dropdown
  6. Open Competency learning objects accordion
  7. Navigate to a learning object by clicking on an item

Info for code review

Due to merging develop into this branch the changes look more than there are from this PR. This PR includes more around (+900/-300) lines and changed the following files:

Server changes:
repository/CompetencyProgressRepository.java
repository/CompetencyRepository.java
repository/CourseCompetencyRepository.java
service/LearningObjectService.java
service/competency/CompetencyProgressService.java
service/learningpath/LearningPathNavigationService.java
service/learningpath/LearningPathRecommendationService.java
web/rest/LearningPathResource.java
web/rest/dto/competency/LearningPathNavigationObjectDTO.java

Server tests:
src/test/java/de/tum/in/www1/artemis/competency/LearningPathIntegrationTest.java

Client changes:
course/learning-paths/components/competency-graph-modal/competency-graph-modal.component.ts
course/learning-paths/components/competency-graph/competency-graph.component.ts
course/learning-paths/components/competency-node/competency-node.component.ts
course/learning-paths/components/learning-path-lecture-unit/learning-path-lecture-unit.component.ts
course/learning-paths/components/learning-path-nav-overview-learning-objects/learning-path-nav-overview-learning-objects.component.html
course/learning-paths/components/learning-path-nav-overview-learning-objects/learning-path-nav-overview-learning-objects.component.ts
course/learning-paths/components/learning-path-nav-overview-learning-objects/learning-path-nav-overview-learning-objects.component.scss
course/learning-paths/components/learning-path-nav-overview/learning-path-nav-overview.component.html
course/learning-paths/components/learning-path-nav-overview/learning-path-nav-overview.component.scss
course/learning-paths/components/learning-path-nav-overview/learning-path-nav-overview.component.ts
course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.html
course/learning-paths/components/learning-path-student-nav/learning-path-student-nav.component.ts
course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts
course/learning-paths/services/base-api-http.service.ts
course/learning-paths/services/learning-path-api.service.ts
course/learning-paths/services/learning-path-navigation.service.ts
entities/competency/learning-path.model.ts

Client tests:
spec/component/learning-paths/components/learning-path-nav-overview-learning-objects.component.spec.ts
spec/component/learning-paths/components/learning-path-nav-overview.component.spec.ts
spec/component/learning-paths/components/learning-path-nav.component.spec.ts
spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts
spec/service/learning-path/learning-path-api.service.spec.ts

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







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
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
competency-form.component.ts 87.77%
competency-graph-modal.component.ts 90.9%
competency-graph.component.ts 85.29%
competency-node.component.ts 100%
learning-path-lecture-unit.component.ts 81.48%
learning-path-nav-overview-learning-objects.component.ts 90%
learning-path-nav-overview.component.ts 90.62%
learning-path-student-nav.component.ts 100%
learning-path.service.ts 75%
learning-path-student-page.component.ts 97.43%
learning-path-container.component.ts 87%
base-api-http.service.ts 72.22%
learning-path-api.service.ts 100%
learning-path-navigation.service.ts 100%
learning-path.model.ts 96.15%

Server

Class/File Line Coverage Confirmation (assert/expect)
Exercise.java 91%
LectureUnit.java 100%
LearningObjectService.java 89%
CompetencyProgressService.java 94%
LearningPathNavigationService.java 87%
LearningPathNgxService.java 97%
LearningPathRecommendationService.java 97%
LearningPathService.java 97%
LearningPathResource.java 100%

Screenshots

image

@JohannesStoehr JohannesStoehr added the stacked-pr PR that depends on another PR label Jun 17, 2024
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Jun 17, 2024
@JohannesStoehr JohannesStoehr changed the title Fix Server Tests Leanring path: Add endpoint for quick navigation Jun 18, 2024
@github-actions github-actions bot added the tests label Jun 18, 2024
@JohannesWt JohannesWt changed the title Leanring path: Add endpoint for quick navigation Learning path: Add endpoint for quick navigation Jun 18, 2024
JohannesStoehr and others added 7 commits June 18, 2024 16:25
…e/learning-paths/refactor-student-ui-new-navigation-endpoints
…into feature/learning-paths/refactor-student-ui-new-navigation-endpoints

# Conflicts:
#	src/main/java/de/tum/in/www1/artemis/repository/CompetencyProgressRepository.java
#	src/main/java/de/tum/in/www1/artemis/service/learningpath/LearningPathRecommendationService.java
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Jun 24, 2024
Copy link

⚠️ Unable to deploy to test servers ⚠️

The docker build needs to run through before deploying.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Jun 24, 2024
@JohannesWt JohannesWt added deploy:artemis-test5 and removed deployment-error Added by deployment workflows if an error occured labels Jun 24, 2024
@JohannesWt JohannesWt temporarily deployed to artemis-test5.artemis.cit.tum.de June 24, 2024 10:26 — with GitHub Actions Inactive
@JohannesWt JohannesWt changed the title Learning path: Add endpoint for quick navigation Learning path: Refactor learning path navigation overview Jun 24, 2024
@JohannesWt JohannesWt removed the deployment-error Added by deployment workflows if an error occured label Jun 25, 2024
Copy link

⚠️ Unable to deploy to test servers ⚠️

The docker build needs to run through before deploying.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Jun 25, 2024
@github-actions github-actions bot removed the deployment-error Added by deployment workflows if an error occured label Jun 25, 2024
@pzdr7 pzdr7 temporarily deployed to artemis-test5.artemis.cit.tum.de June 25, 2024 16:12 — with GitHub Actions Inactive
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.

Tested on ts5, works as expected. Some UI sidenotes that can be done in a follow up PR:

  • I see a scrollbar in content part, which can be hidden if you don't have an exceeding content.
Screenshot 2024-06-25 at 19 49 32 - We might want to have the overview scrollbar in auto mode instead scroll Screenshot 2024-06-25 at 19 50 38

@JohannesWt
Copy link
Contributor

Tested on ts5, works as expected. Some UI sidenotes that can be done in a follow up PR:

  • I see a scrollbar in content part, which can be hidden if you don't have an exceeding content.

Screenshot 2024-06-25 at 19 49 32 - We might want to have the overview scrollbar in auto mode instead scroll Screenshot 2024-06-25 at 19 50 38

Unfortunately we have to use scroll overview here. If we use "auto" the dropdown will grow indefinitely in size ignoring the max-height. The horizontal scroll in the content part is already documented in an issue and will be solved in a follow up PR.

@az108
Copy link
Contributor

az108 commented Jun 25, 2024

I like the changes, maybe in a future PR we could show somewhere which compentency one is at rn. Because right now I would have to click on the dropdown to see that i am in competency 2. 😄
image

@JohannesWt
Copy link
Contributor

I like the changes, maybe in a future PR we could show somewhere which compentency one is at rn. Because right now I would have to click on the dropdown to see that i am in competency 2. 😄 image

That's exactly the plan. In the future the current competency should be marked and opened, as soon as you open the drop down.

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.

Tested on TS5, works with no problems and Code LGTM 👍

…e/learning-paths/refactor-student-ui-new-navigation-endpoints
@MaximilianAnzinger MaximilianAnzinger merged commit eab4fb4 into feature/learning-paths/refactor-student-ui Jun 26, 2024
22 of 25 checks passed
@MaximilianAnzinger MaximilianAnzinger deleted the feature/learning-paths/refactor-student-ui-new-navigation-endpoints branch June 26, 2024 08:51
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 server Pull requests that update Java code. (Added Automatically!) stacked-pr PR that depends on another PR tests
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

9 participants