-
Notifications
You must be signed in to change notification settings - Fork 3
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
Catching LearningTask events. #127
Conversation
Instructor enrollment and monitoring enhancements
…ingBlocks into two modules.
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.
Comments inline.
...or.LearningTasks.Core/Domain/LearningTaskProgress/Events/TaskProgressEvents/ExampleOpened.cs
Show resolved
Hide resolved
...Tutor.LearningTasks.Core/Domain/LearningTaskProgress/Events/TaskProgressEvents/StepOpened.cs
Outdated
Show resolved
Hide resolved
...s/LearningTasks/Tutor.LearningTasks.Infrastructure/Tutor.LearningTasks.Infrastructure.csproj
Outdated
Show resolved
Hide resolved
...s/KnowledgeComponents/Tutor.KnowledgeComponents.Infrastructure/KnowledgeComponentsStartup.cs
Outdated
Show resolved
Hide resolved
src/Modules/LearningTasks/Tutor.LearningTasks.Core/Domain/LearningTaskProgress/TaskProgress.cs
Outdated
Show resolved
Hide resolved
@@ -55,6 +62,8 @@ public Result<TaskProgressDto> ViewStep(int unitId, int id, int stepId, int lear | |||
|
|||
var taskProgress = result.Value; | |||
taskProgress.ViewStep(stepId); | |||
_progressRepository.UpdateEvents(taskProgress); | |||
|
|||
return Update(taskProgress); |
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.
Will this generate two events (one for the UpdateEvents
and another for Update
in line 67)?
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.
Check if this method specifically removes any old events from existence. I do not think this will be the case, but better safe than sorry.
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.
It does not save any events without _progressRepository.UpdateEvents(taskProgress);.
return Update(taskProgress); | ||
} | ||
|
||
public Result OpenSubmission(int unitId, int id, int stepId, int learnerId) |
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.
I think all of these non-state changing methods can be simplified to reduce code duplication. This might not work, as I am writing from memory instead of in an IDE:
public Result PlayExampleVideo(int unitId, int id, int stepId, int learnerId)
{
return HandleNonStateChangingEvent(unitId, id, stepId, learnerId, (taskProgress, step) => taskProgress.PlayExampleVideo(step));
}
public Result PauseExampleVideo(int unitId, int id, int stepId, int learnerId)
{
return HandleNonStateChangingEvent(unitId, id, stepId, learnerId, (taskProgress, step) => taskProgress.PauseExampleVideo(step));
}
public Result FinishExampleVideo(int unitId, int id, int stepId, int learnerId)
{
return HandleNonStateChangingEvent(unitId, id, stepId, learnerId, (taskProgress, step) => taskProgress.FinishExampleVideo(step));
}
private Result HandleNonStateChangingEvent(int unitId, int id, int stepId, int learnerId, Action<TaskProgress, int> action)
{
var result = GetTaskProgress(unitId, learnerId, id);
if (result.IsFailed)
return result.ToResult();
var taskProgress = result.Value;
action(taskProgress, stepId);
_progressRepository.UpdateEvents(taskProgress);
return UnitOfWork.Save();
}
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.
Nice
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.
Apart from the issues in the comments below, the PR text was not updated to reflect the required database changes and the TestData SQL script was not updated to delete events.
I will solve the listed issues and resolve the Sonar issues as well.
@@ -12,14 +12,14 @@ | |||
|
|||
namespace Tutor.KnowledgeComponents.Core.UseCases.Analysis; | |||
|
|||
public class AssessmentAnalysisService : IAssessmentAnalysisService | |||
public class AssessmentAnalysisService<TEvent> : IAssessmentAnalysisService where TEvent : KnowledgeComponentEvent |
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.
Why are these services parametrized?
@@ -54,9 +56,9 @@ public void Retrieves_submissions() | |||
events.Count.ShouldBe(0); | |||
} | |||
|
|||
private static EventsController CreateController(IServiceScope scope, string id) | |||
private static EventsController<KnowledgeComponentEvent> CreateController(IServiceScope scope, string id) |
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.
Why is the controller parametrized?
var result = controller.FinishExampleVideo(-2, -2, -4, "videoUrl"); | ||
|
||
result.ShouldBeOfType<OkResult>(); | ||
} |
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.
A Test Suite with 300+ lines of code (or really 200+) should be a signal (code smell) for refactoring.
|
||
namespace Tutor.API.Controllers.Instructor.Analysis; | ||
|
||
[Authorize(Policy = "instructorPolicy")] | ||
[Route("api/analysis/knowledge-components/events")] | ||
public class EventsController : BaseApiController | ||
public class EventsController<TEvent> : BaseApiController where TEvent : DomainEvent |
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.
No need for in the controller. I guess it was caused by a lack of focus, but we cannot parametrize all the layers of our application when we only need a paramterized infrastructure service.
Quality Gate passedIssues Measures |
🎉 This issue has been resolved in version 2.20.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Change goal
The goal of this PR was to enable the backend to capture the following events:
TaskOpened
StepOpened
SubmissionOpened
GuidanceOpened
ExampleOpened
ExampleVideoPlayed
ExampleVideoPaused
ExampleVideoFinished
StepSubmitted
TaskCompleted
Database changes
Add Events table to learningTasks using the following script.