fix(review): address PR #136 review feedback

Address review comments from @dealako:

- auth0_mgmt.go: added a dryRun parameter to syncProfileToAuth0 so the
  no-metadata-diff no-op path is honored in dry-run mode, matching the
  real-write path's usersUpdated/usersSkipped classification (per @dealako)
- backfill_email_profile.go: removed the duplicated dry-run branches in
  backfillProfileForUser and syncSingleUser; both now delegate to
  syncProfileToAuth0Fn(..., dryRun) for a single source of truth (per @dealako)
- auth0_mgmt.go: renamed syncProfileToAuth0's existing param to primaryUser
  for consistency with emailLinkEligibility/linkEmailIdentity/unlinkEmailIdentity
  (per @dealako)
- handlers_users.go, auth0_mgmt_test.go, handlers_users_test.go: updated call
  sites and test signatures for the new dryRun parameter

Resolves 2 review threads.

Assisted-by: github-copilot:claude-sonnet-5
Signed-off-by: Eric Searcy <eric@linuxfoundation.org>

Author: emsearcy

cmd/lfx-v1-sync-helper/auth0_mgmt.go

@@ -178,21 +178,23 @@ func fetchAuth0User(ctx context.Context, auth0UserID string) (*management.User,
 // pushes the update via the Management API. The caller must pass the
 // pre-fetched Auth0 user so that multiple operations on the same user share a
 // single Management API read. The returned bool is true only when
-// user_metadata was actually written, so callers (in particular backfill
-// summaries) can distinguish an update from a no-op skip.
-func syncProfileToAuth0(ctx context.Context, auth0UserID string, existing *management.User, v1Data map[string]any) (bool, error) {
+// user_metadata was actually written (or, in dry-run mode, would have been
+// written), so callers (in particular backfill summaries) can distinguish an
+// update from a no-op skip. When dryRun is true, all eligibility and diff
+// checks still run but the Management API write is skipped.
+func syncProfileToAuth0(ctx context.Context, auth0UserID string, primaryUser *management.User, v1Data map[string]any, dryRun bool) (bool, error) {
 	// Blocked accounts are treated as inactive/deprovisioned: don't push new
 	// profile data to them. This is a no-op skip, not an error.
-	if existing.GetBlocked() {
+	if primaryUser.GetBlocked() {
 		logger.With("auth0_user_id", auth0UserID).
 			WarnContext(ctx, "Auth0 user is blocked, skipping profile sync")
 		return false, nil
 	}
 
 	// Start from existing user_metadata (or empty map) for diffing.
 	existingMetadata := make(map[string]interface{})
-	if existing.UserMetadata != nil {
-		for k, v := range *existing.UserMetadata {
+	if primaryUser.UserMetadata != nil {
+		for k, v := range *primaryUser.UserMetadata {
 			existingMetadata[k] = v
 		}
 	}
@@ -219,6 +221,12 @@ func syncProfileToAuth0(ctx context.Context, auth0UserID string, existing *manag
 		return false, nil
 	}
 
+	if dryRun {
+		logger.With("auth0_user_id", auth0UserID).
+			InfoContext(ctx, "[dry-run] would sync profile to Auth0 user_metadata")
+		return true, nil
+	}
+
 	// Push the updated user_metadata to Auth0.
 	if err := auth0Users.Update(ctx, auth0UserID, &management.User{
 		UserMetadata: &metadata,

cmd/lfx-v1-sync-helper/auth0_mgmt_test.go

@@ -26,7 +26,7 @@ func TestSyncProfileToAuth0Blocked(t *testing.T) {
 	cleanup := setupLinkTest(t, fake)
 	defer cleanup()
 
-	updated, err := syncProfileToAuth0(context.Background(), "auth0|blocked", fake.users["auth0|blocked"], map[string]any{"title": "Engineer"})
+	updated, err := syncProfileToAuth0(context.Background(), "auth0|blocked", fake.users["auth0|blocked"], map[string]any{"title": "Engineer"}, false)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

cmd/lfx-v1-sync-helper/backfill_email_profile.go

@@ -473,18 +473,7 @@ func backfillProfileForUser(ctx context.Context, auth0UserID, username string, d
 		return fmt.Errorf("fetching Auth0 user %s: %w", auth0UserID, err)
 	}
 
-	if dryRun {
-		if auth0User.GetBlocked() {
-			result.usersSkipped++
-			return nil
-		}
-		logger.With("auth0_user_id", auth0UserID, "user_sfid", userSfid).
-			Info("[dry-run] would sync profile to Auth0 user_metadata")
-		result.usersUpdated++
-		return nil
-	}
-
-	updated, err := syncProfileToAuth0Fn(ctx, auth0UserID, auth0User, v1Data)
+	updated, err := syncProfileToAuth0Fn(ctx, auth0UserID, auth0User, v1Data, dryRun)
 	if err != nil {
 		return fmt.Errorf("syncing profile for %s: %w", auth0UserID, err)
 	}
@@ -529,20 +518,18 @@ func syncSingleUser(ctx context.Context, username string, dryRun bool) error {
 	}
 	if !exists {
 		logger.With("user_sfid", userSfid).Warn("v1 merged_user record not found, skipping profile sync")
-	} else if dryRun {
-		if auth0User.GetBlocked() {
-			logger.With("auth0_user_id", auth0UserID).
-				Info("[dry-run] Auth0 user is blocked, would skip profile sync")
+	} else if updated, err := syncProfileToAuth0Fn(ctx, auth0UserID, auth0User, v1Data, dryRun); err != nil {
+		logger.With("error", err, "auth0_user_id", auth0UserID).
+			Warn("profile sync failed")
+	} else if updated {
+		if dryRun {
+			logger.With("auth0_user_id", auth0UserID).Info("[dry-run] would sync profile")
 		} else {
-			logger.With("auth0_user_id", auth0UserID, "user_sfid", userSfid).
-				Info("[dry-run] would sync profile to Auth0 user_metadata")
+			logger.With("auth0_user_id", auth0UserID).Info("profile synced")
 		}
 	} else {
-		if updated, err := syncProfileToAuth0Fn(ctx, auth0UserID, auth0User, v1Data); err != nil {
-			logger.With("error", err, "auth0_user_id", auth0UserID).
-				Warn("profile sync failed")
-		} else if updated {
-			logger.With("auth0_user_id", auth0UserID).Info("profile synced")
+		if dryRun {
+			logger.With("auth0_user_id", auth0UserID).Info("[dry-run] profile sync would be skipped (no-op)")
 		} else {
 			logger.With("auth0_user_id", auth0UserID).Info("profile sync skipped (no-op)")
 		}

cmd/lfx-v1-sync-helper/handlers_users.go

@@ -285,7 +285,7 @@ func syncMergedUserProfile(ctx context.Context, key, auth0UserID string, v1Data
 		return false
 	}
 
-	if _, err := syncProfileToAuth0Fn(syncCtx, auth0UserID, auth0User, v1Data); err != nil {
+	if _, err := syncProfileToAuth0Fn(syncCtx, auth0UserID, auth0User, v1Data, false); err != nil {
 		if isRetryableAuth0Error(err) {
 			logger.With(errKey, err, "key", key, "auth0_user_id", auth0UserID).
 				WarnContext(ctx, "retryable Auth0 error during profile sync, NACKing for redelivery")

cmd/lfx-v1-sync-helper/handlers_users_test.go

@@ -150,7 +150,7 @@ func TestSyncMergedUserProfile(t *testing.T) {
 			cfg = &Config{}
 
 			var called bool
-			syncProfileToAuth0Fn = func(_ context.Context, _ string, _ *management.User, _ map[string]any) (bool, error) {
+			syncProfileToAuth0Fn = func(_ context.Context, _ string, _ *management.User, _ map[string]any, _ bool) (bool, error) {
 				called = true
 				return tt.syncErr == nil, tt.syncErr
 			}