fix: assessment updates, flakey review test (#2387)

Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>

Author: golanglemonade

internal/ent/hooks/assessment.go

@@ -21,13 +21,15 @@ func HookQuestionnaireAssessment() ent.Hook {
 
 			id, ok := m.TemplateID()
 			if !ok {
-				// user provided jsonconfig and uischema directly then
-				// and not trying to be created/cloned from a template
-				//
-				// but at least the jsonconfig needs to be provided
-				_, ok := m.Jsonconfig()
-				if !ok {
-					return nil, fmt.Errorf("jsonconfig is required if you do not create an assessment from a template") //nolint:err113
+				if m.Op().Is(ent.OpCreate) {
+					// user provided jsonconfig and uischema directly then
+					// and not trying to be created/cloned from a template
+					//
+					// but at least the jsonconfig needs to be provided
+					_, ok := m.Jsonconfig()
+					if !ok {
+						return nil, fmt.Errorf("jsonconfig is required if you do not create an assessment from a template") //nolint:err113
+					}
 				}
 				return next.Mutate(ctx, m)
 			}

internal/ent/hooks/assessment_response.go

@@ -93,21 +93,20 @@ func findExistingAssessmentResponse(ctx context.Context, client *generated.Clien
 // assessment response. For drafts it updates due date only. For non-drafts it increments send
 // attempts, resets overdue status, and sends a new email invitation.
 func handleExistingAssessmentResponse(ctx context.Context, m *generated.AssessmentResponseMutation, existingResponse *generated.AssessmentResponse, isTest bool) (*generated.AssessmentResponse, error) {
-	isDraft, _ := m.IsDraft()
-
-	if existingResponse.Status != enums.AssessmentResponseStatusDraft &&
-		existingResponse.Status != enums.AssessmentResponseStatusSent &&
-		existingResponse.Status != enums.AssessmentResponseStatusOverdue &&
+	if existingResponse.Status == enums.AssessmentResponseStatusCompleted &&
 		!isTest {
-		return nil, ErrAssessmentInProgress
+		return nil, ErrAssessmentInCompleted
 	}
 
 	update := m.Client().AssessmentResponse.UpdateOneID(existingResponse.ID)
 
 	if dueDate, ok := m.DueDate(); ok {
 		update = update.SetDueDate(dueDate)
+		// ensure if due date changes status is not marked as overdue
+		update = update.SetStatus(enums.AssessmentResponseStatusSent)
 	}
 
+	isDraft, _ := m.IsDraft()
 	if isDraft {
 		update = update.SetStatus(enums.AssessmentResponseStatusDraft)
 
@@ -220,12 +219,8 @@ func HookUpdateAssessmentResponse() ent.Hook {
 
 			newStatus, statusExists := m.Status()
 
-			if statusExists {
-				switch assessmentResp.Status {
-				case enums.AssessmentResponseStatusCompleted,
-					enums.AssessmentResponseStatusOverdue:
-					return nil, ErrAssessmentInProgress
-				}
+			if statusExists && assessmentResp.Status == enums.AssessmentResponseStatusCompleted {
+				return nil, ErrAssessmentInCompleted
 			}
 
 			isPastDue := !assessmentResp.DueDate.IsZero() && time.Now().After(assessmentResp.DueDate)

internal/ent/hooks/errors.go

@@ -36,8 +36,8 @@ var (
 	ErrMaxAttemptsAssessments = errors.New("too many attempts to resend assessment invitation")
 	// ErrMaxSubscriptionAttempts is returned when a user has reached the max attempts to subscribe to an org
 	ErrMaxSubscriptionAttempts = errors.New("too many attempts to resend org subscription email")
-	// ErrAssessmentInProgress is returned when attempting to resend an email for an assessment that is already in progress
-	ErrAssessmentInProgress = errors.New("assessment is already in progress or completed")
+	// ErrAssessmentInCompleted is returned when attempting to resend an email for an assessment that is already completed
+	ErrAssessmentInCompleted = errors.New("assessment is already completed")
 	// ErrMissingRecipientEmail is returned when an email is required but not provided
 	ErrMissingRecipientEmail = errors.New("recipient email is required but not provided")
 	// ErrMissingRequiredName is returned when a name is required but not provided

internal/ent/hooks/review.go

@@ -18,7 +18,7 @@ import (
 )
 
 func getNextReviewDate(frequency enums.Frequency, lastReviewedAt models.DateTime) models.DateTime {
-	lastReviewDate := time.Time(lastReviewedAt)
+	lastReviewDate := time.Time(lastReviewedAt).UTC()
 
 	switch frequency {
 	case enums.FrequencyYearly:

internal/graphapi/assessment_test.go

@@ -302,6 +302,15 @@ func TestMutationUpdateAssessment(t *testing.T) {
 			client: suite.client.api,
 			ctx:    testUser1.UserCtx,
 		},
+		{
+			name: "happy path, update due date",
+			id:   assessment.ID,
+			request: testclient.UpdateAssessmentInput{
+				ResponseDueDuration: lo.ToPtr(int64(86400)), // 1 day,
+			},
+			client: suite.client.api,
+			ctx:    adminUser.UserCtx,
+		},
 		{
 			name: "happy path, update tags",
 			id:   assessment.ID,
@@ -386,6 +395,10 @@ func TestMutationUpdateAssessment(t *testing.T) {
 			if len(tc.request.AppendTags) > 0 {
 				assert.Check(t, len(resp.UpdateAssessment.Assessment.Tags) >= len(tc.request.AppendTags))
 			}
+
+			if tc.request.ResponseDueDuration != nil {
+				assert.Check(t, is.Equal(*resp.UpdateAssessment.Assessment.ResponseDueDuration, *tc.request.ResponseDueDuration))
+			}
 		})
 	}
 

internal/graphapi/assessmentresponse_test.go

@@ -422,7 +422,7 @@ func TestMutationCreateAssessmentResponse(t *testing.T) {
 		assert.NilError(t, err)
 
 		_, err = suite.client.api.CreateAssessmentResponse(testUser1.UserCtx, req)
-		assert.ErrorContains(t, err, "assessment is already in progress or completed")
+		assert.ErrorContains(t, err, "assessment is already completed")
 	})
 
 	(&Cleanup[*generated.AssessmentResponseDeleteOne]{client: suite.client.db.AssessmentResponse, IDs: responseIDsOrg1}).MustDelete(testUser1.UserCtx, t)

internal/graphapi/review_test.go

@@ -485,7 +485,7 @@ func TestReviewWithReviewFrequencyCalculation(t *testing.T) {
 				expectedReviewDate = lastReviewTime.AddDate(0, freq.addMonths, 0)
 			}
 
-			nextReviewTime := time.Time(*updatedEntity.NextReviewAt)
+			nextReviewTime := time.Time(*updatedEntity.NextReviewAt).UTC()
 
 			assert.Check(t, is.DeepEqual(expectedReviewDate.Year(), nextReviewTime.Year()),
 				"next_review_at year should match expected")
@@ -526,8 +526,8 @@ func TestReviewWithMultipleConnectedEntities(t *testing.T) {
 	assert.Check(t, newEntity.LastReviewedAt != nil, "last_reviewed_at should be set")
 	assert.Check(t, newEntity.NextReviewAt != nil, "next_review_at should be set")
 
-	lastReviewedTime := time.Time(*newEntity.LastReviewedAt)
-	nextReviewTime := time.Time(*newEntity.NextReviewAt)
+	lastReviewedTime := time.Time(*newEntity.LastReviewedAt).UTC()
+	nextReviewTime := time.Time(*newEntity.NextReviewAt).UTC()
 
 	expectedReviewDate := lastReviewedTime.AddDate(0, 1, 0)
 

internal/httpserve/handlers/questionnaire.go

@@ -2,6 +2,7 @@ package handlers
 
 import (
 	"context"
+	"errors"
 	"time"
 
 	echo "github.com/theopenlane/echox"
@@ -13,6 +14,7 @@ import (
 	"github.com/theopenlane/core/internal/ent/generated/assessment"
 	"github.com/theopenlane/core/internal/ent/generated/assessmentresponse"
 	"github.com/theopenlane/core/internal/ent/generated/privacy"
+	"github.com/theopenlane/core/internal/ent/hooks"
 	"github.com/theopenlane/core/pkg/logx"
 )
 
@@ -257,6 +259,10 @@ func (h *Handler) SubmitQuestionnaire(ctx echo.Context, openapi *OpenAPIContext)
 
 	freshResponse, err := responseUpdate.Save(allowCtx)
 	if err != nil {
+		if errors.Is(err, hooks.ErrAssessmentInCompleted) {
+			return h.BadRequest(ctx, err, openapi)
+		}
+
 		logx.FromContext(reqCtx).Err(err).Msg("could not update assessment response")
 		return h.InternalServerError(ctx, ErrProcessingRequest, openapi)
 	}