Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Mostly styling issues
  • Loading branch information
@fflorent
fflorent committed Aug 6, 2024
1 parent abdfccc commit fae34d1
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 28 deletions.
8 changes: 4 additions & 4 deletions app/gen-server/lib/homedb/UsersManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class UsersManager {
*/
public getAnonymousUserId(): number {
const id = this._specialUserIds[ANONYMOUS_USER_EMAIL];
if (!id) { throw new Error("Anonymous user not available"); }
if (!id) { throw new Error("'Anonymous' user not available"); }
return id;
}

Expand All @@ -123,7 +123,7 @@ export class UsersManager {
*/
public getPreviewerUserId(): number {
const id = this._specialUserIds[PREVIEWER_EMAIL];
if (!id) { throw new Error("Previewer user not available"); }
if (!id) { throw new Error("'Previewer' user not available"); }
return id;
}

Expand All @@ -132,7 +132,7 @@ export class UsersManager {
*/
public getEveryoneUserId(): number {
const id = this._specialUserIds[EVERYONE_EMAIL];
if (!id) { throw new Error("'everyone' user not available"); }
if (!id) { throw new Error("'Everyone' user not available"); }
return id;
}

Expand All @@ -141,7 +141,7 @@ export class UsersManager {
*/
public getSupportUserId(): number {
const id = this._specialUserIds[SUPPORT_EMAIL];
if (!id) { throw new Error("'support' user not available"); }
if (!id) { throw new Error("'Support' user not available"); }
return id;
}

Expand Down
47 changes: 23 additions & 24 deletions test/gen-server/lib/homedb/UsersManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe('UsersManager', function () {

const result = UsersManager.getUsersWithRole(groups, excludedUsersId);

assert.deepEqual( result, new Map([ ['editors', expectedUsers] ]));
assert.deepEqual(result, new Map([ ['editors', expectedUsers] ]));
});
});
});
Expand Down Expand Up @@ -186,9 +186,9 @@ describe('UsersManager', function () {
return localPart;
}

function createUniqueUser(uniqueEmailLocalPart: string) {
function createUniqueUser(uniqueEmailLocalPart: string, options?: GetUserOptions) {
ensureUnique(uniqueEmailLocalPart);
return getOrCreateUser(uniqueEmailLocalPart);
return getOrCreateUser(uniqueEmailLocalPart, options);
}

async function getOrCreateUser(localPart: string, options?: GetUserOptions) {
Expand Down Expand Up @@ -269,10 +269,10 @@ describe('UsersManager', function () {
describe('Without special id initialization', function () {
it('should throw an error', async function () {
await withDataBase('without-special-ids', async function (localDb) {
assert.throws(() => localDb.getAnonymousUserId(), "Anonymous user not available");
assert.throws(() => localDb.getPreviewerUserId(), "Previewer user not available");
assert.throws(() => localDb.getEveryoneUserId(), "'everyone' user not available");
assert.throws(() => localDb.getSupportUserId(), "'support' user not available");
assert.throws(() => localDb.getAnonymousUserId(), "'Anonymous' user not available");
assert.throws(() => localDb.getPreviewerUserId(), "'Previewer' user not available");
assert.throws(() => localDb.getEveryoneUserId(), "'Everyone' user not available");
assert.throws(() => localDb.getSupportUserId(), "'Support' user not available");
});
});
});
Expand All @@ -299,7 +299,7 @@ describe('UsersManager', function () {
assertExists(user, 'Should have returned a user');
assert.strictEqual(user.name, 'Support');
assert.strictEqual(user.loginEmail, SUPPORT_EMAIL);
assertExists(!user.prefs, "should not have retrieved user's prefs");
assert.notExists(user.prefs, "should not have retrieved user's prefs");
});

it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () {
Expand All @@ -308,7 +308,7 @@ describe('UsersManager', function () {
assertExists(user, "Should have retrieved the user");
assertExists(user.loginEmail);
assert.strictEqual(user.loginEmail, expectedUser.loginEmail);
assertExists(Array.isArray(user.prefs), "should not have retrieved user's prefs");
assert.isTrue(Array.isArray(user.prefs), "should not have retrieved user's prefs");
assert.deepEqual(user.prefs, [{
userId: expectedUser.id,
orgId: expectedUser.personalOrg.id,
Expand All @@ -330,7 +330,7 @@ describe('UsersManager', function () {
name: 'Support'
};
assert.deepInclude(user, expectedResult);
assert.ok(!user.anonymous, 'anonymous property should be falsy');
assert.notOk(user.anonymous, 'anonymous property should be falsy');
});

it('should return the anonymous user', async function () {
Expand All @@ -345,10 +345,10 @@ describe('UsersManager', function () {
name: 'Anonymous',
};
assert.deepInclude(user, expectedResult);
assert.ok(!user.isSupport, 'support property should be falsy');
assert.notOk(user.isSupport, 'support property should be falsy');
});

it('should throw when user is not found', async function () {
it('should reject when user is not found', async function () {
await assert.isRejected(db.getFullUser(NON_EXISTING_USER_ID), "unable to find user");
});
});
Expand Down Expand Up @@ -418,7 +418,7 @@ describe('UsersManager', function () {
const fullUser = db.makeFullUser(anon);

assert.isTrue(fullUser.anonymous, "`anonymous` property should be set to true");
assert.ok(!fullUser.isSupport, "`isSupport` should be falsy");
assert.notOk(fullUser.isSupport, "`isSupport` should be falsy");
});

it('sets `isSupport` property to true for support account', async function () {
Expand All @@ -427,7 +427,7 @@ describe('UsersManager', function () {
const fullUser = db.makeFullUser(support!);

assert.isTrue(fullUser.isSupport, "`isSupport` property should be set to true");
assert.ok(!fullUser.anonymous, "`anonymouse` should be falsy");
assert.notOk(fullUser.anonymous, "`anonymouse` should be falsy");
});

it('should throw when no displayEmail exist for this user', function () {
Expand Down Expand Up @@ -621,7 +621,6 @@ describe('UsersManager', function () {
await db.updateUserOptions(createdUser.id, options);

const updatedUser = await getOrCreateUser(localPart);
assertExists(updatedUser);
assertExists(updatedUser.options);
assert.deepEqual(updatedUser.options, options);
});
Expand Down Expand Up @@ -781,7 +780,7 @@ describe('UsersManager', function () {
sandbox.stub(UsersManager.prototype, 'getUserByLogin')
.onFirstCall().throws(makeQueryFailedError())
.callThrough();
await ensureGetUserByLoginWithRetryWorks( 'getuserbyloginwithretry-makes-a-single-retry');
await ensureGetUserByLoginWithRetryWorks('getuserbyloginwithretry-makes-a-single-retry');
});

it('should reject after 2 attempts', async function () {
Expand Down Expand Up @@ -819,8 +818,7 @@ describe('UsersManager', function () {
}

it('should refuse to delete the account of someone else', async function () {
const localPart = ensureUnique('deleteuser-refuses-for-someone-else');
const userToDelete = await getOrCreateUser(localPart);
const userToDelete = await createUniqueUser('deleteuser-refuses-for-someone-else');

const promise = db.deleteUser({userId: 2}, userToDelete.id);

Expand All @@ -837,8 +835,9 @@ describe('UsersManager', function () {

it('should refuse to delete the account if the passed name is not matching', async function () {
disableLoggingLevel('debug');
const localPart = ensureUnique('deleteuser-refuses-if-name-not-matching');
const userToDelete = await getOrCreateUser(localPart, {
const localPart = 'deleteuser-refuses-if-name-not-matching';

const userToDelete = await createUniqueUser(localPart, {
profile: {
name: 'someone to delete',
email: makeEmail(localPart),
Expand All @@ -852,8 +851,8 @@ describe('UsersManager', function () {
});

it('should remove the user and cleanup their info and personal organization', async function () {
const localPart = ensureUnique('deleteuser-removes-user-and-cleanups-info');
const userToDelete = await getOrCreateUser(localPart, {
const localPart = 'deleteuser-removes-user-and-cleanups-info';
const userToDelete = await createUniqueUser(localPart, {
profile: {
name: 'someone to delete',
email: makeEmail(localPart),
Expand All @@ -879,9 +878,9 @@ describe('UsersManager', function () {
});

it("should remove the user when passed name corresponds to the user's name", async function () {
const localPart = ensureUnique('deleteuser-removes-user-when-name-matches');
const userName = 'someone to delete';
const userToDelete = await getOrCreateUser(localPart, {
const localPart = 'deleteuser-removes-user-when-name-matches';
const userToDelete = await createUniqueUser(localPart, {
profile: {
name: userName,
email: makeEmail(localPart),
Expand Down

0 comments on commit fae34d1

Please sign in to comment.