Skip to content

Commit

Permalink
Add tests for UsersManager (gristlabs#1149)
Browse files Browse the repository at this point in the history 
Context

HomeDBManager lacks of direct tests, which makes hard to make rework or refactorations.
Proposed solution

Specifically here, I introduce tests which call exposed UsersManager methods directly and check their result.

Also:

    I removed updateUserName which seems to me useless (updateUser does the same work)
    Taking a look at the getUserByLogin methods, it appears that Typescirpt infers it returns a Promise<User|null> while in no case it may resolve a nullish value, therefore I have forced to return a Promise<User> and have changed the call sites to reflect the change.

Related issues

I make this change for then working on gristlabs#870
  • Loading branch information
@fflorent @hexaltation
fflorent authored and hexaltation committed Sep 24, 2024
1 parent 2ed8285 commit 1104d63
Show file tree
Hide file tree
Showing 21 changed files with 1,217 additions and 169 deletions.
2 changes: 1 addition & 1 deletion app/gen-server/ApiServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ export class ApiServer {
throw new ApiError('Name expected in the body', 400);
}
const name = req.body.name;
await this._dbManager.updateUserName(userId, name);
await this._dbManager.updateUser(userId, { name });
res.sendStatus(200);
}));

Expand Down
22 changes: 12 additions & 10 deletions app/gen-server/lib/homedb/HomeDBManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import {
readJson
} from 'app/gen-server/sqlUtils';
import {appSettings} from 'app/server/lib/AppSettings';
import {getOrCreateConnection} from 'app/server/lib/dbUtils';
import {createNewConnection, getOrCreateConnection} from 'app/server/lib/dbUtils';
import {makeId} from 'app/server/lib/idUtils';
import log from 'app/server/lib/log';
import {Permit} from 'app/server/lib/Permit';
Expand All @@ -70,6 +70,7 @@ import {
Brackets,
Connection,
DatabaseType,
DataSourceOptions,
EntityManager,
ObjectLiteral,
SelectQueryBuilder,
Expand Down Expand Up @@ -248,7 +249,6 @@ export type BillingOptions = Partial<Pick<BillingAccount,
export class HomeDBManager extends EventEmitter {
private _usersManager = new UsersManager(this, this._runInTransaction.bind(this));
private _connection: Connection;
private _dbType: DatabaseType;
private _exampleWorkspaceId: number;
private _exampleOrgId: number;
private _idPrefix: string = ""; // Place this before ids in subdomains, used in routing to
Expand All @@ -258,6 +258,10 @@ export class HomeDBManager extends EventEmitter {
// In restricted mode, documents should be read-only.
private _restrictedMode: boolean = false;

private get _dbType(): DatabaseType {
return this._connection.driver.options.type;
}

/**
* Five aclRules, each with one group (with the names 'owners', 'editors', 'viewers',
* 'guests', and 'members') are created by default on every new entity (Organization,
Expand Down Expand Up @@ -348,7 +352,10 @@ export class HomeDBManager extends EventEmitter {

public async connect(): Promise<void> {
this._connection = await getOrCreateConnection();
this._dbType = this._connection.driver.options.type;
}

public async createNewConnection(overrideConf?: Partial<DataSourceOptions>): Promise<void> {
this._connection = await createNewConnection(overrideConf);
}

// make sure special users and workspaces are available
Expand Down Expand Up @@ -461,25 +468,21 @@ export class HomeDBManager extends EventEmitter {
}
}

public async updateUserName(userId: number, name: string) {
return this._usersManager.updateUserName(userId, name);
}

public async updateUserOptions(userId: number, props: Partial<UserOptions>) {
return this._usersManager.updateUserOptions(userId, props);
}

/**
* @see UsersManager.prototype.getUserByLoginWithRetry
*/
public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User> {
return this._usersManager.getUserByLoginWithRetry(email, options);
}

/**
* @see UsersManager.prototype.getUserByLogin
*/
public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User> {
return this._usersManager.getUserByLogin(email, options);
}

Expand Down Expand Up @@ -4367,7 +4370,6 @@ export class HomeDBManager extends EventEmitter {
});
return verifyEntity(orgQuery);
}

}

// Return a QueryResult reflecting the output of a query builder.
Expand Down
6 changes: 3 additions & 3 deletions app/gen-server/lib/homedb/Interfaces.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export type NonGuestGroup = Group & { name: roles.NonGuestRole };

export type Resource = Organization|Workspace|Document;

export type RunInTransaction = (
export type RunInTransaction = <T>(
transaction: EntityManager|undefined,
op: ((manager: EntityManager) => Promise<any>)
) => Promise<any>;
op: ((manager: EntityManager) => Promise<T>)
) => Promise<T>;
136 changes: 68 additions & 68 deletions app/gen-server/lib/homedb/UsersManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export class UsersManager {
public async testClearUserPrefs(emails: string[]) {
return await this._connection.transaction(async manager => {
for (const email of emails) {
const user = await this.getUserByLogin(email, {manager});
const user = await this.getExistingUserByLogin(email, manager);
if (user) {
await manager.delete(Pref, {userId: user.id});
}
Expand All @@ -116,7 +116,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 @@ -125,7 +125,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 @@ -134,7 +134,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 @@ -143,7 +143,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 Expand Up @@ -221,9 +221,6 @@ export class UsersManager {
profile,
manager
});
if (!newUser) {
throw new ApiError("Unable to create user", 500);
}
// No need to survey this user.
newUser.isFirstTimeUser = false;
await newUser.save();
Expand Down Expand Up @@ -286,13 +283,7 @@ export class UsersManager {
return { user, isWelcomed };
}

public async updateUserName(userId: number, name: string) {
const user = await User.findOne({where: {id: userId}});
if (!user) { throw new ApiError("unable to find user", 400); }
user.name = name;
await user.save();
}

// TODO: rather use the updateUser() method, if that makes sense?
public async updateUserOptions(userId: number, props: Partial<UserOptions>) {
const user = await User.findOne({where: {id: userId}});
if (!user) { throw new ApiError("unable to find user", 400); }
Expand Down Expand Up @@ -321,7 +312,7 @@ export class UsersManager {
// for an email key conflict failure. This is in case our transaction conflicts with a peer
// doing the same thing. This is quite likely if the first page visited by a previously
// unseen user fires off multiple api calls.
public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User> {
try {
return await this.getUserByLogin(email, options);
} catch (e) {
Expand Down Expand Up @@ -367,10 +358,10 @@ export class UsersManager {
* unset/outdated fields of an existing record.
*
*/
public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
public async getUserByLogin(email: string, options: GetUserOptions = {}) {
const {manager: transaction, profile, userOptions} = options;
const normalizedEmail = normalizeEmail(email);
const userByLogin = await this._runInTransaction(transaction, async manager => {
return await this._runInTransaction(transaction, async manager => {
let needUpdate = false;
const userQuery = manager.createQueryBuilder()
.select('user')
Expand Down Expand Up @@ -479,9 +470,8 @@ export class UsersManager {
// In principle this could be optimized, but this is simpler to maintain.
user = await userQuery.getOne();
}
return user;
return user!;
});
return userByLogin;
}

/*
Expand Down Expand Up @@ -537,6 +527,63 @@ export class UsersManager {
};
}

public async initializeSpecialIds(): Promise<void> {
await this._maybeCreateSpecialUserId({
email: ANONYMOUS_USER_EMAIL,
name: "Anonymous"
});
await this._maybeCreateSpecialUserId({
email: PREVIEWER_EMAIL,
name: "Preview"
});
await this._maybeCreateSpecialUserId({
email: EVERYONE_EMAIL,
name: "Everyone"
});
await this._maybeCreateSpecialUserId({
email: SUPPORT_EMAIL,
name: "Support"
});
}

/**
*
* Take a list of user profiles coming from the client's session, correlate
* them with Users and Logins in the database, and construct full profiles
* with user ids, standardized display emails, pictures, and anonymous flags.
*
*/
public async completeProfiles(profiles: UserProfile[]): Promise<FullUser[]> {
if (profiles.length === 0) { return []; }
const qb = this._connection.createQueryBuilder()
.select('logins')
.from(Login, 'logins')
.leftJoinAndSelect('logins.user', 'user')
.where('logins.email in (:...emails)', {emails: profiles.map(profile => normalizeEmail(profile.email))});
const completedProfiles: {[email: string]: FullUser} = {};
for (const login of await qb.getMany()) {
completedProfiles[login.email] = {
id: login.user.id,
email: login.displayEmail,
name: login.user.name,
picture: login.user.picture,
anonymous: login.user.id === this.getAnonymousUserId(),
locale: login.user.options?.locale
};
}
return profiles.map(profile => completedProfiles[normalizeEmail(profile.email)])
.filter(fullProfile => fullProfile);
}

/**
* ==================================
*
* Below methods are public but not exposed by HomeDBManager
*
* They are meant to be used internally (i.e. by homedb/ modules)
*
*/

// Looks up the emails in the permission delta and adds them to the users map in
// the delta object.
// Returns a QueryResult based on the validity of the passed in PermissionDelta object.
Expand Down Expand Up @@ -613,25 +660,6 @@ export class UsersManager {
};
}

public async initializeSpecialIds(): Promise<void> {
await this._maybeCreateSpecialUserId({
email: ANONYMOUS_USER_EMAIL,
name: "Anonymous"
});
await this._maybeCreateSpecialUserId({
email: PREVIEWER_EMAIL,
name: "Preview"
});
await this._maybeCreateSpecialUserId({
email: EVERYONE_EMAIL,
name: "Everyone"
});
await this._maybeCreateSpecialUserId({
email: SUPPORT_EMAIL,
name: "Support"
});
}

/**
* Check for anonymous user, either encoded directly as an id, or as a singular
* profile (this case arises during processing of the session/access/all endpoint
Expand Down Expand Up @@ -708,34 +736,6 @@ export class UsersManager {
return members;
}

/**
*
* Take a list of user profiles coming from the client's session, correlate
* them with Users and Logins in the database, and construct full profiles
* with user ids, standardized display emails, pictures, and anonymous flags.
*
*/
public async completeProfiles(profiles: UserProfile[]): Promise<FullUser[]> {
if (profiles.length === 0) { return []; }
const qb = this._connection.createQueryBuilder()
.select('logins')
.from(Login, 'logins')
.leftJoinAndSelect('logins.user', 'user')
.where('logins.email in (:...emails)', {emails: profiles.map(profile => normalizeEmail(profile.email))});
const completedProfiles: {[email: string]: FullUser} = {};
for (const login of await qb.getMany()) {
completedProfiles[login.email] = {
id: login.user.id,
email: login.displayEmail,
name: login.user.name,
picture: login.user.picture,
anonymous: login.user.id === this.getAnonymousUserId(),
locale: login.user.options?.locale
};
}
return profiles.map(profile => completedProfiles[normalizeEmail(profile.email)])
.filter(profile => profile);
}

// For the moment only the support user can add both everyone@ and anon@ to a
// resource, since that allows spam. TODO: enhance or remove.
Expand All @@ -759,7 +759,7 @@ export class UsersManager {
// user if a bunch of servers start simultaneously and the user doesn't exist
// yet.
const user = await this.getUserByLoginWithRetry(profile.email, {profile});
if (user) { id = this._specialUserIds[profile.email] = user.id; }
id = this._specialUserIds[profile.email] = user.id;
}
if (!id) { throw new Error(`Could not find or create user ${profile.email}`); }
return id;
Expand Down
4 changes: 0 additions & 4 deletions app/server/companion.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,6 @@ export function addSiteCommand(program: commander.Command,
const profile = {email, name: email};
const db = await getHomeDBManager();
const user = await db.getUserByLogin(email, {profile});
if (!user) {
// This should not happen.
throw new Error('failed to create user');
}
db.unwrapQueryResult(await db.addOrg(user, {
name: domain,
domain,
Expand Down
6 changes: 2 additions & 4 deletions app/server/lib/TestLogin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,8 @@ export async function getTestLoginSystem(): Promise<GristLoginSystem> {
if (process.env.TEST_SUPPORT_API_KEY) {
const dbManager = gristServer.getHomeDBManager();
const user = await dbManager.getUserByLogin(SUPPORT_EMAIL);
if (user) {
user.apiKey = process.env.TEST_SUPPORT_API_KEY;
await user.save();
}
user.apiKey = process.env.TEST_SUPPORT_API_KEY;
await user.save();
}
return "test-login";
},
Expand Down
Loading

0 comments on commit 1104d63

Please sign in to comment.