fix: improve error handling for unmarking assessments and update test assertions

Author: rappm

servers/assessment/assessments/actionItem/service_test.go

@@ -81,22 +81,34 @@ func (suite *ActionItemServiceTestSuite) TestUpdateActionItem() {
 	err := CreateActionItem(suite.suiteCtx, createRequest)
 	assert.NoError(suite.T(), err)
 
-	// For update, we need to use an existing ID
-	// Since we can't easily get the ID from create, we'll use a known ID
+	// Get the created action item by listing items for the student in this phase
+	actionItems, err := ListActionItemsForStudentInPhase(suite.suiteCtx, suite.testCourseParticipationID, suite.testCoursePhaseID)
+	assert.NoError(suite.T(), err, "Should be able to list action items to find created item")
+	assert.Greater(suite.T(), len(actionItems), 0, "Should have at least one action item")
+
+	// Find the action item we just created
+	var createdActionItemID uuid.UUID
+	for _, item := range actionItems {
+		if item.Action == "Original action" && item.Author == "[email protected]" {
+			createdActionItemID = item.ID
+			break
+		}
+	}
+	assert.NotEqual(suite.T(), uuid.Nil, createdActionItemID, "Should have found the created action item")
+
+	// Now update the action item using the actual ID
 	updateRequest := actionItemDTO.UpdateActionItemRequest{
-		ID:                    suite.testActionItemID,
+		ID:                    createdActionItemID,
 		CoursePhaseID:         suite.testCoursePhaseID,
 		CourseParticipationID: suite.testCourseParticipationID,
 		Action:                "Updated action",
 		Author:                "[email protected]",
 	}
 
-	err = UpdateActionItem(suite.suiteCtx, updateRequest)
-	assert.NoError(suite.T(), err, "Should be able to update action item")
-	// This might fail if the ID doesn't exist, which is expected
-	// The test verifies the function doesn't panic
+	// Test the update operation with proper error handling
 	assert.NotPanics(suite.T(), func() {
-		UpdateActionItem(suite.suiteCtx, updateRequest)
+		err := UpdateActionItem(suite.suiteCtx, updateRequest)
+		assert.NoError(suite.T(), err, "Should be able to update existing action item")
 	}, "Should not panic when updating action item")
 }
 
@@ -107,7 +119,7 @@ func (suite *ActionItemServiceTestSuite) TestDeleteActionItem() {
 	// This might fail if the ID doesn't exist, which is expected
 	// The test verifies the function doesn't panic
 	assert.NotPanics(suite.T(), func() {
-		DeleteActionItem(suite.suiteCtx, testID)
+		_ = DeleteActionItem(suite.suiteCtx, testID)
 	}, "Should not panic when deleting action item")
 }
 
@@ -119,7 +131,7 @@ func (suite *ActionItemServiceTestSuite) TestDeleteActionItemNonExistent() {
 	// This is because DELETE operations are idempotent in SQL
 	// We just verify it doesn't panic
 	assert.NotPanics(suite.T(), func() {
-		DeleteActionItem(suite.suiteCtx, nonExistentID)
+		_ = DeleteActionItem(suite.suiteCtx, nonExistentID)
 	}, "Should not panic when deleting non-existent action item")
 }
 

servers/assessment/assessments/assessmentCompletion/router.go

@@ -1,6 +1,7 @@
 package assessmentCompletion
 
 import (
+	"errors"
 	"net/http"
 
 	"github.com/gin-gonic/gin"
@@ -95,7 +96,7 @@ func unmarkAssessmentAsCompleted(c *gin.Context) {
 	}
 	if err := UnmarkAssessmentAsCompleted(c, courseParticipationID, coursePhaseID); err != nil {
 		// Check if the error is due to deadline being passed
-		if err.Error() == "cannot unmark assessment as completed: deadline has passed" {
+		if errors.Is(err, ErrDeadlinePassed) {
 			handleError(c, http.StatusForbidden, err)
 		} else {
 			handleError(c, http.StatusInternalServerError, err)

servers/assessment/assessments/assessmentCompletion/service.go

@@ -17,6 +17,9 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+// ErrDeadlinePassed represents an error when trying to perform an action after the deadline has passed
+var ErrDeadlinePassed = errors.New("cannot unmark assessment as completed: deadline has passed")
+
 type AssessmentCompletionService struct {
 	queries db.Queries
 	conn    *pgxpool.Pool
@@ -138,7 +141,7 @@ func UnmarkAssessmentAsCompleted(ctx context.Context, courseParticipationID, cou
 
 	// If deadline exists and has passed, prevent unmarking
 	if deadline != nil && time.Now().After(*deadline) {
-		return errors.New("cannot unmark assessment as completed: deadline has passed")
+		return ErrDeadlinePassed
 	}
 
 	err = AssessmentCompletionServiceSingleton.queries.UnmarkAssessmentAsFinished(ctx, db.UnmarkAssessmentAsFinishedParams{

servers/assessment/coursePhaseConfig/service_test.go

@@ -84,7 +84,7 @@ func (suite *CoursePhaseConfigServiceTestSuite) TestUpdateCoursePhaseDeadlineInv
 	// We expect either an error or successful execution
 	// The exact assertion would depend on your database schema
 	assert.NotPanics(suite.T(), func() {
-		UpdateCoursePhaseDeadline(suite.suiteCtx, emptyID, testDeadline)
+		_ = UpdateCoursePhaseDeadline(suite.suiteCtx, emptyID, testDeadline)
 	}, "Should not panic with empty UUID")
 }