user-d: patch holes in expiry mechanism

I hope so, at least. Saw a few areas where I wasn't sure something
couldn't break so I did some maybe redundant checks. For #419.

user-d.go

@@ -18,6 +18,11 @@ func newUserDaemon(interval time.Duration, app *appContext) *GenericDaemon {
 	return d
 }
 
+const (
+	ExpiryModeDisable = iota
+	ExpiryModeDelete
+)
+
 func (app *appContext) checkUsers() {
 	if len(app.storage.GetUserExpiries()) == 0 {
 		return
@@ -29,25 +34,27 @@ func (app *appContext) checkUsers() {
 		app.err.Printf(lm.FailedGetUsers, lm.Jellyfin, err)
 		return
 	}
-	mode := "disable"
+	expiryMode := ExpiryModeDisable
 	if app.config.Section("user_expiry").Key("behaviour").MustString("disable_user") == "delete_user" {
-		mode = "delete"
+		expiryMode = ExpiryModeDelete
 	}
 
 	deleteAfterPeriod := app.config.Section("user_expiry").Key("delete_expired_after_days").MustInt(0)
-	if mode == "delete" {
+	if expiryMode == ExpiryModeDelete {
 		deleteAfterPeriod = 0
 	}
 
-	contact := false
-	if messagesEnabled && app.config.Section("user_expiry").Key("send_email").MustBool(true) {
-		contact = true
-	}
+	shouldContact := messagesEnabled && app.config.Section("user_expiry").Key("send_email").MustBool(true)
+
 	// Use a map to speed up checking for deleted users later
+	// FIXME: Maybe expose MediaBrowser.usersByID in some way and use that instead.
 	userExists := map[string]mediabrowser.User{}
 	for _, user := range users {
 		userExists[user.ID] = user
 	}
+
+	shouldInvalidateCache := false
+
 	for _, expiry := range app.storage.GetUserExpiries() {
 		id := expiry.JellyfinID
 		user, ok := userExists[id]
@@ -59,18 +66,34 @@ func (app *appContext) checkUsers() {
 		if !time.Now().After(expiry.Expiry) {
 			continue
 		}
-		deleteUserLater := deleteAfterPeriod != 0 && expiry.DeleteAfterPeriod
+
+		// True when "Delete after period" enabled and this user's account has already expired.
+		alreadyExpired := false
+		// True when the user has expired and N days has passed for them to be deleted.
+		alreadyExpiredShouldDelete := false
+		if expiry.DeleteAfterPeriod {
+			// Delete hanging expiries after the admin disables "delete after N days"
+			if deleteAfterPeriod <= 0 {
+				app.storage.DeleteUserExpiryKey(user.ID)
+				continue
+			}
+			alreadyExpired = true
+			alreadyExpiredShouldDelete = time.Now().After(expiry.Expiry.AddDate(0, 0, deleteAfterPeriod))
+			if !alreadyExpiredShouldDelete {
+				continue
+			}
+		}
 
 		// Record activity
 		activity := Activity{
 			UserID:     id,
 			SourceType: ActivityDaemon,
 			Time:       time.Now(),
+			Type:       ActivityUnknown,
 		}
 
-		deleteUserNow := deleteUserLater && time.Now().After(expiry.Expiry.AddDate(0, 0, deleteAfterPeriod))
-
-		if mode == "delete" || deleteUserNow {
+		// To save you the brain power: these conditions are fine because of all the "continue"s above us, no further checks are needed.
+		if expiryMode == ExpiryModeDelete || alreadyExpiredShouldDelete {
 			app.info.Printf(lm.DeleteExpiredUser, user.Name)
 			deleted := false
 			err, deleted = app.DeleteUser(user)
@@ -81,7 +104,7 @@ func (app *appContext) checkUsers() {
 			activity.Type = ActivityDeletion
 			// Store the user name, since there's no longer a user ID to reference back to
 			activity.Value = user.Name
-		} else if mode == "disable" && !deleteUserLater {
+		} else {
 			app.info.Printf(lm.DisableExpiredUser, user.Name)
 			// Admins can't be disabled
 			// so they're not an admin anymore, sorry
@@ -94,21 +117,26 @@ func (app *appContext) checkUsers() {
 			continue
 		}
 
-		app.storage.SetActivityKey(shortuuid.New(), activity, nil, false)
+		// Sanity check
+		if activity.Type != ActivityUnknown {
+			app.storage.SetActivityKey(shortuuid.New(), activity, nil, false)
+		}
 
-		// If the we're not gonna be deleting the user later, we don't need this.
-		// also, if the admin re-enabled the user, we should get rid of the countdown.
-		if deleteAfterPeriod <= 0 || mode == "delete" || deleteUserNow || (deleteUserLater && !user.Policy.IsDisabled) {
+		// If we're not gonna be deleting the user later, we don't need the expiry stored anymore:
+		// 1. Delete after N days is disabled, or Expiry mode is set to delete.
+		// 2. User has expired and been deleted after N days.
+		// 3. User has expired, but their account is not disabled (i.e. an Admin intervened).
+		if deleteAfterPeriod <= 0 || alreadyExpiredShouldDelete || (alreadyExpired && !user.Policy.IsDisabled) {
 			app.storage.DeleteUserExpiryKey(user.ID)
-		} else if deleteAfterPeriod > 0 && !deleteUserLater {
+		} else if deleteAfterPeriod > 0 && !alreadyExpired {
+			// Otherwise, mark the expiry as done pending a delete after N days.
 			expiry.DeleteAfterPeriod = true
 			app.storage.SetUserExpiryKey(user.ID, expiry)
 		}
-		app.InvalidateJellyfinCache()
-		if contact {
-			if !ok {
-				continue
-			}
+
+		shouldInvalidateCache = true
+
+		if shouldContact {
 			name := app.getAddressOrName(user.ID)
 			msg, err := app.email.constructUserExpired(app, false)
 			if err != nil {
@@ -116,8 +144,12 @@ func (app *appContext) checkUsers() {
 			} else if err := app.sendByID(msg, user.ID); err != nil {
 				app.err.Printf(lm.FailedSendExpiryMessage, user.ID, name, err)
 			} else {
-				app.err.Printf(lm.SentExpiryMessage, user.ID, name)
+				app.info.Printf(lm.SentExpiryMessage, user.ID, name)
 			}
 		}
 	}
+
+	if shouldInvalidateCache {
+		app.InvalidateJellyfinCache()
+	}
 }