Store course completed_at on pivot and improve test step UI #85
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add completed_at column to lms_course_user pivot table - Store course completion timestamp when user meets all requirements - Fix redirect loop by checking allStepsCompletedByUser for page access - Update Course::completedByUserAt() to read from pivot instead of calculating - Add maybeSetCompletedAtForUser() to set completion when requirements met - Call completion check after each step completion (supports retakes) - Improve test step spacing consistency - Update CourseResource label and simplify help text - Set default required_test_percentage to 0 Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ficate download after retake - Skip test percentage check when no test steps exist (prevents impossible completion) - Remove unused urlToFirstStepBelowPerfect property from CourseCompleted page - Clear course completed_at when retaking tests to re-evaluate eligibility
|
Bugbot Autofix prepared a fix for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (c8724dacc6)diff --git a/src/Livewire/TestStep.php b/src/Livewire/TestStep.php
--- a/src/Livewire/TestStep.php
+++ b/src/Livewire/TestStep.php
@@ -90,6 +90,10 @@
if ($this->step->completed_at) {
$this->step->progress()->delete();
}
+
+ // Clear course completion so it can be re-evaluated after the retake
+ $course = $this->step->lesson->course;
+ $course->users()->updateExistingPivot(Auth::id(), ['completed_at' => null]);
}
private function checkTestResults(): void
diff --git a/src/Models/Course.php b/src/Models/Course.php
--- a/src/Models/Course.php
+++ b/src/Models/Course.php
@@ -222,9 +222,13 @@
}
if ($this->required_test_percentage !== null) {
- $overall = $this->getOverallTestPercentageForUser($userId);
- if ($overall < (float) $this->required_test_percentage) {
- return;
+ $testSteps = $this->getOrderedTestSteps();
+ // Only apply test percentage requirement if there are test steps
+ if ($testSteps->isNotEmpty()) {
+ $overall = $this->getOverallTestPercentageForUser($userId);
+ if ($overall < (float) $this->required_test_percentage) {
+ return;
+ }
}
}
diff --git a/src/Pages/CourseCompleted.php b/src/Pages/CourseCompleted.php
--- a/src/Pages/CourseCompleted.php
+++ b/src/Pages/CourseCompleted.php
@@ -22,8 +22,6 @@
public ?int $requiredPercent = null;
- public ?string $urlToFirstStepBelowPerfect = null;
-
public array $testStepDetails = [];
protected static string|BackedEnum|null $navigationIcon = 'heroicon-o-trophy';
@@ -57,12 +55,15 @@
}
if ($this->course->required_test_percentage !== null) {
- $overall = $this->course->getOverallTestPercentageForUser(auth()->id());
- if ($overall < (float) $this->course->required_test_percentage) {
- $this->qualifiedForCertificate = false;
- $this->overallPercent = $overall;
- $this->requiredPercent = (int) $this->course->required_test_percentage;
- $this->urlToFirstStepBelowPerfect = $this->course->getUrlToFirstTestStepBelowPerfectForUser(auth()->id());
+ $testSteps = $this->course->getOrderedTestSteps();
+ // Only check test percentage if there are test steps
+ if ($testSteps->isNotEmpty()) {
+ $overall = $this->course->getOverallTestPercentageForUser(auth()->id());
+ if ($overall < (float) $this->course->required_test_percentage) {
+ $this->qualifiedForCertificate = false;
+ $this->overallPercent = $overall;
+ $this->requiredPercent = (int) $this->course->required_test_percentage;
+ }
}
} |
…certificates-3c08 Course completion and certificates
…operty, CourseCompleted rubric type Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot Autofix prepared a fix for 2 of the 2 bugs found in the latest run.
Or push these changes by commenting: Preview (42e829d74d)diff --git a/src/Models/Course.php b/src/Models/Course.php
--- a/src/Models/Course.php
+++ b/src/Models/Course.php
@@ -196,13 +196,34 @@
{
$pivot = $this->users()->where('user_id', $userId)->first()?->pivot;
- if (! $pivot || $pivot->completed_at === null) {
+ if ($pivot && $pivot->completed_at !== null) {
+ $at = $pivot->completed_at;
+
+ return $at instanceof DateTimeInterface ? $at->format('Y-m-d H:i:s') : (string) $at;
+ }
+
+ // Fallback: Calculate from StepUser records for backward compatibility.
+ // This handles existing completions before the completed_at column was added.
+ if (! $this->allStepsCompletedByUser($userId)) {
return null;
}
- $at = $pivot->completed_at;
+ // Check test percentage requirement (if any)
+ if ($this->required_test_percentage !== null) {
+ $testSteps = $this->getOrderedTestSteps();
+ if ($testSteps->isNotEmpty()) {
+ $overall = $this->getOverallTestPercentageForUser($userId);
+ if ($overall < (float) $this->required_test_percentage) {
+ return null;
+ }
+ }
+ }
- return $at instanceof DateTimeInterface ? $at->format('Y-m-d H:i:s') : (string) $at;
+ // Get the max completed_at from StepUser records
+ return StepUser::whereIn('step_id', $this->steps()->pluck('lms_steps.id'))
+ ->where('user_id', $userId)
+ ->whereNotNull('completed_at')
+ ->max('completed_at');
}
/**
@@ -223,13 +244,13 @@
if ($this->required_test_percentage !== null) {
$testSteps = $this->getOrderedTestSteps();
- if ($testSteps->isEmpty()) {
- return;
+ // Only check test percentage if there are test steps to check
+ if ($testSteps->isNotEmpty()) {
+ $overall = $this->getOverallTestPercentageForUser($userId);
+ if ($overall < (float) $this->required_test_percentage) {
+ return;
+ }
}
- $overall = $this->getOverallTestPercentageForUser($userId);
- if ($overall < (float) $this->required_test_percentage) {
- return;
- }
}
$this->users()->syncWithoutDetaching([$userId => ['completed_at' => now()]]); |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| @if(!$testPassed && $step->require_perfect_score) | ||
| <x-filament::section class="mt-8"> | ||
| @if($testGrade !== null && $testGrade < 100.0 && $step->require_perfect_score) |
There was a problem hiding this comment.
Grading exception leaves user stuck with no retake option
Medium Severity
The condition change from !$testPassed && $step->require_perfect_score to $testGrade !== null && $testGrade < 100.0 && $step->require_perfect_score causes a regression when grading fails with an exception. When gradeEntry() returns an Exception (e.g., missing rubric), $testGrade is set to null but $testPassed is false. The new condition requires $testGrade !== null, so neither the danger warning block nor the soft message block renders, meaning no "Retake Test" button appears. For non-optional steps requiring perfect score, the next button remains disabled, leaving the user stuck with no way to proceed or retry.
Additional Locations (1)
|
Bugbot Autofix prepared a fix for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (31d6810dee)diff --git a/database/migrations/add_completed_at_to_lms_course_user_table.php.stub b/database/migrations/add_completed_at_to_lms_course_user_table.php.stub
--- a/database/migrations/add_completed_at_to_lms_course_user_table.php.stub
+++ b/database/migrations/add_completed_at_to_lms_course_user_table.php.stub
@@ -2,6 +2,7 @@
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
return new class extends Migration
@@ -14,9 +15,62 @@
Schema::table('lms_course_user', function (Blueprint $table): void {
$table->timestamp('completed_at')->nullable()->after('updated_at');
});
+
+ // Backfill completed_at for users who have already completed all steps of a course
+ $this->backfillExistingCompletions();
}
/**
+ * Backfill completed_at for existing course completions.
+ * A user is considered to have completed a course if they have completed all steps.
+ */
+ private function backfillExistingCompletions(): void
+ {
+ // Get all course-user pairs from the pivot table
+ $courseUsers = DB::table('lms_course_user')
+ ->whereNull('completed_at')
+ ->get();
+
+ foreach ($courseUsers as $courseUser) {
+ $courseId = $courseUser->course_id;
+ $userId = $courseUser->user_id;
+
+ // Get all step IDs for this course (through lessons)
+ $stepIds = DB::table('lms_steps')
+ ->join('lms_lessons', 'lms_steps.lesson_id', '=', 'lms_lessons.id')
+ ->where('lms_lessons.course_id', $courseId)
+ ->pluck('lms_steps.id');
+
+ if ($stepIds->isEmpty()) {
+ continue;
+ }
+
+ // Count how many steps have been completed by this user
+ $completedCount = DB::table('lms_step_user')
+ ->whereIn('step_id', $stepIds)
+ ->where('user_id', $userId)
+ ->whereNotNull('completed_at')
+ ->count();
+
+ // If all steps are completed, set completed_at to the latest step completion time
+ if ($completedCount === $stepIds->count()) {
+ $latestCompletion = DB::table('lms_step_user')
+ ->whereIn('step_id', $stepIds)
+ ->where('user_id', $userId)
+ ->whereNotNull('completed_at')
+ ->max('completed_at');
+
+ if ($latestCompletion) {
+ DB::table('lms_course_user')
+ ->where('course_id', $courseId)
+ ->where('user_id', $userId)
+ ->update(['completed_at' => $latestCompletion]);
+ }
+ }
+ }
+ }
+
+ /**
* Reverse the migrations.
*/
public function down(): void
diff --git a/resources/views/livewire/test-step.blade.php b/resources/views/livewire/test-step.blade.php
--- a/resources/views/livewire/test-step.blade.php
+++ b/resources/views/livewire/test-step.blade.php
@@ -81,6 +81,50 @@
</div>
</div>
</x-filament::section>
+ @elseif($testGrade === null && !$testPassed)
+ {{-- Grading failed (e.g., missing rubric) - allow retry --}}
+ <x-filament::section class="mt-6">
+ <div class="space-y-4">
+ <div class="rounded-lg bg-warning-50 dark:bg-warning-900/20 p-4">
+ <div class="flex">
+ <div class="flex-shrink-0">
+ <x-filament::icon
+ icon="heroicon-o-exclamation-triangle"
+ class="h-5 w-5 text-warning-600 dark:text-warning-400"
+ />
+ </div>
+ <div class="ml-3">
+ <h3 class="text-sm font-medium text-warning-800 dark:text-warning-200">
+ Unable to Grade Test
+ </h3>
+ <div class="mt-2 text-sm text-warning-700 dark:text-warning-300">
+ <p>
+ There was an issue grading your test submission. Please try retaking the test.
+ </p>
+ </div>
+ <div class="mt-4 flex gap-3">
+ @if($step->retryStep)
+ <x-filament::button
+ tag="a"
+ :href="$step->retryStep->url"
+ color="warning"
+ outlined
+ >
+ Review Material
+ </x-filament::button>
+ @endif
+ <x-filament::button
+ wire:click="retakeTest"
+ color="primary"
+ >
+ Retake Test
+ </x-filament::button>
+ </div>
+ </div>
+ </div>
+ </div>
+ </div>
+ </x-filament::section>
@endif
@else
@livewire('create-test-entry', ['test' => $test])
diff --git a/src/Pages/CourseCompleted.php b/src/Pages/CourseCompleted.php
--- a/src/Pages/CourseCompleted.php
+++ b/src/Pages/CourseCompleted.php
@@ -110,6 +110,15 @@
$rubric = $test->rubric;
if (! $rubric instanceof \Tapp\FilamentFormBuilder\Models\FilamentFormUser) {
+ // Include tests without rubrics as 0% to match getTestStepPercentageForUser() behavior
+ $details[] = [
+ 'name' => $step->name,
+ 'total_questions' => 0,
+ 'correct_questions' => 0,
+ 'percentage' => 0.0,
+ 'url' => Step::getUrlForStep($step),
+ ];
+
continue;
} |
…st for required_test_percentage Co-authored-by: Cursor <cursoragent@cursor.com>
| if (! $this->course->completed_at) { | ||
| // Only redirect when the user has not completed all steps. Allow viewing this page when all steps | ||
| // are done even if required_test_percentage is not met (we show retake/certificate state below). | ||
| if (! $this->course->allStepsCompletedByUser(auth()->id())) { |
There was a problem hiding this comment.
Missing auth check causes TypeError for unauthenticated users
Low Severity
The old code used $this->course->completed_at which internally checks Auth::check() and returns null gracefully for unauthenticated users. The new code passes auth()->id() directly to allStepsCompletedByUser(int $userId), which has a strict int type hint. If an unauthenticated user somehow reaches this page (misconfigured middleware, testing scenarios), auth()->id() returns null, causing a TypeError instead of graceful handling.
|
Bugbot Autofix prepared a fix for 2 of the 2 bugs found in the latest run.
Or push these changes by commenting: Preview (2e17ee1468)diff --git a/src/Pages/CourseCompleted.php b/src/Pages/CourseCompleted.php
--- a/src/Pages/CourseCompleted.php
+++ b/src/Pages/CourseCompleted.php
@@ -38,6 +38,11 @@
{
$this->course = Course::where('slug', $courseSlug)->firstOrFail();
+ // Redirect unauthenticated users to dashboard
+ if (! auth()->check()) {
+ return redirect()->to(Dashboard::getUrl());
+ }
+
// If course has no steps, redirect to dashboard
if (! $this->course->steps()->exists()) {
return redirect()->to(Dashboard::getUrl());
diff --git a/src/Services/CourseProgressQueryService.php b/src/Services/CourseProgressQueryService.php
--- a/src/Services/CourseProgressQueryService.php
+++ b/src/Services/CourseProgressQueryService.php
@@ -12,18 +12,21 @@
* Build query for course progress reporting. Uses lms_course_user.completed_at as the source
* of truth for completion (not step-derived calculation).
*
+ * Starts from lms_step_user to capture all users with any progress (including public course
+ * users who haven't completed), and LEFT JOINs to lms_course_user for completion status.
+ *
* @return Builder|QueryBuilder
*/
public static function buildQuery()
{
- return DB::table('lms_course_user')
- ->join('users', 'users.id', '=', 'lms_course_user.user_id')
- ->join('lms_courses', 'lms_courses.id', '=', 'lms_course_user.course_id')
- ->leftJoin('lms_lessons', 'lms_lessons.course_id', '=', 'lms_courses.id')
- ->leftJoin('lms_steps', 'lms_steps.lesson_id', '=', 'lms_lessons.id')
- ->leftJoin('lms_step_user', function ($join): void {
- $join->on('lms_step_user.step_id', '=', 'lms_steps.id')
- ->on('lms_step_user.user_id', '=', 'lms_course_user.user_id');
+ return DB::table('lms_step_user')
+ ->join('users', 'users.id', '=', 'lms_step_user.user_id')
+ ->join('lms_steps', 'lms_steps.id', '=', 'lms_step_user.step_id')
+ ->join('lms_lessons', 'lms_lessons.id', '=', 'lms_steps.lesson_id')
+ ->join('lms_courses', 'lms_courses.id', '=', 'lms_lessons.course_id')
+ ->leftJoin('lms_course_user', function ($join): void {
+ $join->on('lms_course_user.course_id', '=', 'lms_courses.id')
+ ->on('lms_course_user.user_id', '=', 'lms_step_user.user_id');
})
->select([
'users.id as user_id',
@@ -33,13 +36,13 @@
'lms_courses.id as course_id',
'lms_courses.name as course_name',
DB::raw('MIN(lms_step_user.created_at) as started_at'),
- 'lms_course_user.completed_at as completed_at',
- 'lms_course_user.completed_at as completion_date',
+ DB::raw('MAX(lms_course_user.completed_at) as completed_at'),
+ DB::raw('MAX(lms_course_user.completed_at) as completion_date'),
DB::raw('COUNT(DISTINCT CASE WHEN lms_step_user.completed_at IS NOT NULL THEN lms_step_user.step_id END) as steps_completed'),
DB::raw('(SELECT COUNT(DISTINCT s.id) FROM lms_steps s JOIN lms_lessons l ON s.lesson_id = l.id WHERE l.course_id = lms_courses.id) as total_steps'),
- DB::raw("CASE WHEN lms_course_user.completed_at IS NOT NULL THEN 'Completed' ELSE 'In Progress' END as status"),
+ DB::raw("CASE WHEN MAX(lms_course_user.completed_at) IS NOT NULL THEN 'Completed' ELSE 'In Progress' END as status"),
])
- ->groupBy('lms_course_user.user_id', 'lms_course_user.course_id', 'lms_course_user.completed_at', 'users.id', 'users.first_name', 'users.last_name', 'users.email', 'lms_courses.id', 'lms_courses.name')
+ ->groupBy('users.id', 'users.first_name', 'users.last_name', 'users.email', 'lms_courses.id', 'lms_courses.name')
->orderBy('users.id', 'asc')
->orderBy('lms_courses.id', 'asc');
}
@@ -51,7 +54,7 @@
public static function sortByStatus($query, $direction)
{
return $query->reorder()
- ->orderByRaw("CASE WHEN lms_course_user.completed_at IS NOT NULL THEN 1 ELSE 0 END {$direction}")
+ ->orderByRaw("CASE WHEN MAX(lms_course_user.completed_at) IS NOT NULL THEN 1 ELSE 0 END {$direction}")
->orderBy('users.id', 'asc')
->orderBy('lms_courses.id', 'asc');
}
@@ -135,7 +138,7 @@
public static function sortByCompletedAt($query, $direction)
{
return $query->reorder()
- ->orderBy('lms_course_user.completed_at', $direction)
+ ->orderByRaw("MAX(lms_course_user.completed_at) {$direction}")
->orderBy('users.id', 'asc')
->orderBy('lms_courses.id', 'asc');
} |
- Build query from lms_step_user (step activity) instead of lms_course_user - LEFT JOIN lms_course_user for completed_at so users who started but have not completed a course appear in reports with status In Progress - Add first_name/last_name to test users table for progress query - Add CourseProgressQueryServiceTest for in-progress and completed users Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot Autofix prepared a fix for the bug found in the latest run.
Or push these changes by commenting: Preview (726034ea45)diff --git a/src/Models/Course.php b/src/Models/Course.php
--- a/src/Models/Course.php
+++ b/src/Models/Course.php
@@ -424,7 +424,7 @@
/**
* Get the percentage score for a single test step for a user (0–100), or 0 if no entry or on error.
*/
- protected function getTestStepPercentageForUser(Step $step, int $userId): float
+ public function getTestStepPercentageForUser(Step $step, int $userId): float
{
$step->load('material');
$test = $step->material;
diff --git a/src/Pages/CourseCompleted.php b/src/Pages/CourseCompleted.php
--- a/src/Pages/CourseCompleted.php
+++ b/src/Pages/CourseCompleted.php
@@ -5,7 +5,6 @@
namespace Tapp\FilamentLms\Pages;
use BackedEnum;
-use Exception;
use Filament\Pages\Page;
use Tapp\FilamentLms\Concerns\CourseLayout;
use Tapp\FilamentLms\Models\Course;
@@ -115,38 +114,8 @@
$totalQuestions = count($rubric->entry);
- $entry = \Tapp\FilamentFormBuilder\Models\FilamentFormUser::where('filament_form_id', $test->filament_form_id)
- ->where('user_id', $userId)
- ->when($test->filament_form_user_id, fn ($q) => $q->where('id', '!=', $test->filament_form_user_id))
- ->first();
-
- if (! $entry) {
- $details[] = [
- 'name' => $step->name,
- 'total_questions' => $totalQuestions,
- 'correct_questions' => 0,
- 'percentage' => 0.0,
- 'url' => Step::getUrlForStep($step),
- ];
-
- continue;
- }
-
- $grade = $test->gradeEntry($entry);
-
- if ($grade instanceof Exception) {
- $details[] = [
- 'name' => $step->name,
- 'total_questions' => $totalQuestions,
- 'correct_questions' => 0,
- 'percentage' => 0.0,
- 'url' => Step::getUrlForStep($step),
- ];
-
- continue;
- }
-
- $percentage = (float) $grade;
+ // Use the shared grading method instead of duplicating the entry-fetching and grading logic
+ $percentage = $this->course->getTestStepPercentageForUser($step, $userId);
$correctQuestions = (int) round(($percentage / 100) * $totalQuestions);
$details[] = [ |
…o 868h658e9-require-course-correctness-percentage
scottgrayson
deleted the
868h658e9-require-course-correctness-percentage
branch
| ->numeric() | ||
| ->minValue(0) | ||
| ->maxValue(100) | ||
| ->default(0) |
There was a problem hiding this comment.
Form default causes confusing "0%" requirement message
Medium Severity
The required_test_percentage form field has ->default(0) while the templates check !== null to display the requirement message. When a course is created with the default value of 0, users see "This course requires an overall test average of at least 0% to receive your certificate" - a misleading message since 0% is not a meaningful requirement. The database column is defined as nullable() without a default, suggesting null was intended to mean "no requirement" rather than 0.
Additional Locations (1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
completed_atcolumn tolms_course_userpivot tableallStepsCompletedByUserfor page access instead ofcompletedByUserAtCourse::completedByUserAt()to read from pivot instead of calculatingmaybeSetCompletedAtForUser()to set completion when requirements metrequired_test_percentageto 0Testing
All course-related tests pass, including CourseRedirectTest, CourseTest, and CourseOverallGradeTest.
Made with Cursor
Note
Medium Risk
Touches completion/certification state and reporting queries, so incorrect pivot updates or edge cases (retakes, courses with/without tests) could mislabel completion or block certificate download, though changes are localized and covered by new feature tests.
Overview
Course completion is now persisted on
lms_course_user.completed_at(new migration + pivot relationship updates) rather than being derived from step progress, withCourse::maybeSetCompletedAtForUser()setting the timestamp once a user has finished all steps and (optionally) met a course-levelrequired_test_percentageaverage across test steps.The certificate flow is updated to avoid redirect loops and to clearly handle “all steps done but not certificate-qualified”:
CourseCompletednow allows entry once all steps are complete, shows per-test/overall grade details and retake links when below the required average, and only enables certificate download when the pivotcompleted_atis set; retaking a test clears the pivot completion so it can be re-evaluated.Reporting and progress queries are simplified to rely on the pivot completion timestamp for status/date filtering/sorting, and the admin UI adds
required_test_percentageto course settings plus improved test-step messaging (including non-perfect-score guidance and optional review-step selection for all test steps).Written by Cursor Bugbot for commit 3cecf8c. This will update automatically on new commits. Configure here.