fix: unbans not triggering if user data is loaded wit 'banned' property only

this was happening because of `fieldsToRemove` running before unban logic and clearing out 'banned:expire' field to undefined

src/user/data.js

@@ -118,31 +118,22 @@ module.exports = function (User) {
 	};
 
 	function ensureRequiredFields(fields, fieldsToRemove) {
-		function addField(field) {
-			if (!fields.includes(field)) {
-				fields.push(field);
-				fieldsToRemove.push(field);
-			}
-		}
-
 		if (fields.length && !fields.includes('uid')) {
 			fields.push('uid');
 		}
 
-		if (fields.includes('picture')) {
+		const requiredFields = {
-			addField('uploadedpicture');
+			picture: 'uploadedpicture',
-		}
+			status: 'lastonline',
-
+			banned: 'banned:expire',
-		if (fields.includes('status')) {
+			'banned:expire': 'banned',
-			addField('lastonline');
+			username: 'fullname',
-		}
+		};
-
+		for (const [key, field] of Object.entries(requiredFields)) {
-		if (fields.includes('banned') && !fields.includes('banned:expire')) {
+			if (fields.includes(key) && !fields.includes(field)) {
-			addField('banned:expire');
+				fields.push(field);
-		}
+				fieldsToRemove.push(field);
-
+			}
-		if (fields.includes('username') && !fields.includes('fullname')) {
-			addField('fullname');
 		}
 	}
 
@@ -246,7 +237,7 @@ module.exports = function (User) {
 			}
 
 			if (user.hasOwnProperty('email')) {
-				user.email = validator.escape(user.email ? user.email.toString() : '');
+				user.email = validator.escape(String(user.email || ''));
 			}
 
 			if (!user.uid && !activitypub.helpers.isUri(user.uid)) {
@@ -280,19 +271,6 @@ module.exports = function (User) {
 				user.status = User.getStatus(user);
 			}
 
-			for (let i = 0; i < fieldsToRemove.length; i += 1) {
-				user[fieldsToRemove[i]] = undefined;
-			}
-
-			// User Icons
-			if (requestedFields.includes('picture') && user.username && user.uid !== 0 && !meta.config.defaultAvatar) {
-				if (!iconBackgrounds.includes(user['icon:bgColor'])) {
-					const nameAsIndex = Array.from(user.username).reduce((cur, next) => cur + next.charCodeAt(), 0);
-					user['icon:bgColor'] = iconBackgrounds[nameAsIndex % iconBackgrounds.length];
-				}
-				user['icon:text'] = (user.username[0] || '').toUpperCase();
-			}
-
 			if (user.hasOwnProperty('joindate')) {
 				user.joindateISO = utils.toISOString(user.joindate);
 			}
@@ -305,7 +283,18 @@ module.exports = function (User) {
 				user.muted = user.mutedUntil > Date.now();
 			}
 
-			if (user.hasOwnProperty('banned') || user.hasOwnProperty('banned:expire')) {
+			user.isLocal = utils.isNumber(user.uid);
+
+			// User Icons
+			if (requestedFields.includes('picture') && user.username && user.uid !== 0 && !meta.config.defaultAvatar) {
+				if (!iconBackgrounds.includes(user['icon:bgColor'])) {
+					const nameAsIndex = Array.from(user.username).reduce((cur, next) => cur + next.charCodeAt(), 0);
+					user['icon:bgColor'] = iconBackgrounds[nameAsIndex % iconBackgrounds.length];
+				}
+				user['icon:text'] = (user.username[0] || '').toUpperCase();
+			}
+
+			if (user.hasOwnProperty('banned') && user.hasOwnProperty('banned:expire')) {
 				const result = User.bans.calcExpiredFromUserData(user);
 				user.banned = result.banned;
 				const unban = result.banned && result.banExpired;
@@ -316,9 +305,17 @@ module.exports = function (User) {
 					user.banned = false;
 				}
 			}
-
-			user.isLocal = utils.isNumber(user.uid);
 		});
+
+		// remove fields that were added just for processing
+		fieldsToRemove.forEach((field) => {
+			users.forEach((user) => {
+				if (user) {
+					user[field] = undefined;
+				}
+			});
+		});
+
 		if (unbanUids.length) {
 			await User.bans.unban(unbanUids, '[[user:info.ban-expired]]');
 		}

test/user.js

@@ -1377,6 +1377,16 @@ describe('User', () => {
 			assert(result);
 			assert.strictEqual(result.topicData.title, 'banned topic');
 		});
+
+		it('should unban user properly if only "banned" field is requested', async () => {
+			const testUid = await User.create({ username: 'bannedUser3' });
+			await User.bans.ban(testUid, Date.now() + 2000);
+			assert.strictEqual(await db.isSortedSetMember('users:banned', testUid), true);
+			await setTimeout(3000);
+			await User.getUserFields(testUid, ['uid', 'banned']); // loading their data unbans the user
+			assert.strictEqual(await db.isSortedSetMember('users:banned', testUid), false);
+		});
+
 	});
 
 	describe('Digest.getSubscribers', () => {