-
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
Learning paths
: Redesign learning path instructor view
#9144
Changes from all commits
6557bfb
74093a5
ce4d57a
3f6f4ee
ce68ec4
42f53c3
61b8d78
c51bd46
ed604b4
b6e27b2
0727b08
748e701
6e24fd3
d38de02
e9acb9e
487ad36
168070c
f73e021
cfdcf8f
82dd171
6a87251
451bcde
b25ede5
408b34d
8bf9f64
a98a156
0da10a7
fd3d83b
6576e7b
68df4ae
41a4a84
f1bcbe3
52dcaa7
22b2df7
c1b927e
7fe32c7
0ca2f03
4844d40
11c6649
23c5ba7
d6e5f8b
8f0b2b2
bc613ce
1f3725f
b1ff6b5
5a4e142
2ded01c
b93d129
2898a31
6656f14
cff42c9
fb380b5
fd6c569
130abaa
94c1016
b732707
7fb8a9b
1935c2f
59e21d7
1fd81fb
6d9cd7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ | |
import de.tum.cit.aet.artemis.atlas.repository.CompetencyProgressRepository; | ||
import de.tum.cit.aet.artemis.atlas.repository.CompetencyRelationRepository; | ||
import de.tum.cit.aet.artemis.atlas.repository.CompetencyRepository; | ||
import de.tum.cit.aet.artemis.atlas.repository.CourseCompetencyRepository; | ||
import de.tum.cit.aet.artemis.atlas.repository.LearningPathRepository; | ||
import de.tum.cit.aet.artemis.atlas.service.competency.CompetencyProgressService; | ||
import de.tum.cit.aet.artemis.core.domain.Course; | ||
|
@@ -89,10 +90,13 @@ public class LearningPathService { | |
|
||
private final StudentParticipationRepository studentParticipationRepository; | ||
|
||
private final CourseCompetencyRepository courseCompetencyRepository; | ||
|
||
public LearningPathService(UserRepository userRepository, LearningPathRepository learningPathRepository, CompetencyProgressRepository competencyProgressRepository, | ||
LearningPathNavigationService learningPathNavigationService, CourseRepository courseRepository, CompetencyRepository competencyRepository, | ||
CompetencyRelationRepository competencyRelationRepository, LearningPathNgxService learningPathNgxService, | ||
LectureUnitCompletionRepository lectureUnitCompletionRepository, StudentParticipationRepository studentParticipationRepository) { | ||
LectureUnitCompletionRepository lectureUnitCompletionRepository, StudentParticipationRepository studentParticipationRepository, | ||
CourseCompetencyRepository courseCompetencyRepository) { | ||
Comment on lines
+93
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider reducing constructor dependencies to improve maintainability The Consider refactoring the class to adhere to the Single Responsibility Principle by delegating some functionality to other services or components. This can reduce the number of dependencies and improve code modularity. |
||
this.userRepository = userRepository; | ||
this.learningPathRepository = learningPathRepository; | ||
this.competencyProgressRepository = competencyProgressRepository; | ||
|
@@ -103,6 +107,7 @@ public LearningPathService(UserRepository userRepository, LearningPathRepository | |
this.learningPathNgxService = learningPathNgxService; | ||
this.lectureUnitCompletionRepository = lectureUnitCompletionRepository; | ||
this.studentParticipationRepository = studentParticipationRepository; | ||
this.courseCompetencyRepository = courseCompetencyRepository; | ||
} | ||
|
||
/** | ||
|
@@ -298,20 +303,11 @@ else if (learningPath.isStartedByStudent()) { | |
* @return dto containing the health status and additional information (missing learning paths) if needed | ||
*/ | ||
public LearningPathHealthDTO getHealthStatusForCourse(@NotNull Course course) { | ||
if (!course.getLearningPathsEnabled()) { | ||
return new LearningPathHealthDTO(Set.of(LearningPathHealthDTO.HealthStatus.DISABLED)); | ||
} | ||
|
||
Set<LearningPathHealthDTO.HealthStatus> status = new HashSet<>(); | ||
Long numberOfMissingLearningPaths = checkMissingLearningPaths(course, status); | ||
checkNoCompetencies(course, status); | ||
checkNoRelations(course, status); | ||
|
||
// if no issues where found, add OK status | ||
if (status.isEmpty()) { | ||
status.add(LearningPathHealthDTO.HealthStatus.OK); | ||
} | ||
|
||
return new LearningPathHealthDTO(status, numberOfMissingLearningPaths); | ||
} | ||
|
||
|
@@ -366,6 +362,25 @@ public LearningPathCompetencyGraphDTO generateLearningPathCompetencyGraph(@NotNu | |
return new LearningPathCompetencyGraphDTO(progressDTOs, relationDTOs); | ||
} | ||
|
||
/** | ||
* Generates the graph of competencies with the student's progress for the given learning path. | ||
* | ||
* @param courseId the id of the course for which the graph should be generated | ||
* @return dto containing the competencies and relations of the learning path | ||
*/ | ||
JohannesWt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public LearningPathCompetencyGraphDTO generateLearningPathCompetencyInstructorGraph(long courseId) { | ||
List<CourseCompetency> competencies = courseCompetencyRepository.findByCourseIdOrderById(courseId); | ||
Set<CompetencyGraphNodeDTO> progressDTOs = competencies.stream().map(competency -> { | ||
double averageMasteryProgress = competencyProgressRepository.findAverageOfAllNonZeroStudentProgressByCompetencyId(competency.getId()); | ||
return CompetencyGraphNodeDTO.of(competency, averageMasteryProgress, CompetencyGraphNodeDTO.CompetencyNodeValueType.AVERAGE_MASTERY_PROGRESS); | ||
}).collect(Collectors.toSet()); | ||
Comment on lines
+373
to
+376
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Optimize database queries to prevent N+1 query issue The method double averageMasteryProgress = competencyProgressRepository.findAverageOfAllNonZeroStudentProgressByCompetencyId(competency.getId()); This can lead to performance issues due to the N+1 query problem, especially when dealing with a large number of competencies. Consider modifying the repository method to fetch the average mastery progress for all competencies in a single query. This can be achieved by adding a method that retrieves averages grouped by competency IDs. Here's an example of how you might adjust the code: // In the repository:
Map<Long, Double> averageMasteryProgresses = competencyProgressRepository.findAverageProgressByCompetencyIds(competencyIds);
// In the service method:
Set<CompetencyGraphNodeDTO> progressDTOs = competencies.stream().map(competency -> {
double averageMasteryProgress = averageMasteryProgresses.getOrDefault(competency.getId(), 0.0);
return CompetencyGraphNodeDTO.of(competency, averageMasteryProgress, CompetencyGraphNodeDTO.CompetencyNodeValueType.AVERAGE_MASTERY_PROGRESS);
}).collect(Collectors.toSet()); This approach reduces the number of database queries and improves performance. 🧹 Nitpick (assertive) Consider preserving the order of competencies The Consider collecting into a You can modify the collection to: List<CompetencyGraphNodeDTO> progressDTOs = competencies.stream().map(competency -> {
// existing mapping logic
}).collect(Collectors.toList()); |
||
|
||
Set<CompetencyRelation> relations = competencyRelationRepository.findAllWithHeadAndTailByCourseId(courseId); | ||
Set<CompetencyGraphEdgeDTO> relationDTOs = relations.stream().map(CompetencyGraphEdgeDTO::of).collect(Collectors.toSet()); | ||
|
||
return new LearningPathCompetencyGraphDTO(progressDTOs, relationDTOs); | ||
} | ||
Comment on lines
+372
to
+382
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor to eliminate code duplication with The method Here's how you can refactor:
Comment on lines
+371
to
+382
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add unit tests for To ensure the new method works as intended and to prevent future regressions, please add unit tests for Would you like assistance in creating unit tests for this method? |
||
|
||
/** | ||
* Generates Ngx graph representation of the learning path graph. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<div class="learning-paths-analytics-container"> | ||
<h5 class="m-0" jhiTranslate="artemisApp.learningPathManagement.learningPathsAnalytics.title"></h5> | ||
<hr class="my-2" /> | ||
<div class="row h-100 m-0 gap-3"> | ||
<div class="col-2 p-0 learning-paths-analytics-graph-selection-container"> | ||
<ng-container [ngTemplateOutlet]="radioTemplate" [ngTemplateOutletContext]="{ $implicit: CompetencyGraphNodeValueType.AVERAGE_MASTERY_PROGRESS }"></ng-container> | ||
</div> | ||
<div class="col p-0"> | ||
@if (isLoading()) { | ||
<div class="row justify-content-center p-2"> | ||
<div class="spinner-border text-primary" role="status"> | ||
<span class="sr-only" jhiTranslate="loading"></span> | ||
</div> | ||
</div> | ||
} @else if (instructorCompetencyGraph()) { | ||
<jhi-competency-graph [competencyGraph]="instructorCompetencyGraph()!" /> | ||
} | ||
</div> | ||
</div> | ||
</div> | ||
|
||
<ng-template #radioTemplate let-competencyNodeValueType> | ||
<div class="row m-0 align-items-center"> | ||
<input type="radio" class="col-md-auto" [checked]="valueSelection() === competencyNodeValueType" /> | ||
<label class="col-md-auto pe-0 text-break" [jhiTranslate]="'artemisApp.learningPathManagement.learningPathsAnalytics.graphSelection.' + competencyNodeValueType"></label> | ||
</div> | ||
</ng-template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
.learning-paths-analytics-container { | ||
height: 500px; | ||
|
||
.learning-paths-analytics-graph-selection-container { | ||
border-right: var(--bs-border-width) solid var(--border-color); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { Component, effect, inject, input, signal } from '@angular/core'; | ||
import { LearningPathApiService } from 'app/course/learning-paths/services/learning-path-api.service'; | ||
import { CompetencyGraphDTO, CompetencyGraphNodeValueType } from 'app/entities/competency/learning-path.model'; | ||
import { AlertService } from 'app/core/util/alert.service'; | ||
import { ArtemisSharedCommonModule } from 'app/shared/shared-common.module'; | ||
import { CompetencyGraphComponent } from 'app/course/learning-paths/components/competency-graph/competency-graph.component'; | ||
import { onError } from 'app/shared/util/global.utils'; | ||
|
||
@Component({ | ||
selector: 'jhi-learning-paths-analytics', | ||
standalone: true, | ||
imports: [ArtemisSharedCommonModule, CompetencyGraphComponent], | ||
templateUrl: './learning-paths-analytics.component.html', | ||
styleUrl: './learning-paths-analytics.component.scss', | ||
}) | ||
export class LearningPathsAnalyticsComponent { | ||
protected readonly CompetencyGraphNodeValueType = CompetencyGraphNodeValueType; | ||
|
||
private readonly learningPathApiService = inject(LearningPathApiService); | ||
private readonly alertService = inject(AlertService); | ||
|
||
readonly courseId = input.required<number>(); | ||
|
||
readonly isLoading = signal<boolean>(false); | ||
readonly instructorCompetencyGraph = signal<CompetencyGraphDTO | undefined>(undefined); | ||
|
||
readonly valueSelection = signal<CompetencyGraphNodeValueType>(CompetencyGraphNodeValueType.AVERAGE_MASTERY_PROGRESS); | ||
|
||
constructor() { | ||
effect(() => this.loadInstructionCompetencyGraph(this.courseId()), { allowSignalWrites: true }); | ||
} | ||
|
||
private async loadInstructionCompetencyGraph(courseId: number): Promise<void> { | ||
try { | ||
this.isLoading.set(true); | ||
const instructorCompetencyGraph = await this.learningPathApiService.getLearningPathInstructorCompetencyGraph(courseId); | ||
this.instructorCompetencyGraph.set(instructorCompetencyGraph); | ||
} catch (error) { | ||
onError(this.alertService, error); | ||
} finally { | ||
this.isLoading.set(false); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
<div> | ||
<div class="row m-0 align-items-center justify-content-between"> | ||
<h5 class="m-0 col-md-auto p-0" jhiTranslate="artemisApp.learningPathManagement.learningPathsConfiguration.title"></h5> | ||
@if (isEditMode()) { | ||
<button id="save-learning-paths-configuration-button" class="btn btn-primary btn-sm col-md-auto" (click)="saveLearningPathsConfiguration()"> | ||
@if (isSaving()) { | ||
<fa-icon [icon]="faSpinner" animation="spin" /> | ||
} | ||
<span jhiTranslate="artemisApp.learningPathManagement.learningPathsConfiguration.saveButtonLabel"></span> | ||
</button> | ||
} @else { | ||
<button | ||
id="edit-learning-paths-configuration-button" | ||
class="btn btn-secondary btn-sm col-md-auto" | ||
(click)="enableEditMode()" | ||
jhiTranslate="artemisApp.learningPathManagement.learningPathsConfiguration.editButtonLabel" | ||
></button> | ||
} | ||
</div> | ||
<hr class="my-2" /> | ||
<div class="row align-items-center m-0 learning-paths-management-container"> | ||
@if (isConfigLoading()) { | ||
<div class="row justify-content-center p-2"> | ||
<div class="spinner-border text-primary" role="status"> | ||
<span class="sr-only" jhiTranslate="loading"></span> | ||
</div> | ||
</div> | ||
} @else { | ||
<input | ||
id="include-all-graded-exercises-checkbox" | ||
type="checkbox" | ||
class="col-md-auto" | ||
(change)="toggleIncludeAllGradedExercises()" | ||
[checked]="includeAllGradedExercisesEnabled()" | ||
[disabled]="!isEditMode()" | ||
/> | ||
<label class="col-md-auto" jhiTranslate="artemisApp.learningPathManagement.learningPathsConfiguration.configuration.includeAllGradedExercises"></label> | ||
<jhi-help-icon class="col-md-auto p-0" text="artemisApp.learningPathManagement.learningPathsConfiguration.configuration.includeAllGradedExercisesToolTip" /> | ||
} | ||
</div> | ||
</div> |
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.
🧹 Nitpick (assertive)
💡 Codebase verification
Inconsistency Detected in
HealthStatus
Enum ModificationsDISABLED
value is still referenced inLearningPathServiceTest.java
andLearningPathService.java
but has been removed from theHealthStatus
enum.🔗 Analysis chain
Verify the consistency of enum values across the codebase
The
HealthStatus
enum has been modified to remove theOK
status. This change aligns with the PR objectives of providing more specific health statuses for learning paths. However, there's a discrepancy noted in a previous review comment about the removal of aDISABLED
status in the client code.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in the usage of
HealthStatus
across the codebase.Consider adding a comment above the enum to explain the purpose of each status, which would improve code documentation and maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 1103