Data spillage report api use available data (#36699)

* Read available value when viable

* Added tests

* Restored package lock

* CI

* Used saved actor details when available

* i18n fix

* Updated data in log

server/channels/app/content_flagging_report.go

@@ -34,10 +34,6 @@ const (
 // temporary file and returns the file path. The caller is responsible for
 // removing the file when the response has been served.
 func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUserID, comment, action string) (string, *model.AppError) {
-	if appErr := a.ensureActorCommentForReport(rctx, postID, comment); appErr != nil {
-		return "", appErr
-	}
-
 	tmp, err := os.CreateTemp("", flaggedPostReportTempPattern)
 	if err != nil {
 		return "", model.NewAppError("GenerateFlaggedPostReport", "app.data_spillage.report.tempfile.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
@@ -51,7 +47,7 @@ func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUse
 
 	zw := zip.NewWriter(tmp)
 
-	if appErr := a.writeFlaggedPostReport(rctx, zw, postID, generatedByUserID, action); appErr != nil {
+	if appErr := a.writeFlaggedPostReport(rctx, zw, postID, generatedByUserID, comment, action); appErr != nil {
 		_ = zw.Close()
 		cleanup()
 		return "", appErr
@@ -73,7 +69,7 @@ func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUse
 	return tmpPath, nil
 }
 
-func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, generatedByUserID, action string) *model.AppError {
+func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, generatedByUserID, comment, action string) *model.AppError {
 	rc, appErr := a.loadFlaggedPostReportContext(rctx, postID)
 	if appErr != nil {
 		return appErr
@@ -89,7 +85,7 @@ func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, g
 	if appErr := a.writeEditHistorySection(rctx, zw, rc, seenFiles); appErr != nil {
 		return appErr
 	}
-	if appErr := a.writeContentReviewEntry(rctx, zw, rc.Post, generatedByUserID, action); appErr != nil {
+	if appErr := a.writeContentReviewEntry(rctx, zw, rc.Post, generatedByUserID, comment, action); appErr != nil {
 		return appErr
 	}
 	if appErr := a.writeReportMetadataEntry(zw, generatedByUserID); appErr != nil {
@@ -183,8 +179,8 @@ func (a *App) writeEditHistorySection(rctx request.CTX, zw *zip.Writer, rc *mode
 	return nil
 }
 
-func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *model.Post, generatedByUserID, action string) *model.AppError {
-	payload, appErr := a.buildContentReviewYAML(rctx, post, generatedByUserID, action)
+func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *model.Post, generatedByUserID, comment, action string) *model.AppError {
+	payload, appErr := a.buildContentReviewYAML(rctx, post, generatedByUserID, comment, action)
 	if appErr != nil {
 		return appErr
 	}
@@ -194,54 +190,6 @@ func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *mo
 	return nil
 }
 
-// ensureActorCommentForReport persists the report-generator's comment as the
-// actor_comment property when the post does not yet have one. If a value is
-// already present (set by a prior keep/remove or report-generation), it is
-// preserved so the existing reviewer note is never overwritten.
-func (a *App) ensureActorCommentForReport(rctx request.CTX, postID, comment string) *model.AppError {
-	if comment == "" {
-		return nil
-	}
-
-	existing, appErr := a.GetPostContentFlaggingPropertyValue(postID, contentFlaggingPropertyNameActorComment)
-	if appErr != nil && appErr.StatusCode != http.StatusNotFound {
-		return appErr
-	}
-
-	if existing != nil {
-		return nil
-	}
-
-	groupID, gErr := a.ContentFlaggingGroupId()
-	if gErr != nil {
-		return gErr
-	}
-	mappedFields, appErr := a.GetContentFlaggingMappedFields(groupID)
-	if appErr != nil {
-		return appErr
-	}
-
-	commentBytes, jsonErr := json.Marshal(comment)
-	if jsonErr != nil {
-		return model.NewAppError("ensureActorCommentForReport", "app.data_spillage.report.marshal_comment.app_error", nil, "", http.StatusInternalServerError).Wrap(jsonErr)
-	}
-
-	propertyValues := []*model.PropertyValue{
-		{
-			TargetID:   postID,
-			TargetType: model.PropertyValueTargetTypePost,
-			GroupID:    groupID,
-			FieldID:    mappedFields[contentFlaggingPropertyNameActorComment].ID,
-			Value:      json.RawMessage(commentBytes),
-		},
-	}
-
-	if _, appErr := a.CreatePropertyValues(rctx, propertyValues); appErr != nil {
-		return model.NewAppError("ensureActorCommentForReport", "app.data_spillage.create_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
-	}
-	return nil
-}
-
 func (a *App) writeReportMetadataEntry(zw *zip.Writer, generatedByUserID string) *model.AppError {
 	generator, appErr := a.GetUser(generatedByUserID)
 	if appErr != nil {
@@ -282,7 +230,7 @@ func buildPostYAML(post *model.Post, channel *model.Channel, team *model.Team, a
 	return out
 }
 
-func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post, generatedByUserID, pendingAction string) (model.FlaggedPostReportContentReview, *model.AppError) {
+func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post, generatedByUserID, actorComment, pendingAction string) (model.FlaggedPostReportContentReview, *model.AppError) {
 	out := model.FlaggedPostReportContentReview{}
 
 	values, appErr := a.GetPostContentFlaggingPropertyValues(post.Id)
@@ -337,16 +285,29 @@ func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post, generat
 
 	reviewerID := decodePropertyString(rctx, byName, contentFlaggingPropertyNameReviewerUserID)
 	out.ReviewerUserID = reviewerID
+
+	// Use saved comment if available, else use incoming comment
 	out.ReviewerComment = decodePropertyString(rctx, byName, contentFlaggingPropertyNameActorComment)
+	if out.ReviewerComment == "" && actorComment != "" {
+		out.ReviewerComment = actorComment
+	}
+
 	out.ActionTime = decodePropertyInt64(rctx, byName, contentFlaggingPropertyNameActionTime)
 
-	// We want to include the actor details only when an action is being performed - retain or delete the quarantined post.
-	if pendingAction != "" {
-		if u, uErr := a.GetUser(generatedByUserID); uErr == nil {
+	// Use saved actor if available, else use calling user. The check for pending action is used
+	// as the client passes a pending action when generating report just before performing an action.
+	// All other flows do not pass an action.
+	actorUserId := decodePropertyString(rctx, byName, contentFlaggingPropertyNameActorUserID)
+	if actorUserId == "" && pendingAction != "" {
+		actorUserId = generatedByUserID
+	}
+
+	if actorUserId != "" {
+		if u, uErr := a.GetUser(actorUserId); uErr == nil {
 			out.ActorUsername = u.Username
 			out.ActorUserId = u.Id
 		} else {
-			rctx.Logger().Warn("Failed to fetch report generator user for flagged post report", mlog.String("user_id", generatedByUserID), mlog.Err(uErr))
+			rctx.Logger().Warn("Failed to fetch report generator user for flagged post report", mlog.String("actor_user_id", actorUserId), mlog.Err(uErr))
 		}
 	}
 

server/channels/app/content_flagging_report_test.go

@@ -148,7 +148,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) {
 		require.Empty(t, review.ActorUsername)
 	})
 
-	t.Run("content_review.yaml records remove decision after permanent delete", func(t *testing.T) {
+	t.Run("content_review.yaml records remove decision and committed actor after permanent delete", func(t *testing.T) {
 		appErr := setBaseConfig(th)
 		require.Nil(t, appErr)
 
@@ -157,6 +157,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) {
 		appErr = th.App.PermanentDeleteFlaggedPost(th.Context, &model.FlagContentActionRequest{Comment: "violates policy"}, th.SystemAdminUser.Id, post)
 		require.Nil(t, appErr)
 
+		// Report is generated by BasicUser, but the committed actor on the post is SystemAdminUser.
 		path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "")
 		require.Nil(t, appErr)
 
@@ -164,11 +165,11 @@ func TestGenerateFlaggedPostReport(t *testing.T) {
 		var review model.FlaggedPostReportContentReview
 		require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review))
 		require.Equal(t, "remove", review.ActorDecision)
-		require.Empty(t, review.ActorUserId)
-		require.Empty(t, review.ActorUsername)
+		require.Equal(t, th.SystemAdminUser.Id, review.ActorUserId)
+		require.Equal(t, th.SystemAdminUser.Username, review.ActorUsername)
 	})
 
-	t.Run("content_review.yaml records keep decision after keep action", func(t *testing.T) {
+	t.Run("content_review.yaml records keep decision and committed actor after keep action", func(t *testing.T) {
 		appErr := setBaseConfig(th)
 		require.Nil(t, appErr)
 
@@ -184,8 +185,30 @@ func TestGenerateFlaggedPostReport(t *testing.T) {
 		var review model.FlaggedPostReportContentReview
 		require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review))
 		require.Equal(t, "keep", review.ActorDecision)
-		require.Empty(t, review.ActorUserId)
-		require.Empty(t, review.ActorUsername)
+		require.Equal(t, th.SystemAdminUser.Id, review.ActorUserId)
+		require.Equal(t, th.SystemAdminUser.Username, review.ActorUsername)
+	})
+
+	t.Run("content_review.yaml prefers committed actor over pending action's generator", func(t *testing.T) {
+		appErr := setBaseConfig(th)
+		require.Nil(t, appErr)
+
+		post := setupFlaggedPost(t, th)
+
+		// Commit a keep action so the actor user id property is set to SystemAdminUser.
+		appErr = th.App.KeepFlaggedPost(th.Context, &model.FlagContentActionRequest{Comment: "looks fine"}, th.SystemAdminUser.Id, post)
+		require.Nil(t, appErr)
+
+		// Even with a pending action supplied (and a different generator), the
+		// committed actor on the post should win over the pending-action fallback.
+		path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", model.ContentFlaggingActionRemove)
+		require.Nil(t, appErr)
+
+		entries := readReportZip(t, path)
+		var review model.FlaggedPostReportContentReview
+		require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review))
+		require.Equal(t, th.SystemAdminUser.Id, review.ActorUserId)
+		require.Equal(t, th.SystemAdminUser.Username, review.ActorUsername)
 	})
 
 	t.Run("content_review.yaml uses pending action when status is not yet committed", func(t *testing.T) {
@@ -311,6 +334,41 @@ func TestGenerateFlaggedPostReport(t *testing.T) {
 	})
 }
 
+// TestBuildContentReviewYAMLActorCommentFallback exercises the replica-lag
+// fallback in buildContentReviewYAML: when the property-value read (which is
+// served from a read replica) returns no actor_comment value, the function
+// must fall back to the in-hand actorComment supplied by the caller so the
+// generated report still reflects the comment the user just submitted.
+func TestBuildContentReviewYAMLActorCommentFallback(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	t.Run("falls back to in-hand comment when no actor_comment is persisted", func(t *testing.T) {
+		appErr := setBaseConfig(th)
+		require.Nil(t, appErr)
+
+		post := setupFlaggedPost(t, th)
+
+		// setupFlaggedPost only writes the reporting_* properties — no
+		// actor_comment row exists yet, so the replica read returns "" for
+		// that field and the fallback branch must take effect.
+		review, appErr := th.App.buildContentReviewYAML(th.Context, post, th.BasicUser.Id, "fallback note", "")
+		require.Nil(t, appErr)
+		require.Equal(t, "fallback note", review.ReviewerComment)
+	})
+
+	t.Run("reviewer comment stays empty when both sources are empty", func(t *testing.T) {
+		appErr := setBaseConfig(th)
+		require.Nil(t, appErr)
+
+		post := setupFlaggedPost(t, th)
+
+		review, appErr := th.App.buildContentReviewYAML(th.Context, post, th.BasicUser.Id, "", "")
+		require.Nil(t, appErr)
+		require.Empty(t, review.ReviewerComment)
+	})
+}
+
 func TestBuildPostYAML(t *testing.T) {
 	channel := &model.Channel{Id: "channel-id", DisplayName: "Channel Name"}
 	team := &model.Team{Id: "team-id", DisplayName: "Team Name"}

server/i18n/en.json

@@ -6208,10 +6208,6 @@
     "id": "app.data_spillage.report.incomplete_warning",
     "translation": "Post deletion incomplete. Review the Error Log and escalate to a System Administrator for manual remediation."
   },
-  {
-    "id": "app.data_spillage.report.marshal_comment.app_error",
-    "translation": "An error occurred while marshaling the comment for the flagged post report."
-  },
   {
     "id": "app.data_spillage.report.post_id",
     "translation": "Post ID:"