Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable filters config and scientific conditions on the backend #1337

Merged
merged 15 commits into from
Aug 21, 2024
Merged
2 changes: 2 additions & 0 deletions src/users/dto/create-user-settings.dto.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { ApiProperty } from "@nestjs/swagger";
import { UpdateUserSettingsDto } from "./update-user-settings.dto";
import { IsString } from "class-validator";

export class CreateUserSettingsDto extends UpdateUserSettingsDto {
@ApiProperty({ type: String, required: true })
@IsString()
readonly userId: string;
}
16 changes: 16 additions & 0 deletions src/users/dto/update-user-settings.dto.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,30 @@
import { ApiProperty, PartialType } from "@nestjs/swagger";
import {
FilterConfig,
ScientificCondition,
} from "../schemas/user-settings.schema";
import { IsArray, IsNumber } from "class-validator";

export class UpdateUserSettingsDto {
@ApiProperty()
@IsArray()
readonly columns: Record<string, unknown>[];

@ApiProperty({ type: Number, required: false, default: 25 })
@IsNumber()
readonly datasetCount?: number;

@ApiProperty({ type: Number, required: false, default: 25 })
@IsNumber()
readonly jobCount?: number;

@ApiProperty()
@IsArray()
readonly filters: FilterConfig[];

@ApiProperty()
@IsArray()
readonly conditions: ScientificCondition[];
}

export class PartialUpdateUserSettingsDto extends PartialType(
Expand Down
3 changes: 3 additions & 0 deletions src/users/interceptors/create-user-settings.interceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { Observable, tap } from "rxjs";
import { CreateUserSettingsDto } from "../dto/create-user-settings.dto";
import { UsersService } from "../users.service";
import { kDefaultFilters } from "../schemas/user-settings.schema";

@Injectable()
export class CreateUserSettingsInterceptor implements NestInterceptor {
Expand All @@ -34,6 +35,8 @@ export class CreateUserSettingsInterceptor implements NestInterceptor {
const createUserSettingsDto: CreateUserSettingsDto = {
userId,
columns: [],
filters: kDefaultFilters,
conditions: [],
};
return this.usersService.createUserSettings(
userId,
Expand Down
34 changes: 34 additions & 0 deletions src/users/interceptors/default-user-settings.interceptor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import {
CallHandler,
ExecutionContext,
Injectable,
Logger,
NestInterceptor,
} from "@nestjs/common";
import { map, Observable, tap } from "rxjs";

Check warning on line 8 in src/users/interceptors/default-user-settings.interceptor.ts

View workflow job for this annotation

GitHub Actions / eslint

'tap' is defined but never used
import { CreateUserSettingsDto } from "../dto/create-user-settings.dto";

Check warning on line 9 in src/users/interceptors/default-user-settings.interceptor.ts

View workflow job for this annotation

GitHub Actions / eslint

'CreateUserSettingsDto' is defined but never used
import { UsersService } from "../users.service";
import { kDefaultFilters } from "../schemas/user-settings.schema";
import { UpdateUserSettingsDto } from "../dto/update-user-settings.dto";

@Injectable()
export class DefaultUserSettingsInterceptor implements NestInterceptor {
constructor(private usersService: UsersService) {}
async intercept(
context: ExecutionContext,
next: CallHandler,
): Promise<Observable<unknown>> {
return next.handle().pipe(
map(async () => {
Logger.log("DefaultUserSettingsInterceptor");
const defaultUserSettings: UpdateUserSettingsDto = {
columns: [],
filters: kDefaultFilters,
conditions: [],
};
console.log(defaultUserSettings);
return defaultUserSettings;
}),
);
}
}
56 changes: 56 additions & 0 deletions src/users/schemas/user-settings.schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,43 @@ import { Document } from "mongoose";

export type UserSettingsDocument = UserSettings & Document;

// Define possible filter component types as a union of string literals
export type FilterComponentType =
| "LocationFilterComponent"
| "PidFilterComponent"
| "PidFilterContainsComponent"
| "PidFilterStartsWithComponent"
| "GroupFilterComponent"
| "TypeFilterComponent"
| "KeywordFilterComponent"
| "DateRangeFilterComponent"
| "TextFilterComponent";

// Define the Filter interface
export interface FilterConfig {
type: FilterComponentType;
visible: boolean;
}

// Define the Condition interface
export interface ScientificCondition {
field: string;
value: string;
operator: string;
}

export const kDefaultFilters: FilterConfig[] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k stands for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, old habit - roots from my C++ experience to follow Google code styling standards

k - prefix for a static const

I used to maintain JavaScriptMVC (pre-Angular SPA vanilla JS framework), together with the folks we adopted that standard also for JS

If that makes any sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it would be more appreciated if the naming is consistent with existing naming convention within the application scope.

Copy link
Contributor

@Junjiequan Junjiequan Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#Sure, what is the current convention?

We don't have a strict naming convention in this project yet, but we should have one documented in the future.
Google also has a code styling standard for typescript as well, which, generally speaking is like unspoken standard for most of the typescript applications

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will check that out!

{ type: "LocationFilterComponent", visible: true },
{ type: "PidFilterComponent", visible: true },
{ type: "PidFilterContainsComponent", visible: false },
{ type: "PidFilterStartsWithComponent", visible: false },
{ type: "GroupFilterComponent", visible: true },
{ type: "TypeFilterComponent", visible: true },
{ type: "KeywordFilterComponent", visible: true },
{ type: "DateRangeFilterComponent", visible: true },
{ type: "TextFilterComponent", visible: true },
];

@Schema({
collection: "UserSetting",
toJSON: {
Expand Down Expand Up @@ -43,6 +80,25 @@ export class UserSettings {
@ApiProperty({ type: String, required: true })
@Prop({ type: mongoose.Schema.Types.ObjectId, ref: "User", required: true })
userId: string;

@ApiProperty({
type: [Object],
default: kDefaultFilters,
description: "Array of filters the user has set",
})
@Prop({
type: [{ type: Object }],
default: kDefaultFilters,
})
filters: FilterConfig[];

@ApiProperty({
type: [Object],
default: [],
description: "Array of conditions the user has set",
})
@Prop({ type: [{ type: Object }], default: [] })
conditions: ScientificCondition[];
}

export const UserSettingsSchema = SchemaFactory.createForClass(UserSettings);
75 changes: 75 additions & 0 deletions src/users/users.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,43 @@
import { CaslModule } from "src/casl/casl.module";
import { UsersController } from "./users.controller";
import { UsersService } from "./users.service";
import { UpdateUserSettingsDto } from "./dto/update-user-settings.dto";

class UsersServiceMock {
findByIdUserIdentity(id: string) {
return { id };
}

async findByIdUserSettings(userId: string) {

Check warning on line 13 in src/users/users.controller.spec.ts

View workflow job for this annotation

GitHub Actions / eslint

'userId' is defined but never used
return mockUserSettings;
}

async findOneAndUpdateUserSettings(
userId: string,
updateUserSettingsDto: UpdateUserSettingsDto,
) {
return { ...updateUserSettingsDto, _id: userId };
}
}

const mockUserSettings = {
_id: "user1",
userId: "user1",
columns: [],
datasetCount: 25,
jobCount: 25,
filters: [
{ type: "LocationFilterComponent", visible: true },
{ type: "PidFilterComponent", visible: true },
],
conditions: [{ field: "status", value: "active", operator: "equals" }],
};

class AuthServiceMock {}

describe("UsersController", () => {
let controller: UsersController;
let usersService: UsersService;

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
Expand All @@ -26,9 +52,58 @@
}).compile();

controller = module.get<UsersController>(UsersController);
usersService = module.get<UsersService>(UsersService);

// bypass authorization
jest
.spyOn(controller as UsersController, "checkUserAuthorization")
.mockImplementation(() => Promise.resolve());
});

it("should be defined", () => {
expect(controller).toBeDefined();
});

it("should return user settings with filters and conditions", async () => {
jest
.spyOn(usersService, "findByIdUserSettings")
.mockResolvedValue(mockUserSettings);

const userId = "user1";
const result = await controller.getSettings(
{ user: { _id: userId } },
userId,
);

expect(result).toEqual(mockUserSettings);
expect(result.filters).toBeDefined();
expect(result.filters.length).toBeGreaterThan(0);
expect(result.conditions).toBeDefined();
expect(result.conditions.length).toBeGreaterThan(0);
});

it("should update user settings with filters and conditions", async () => {
const updatedSettings = {
...mockUserSettings,
filters: [{ type: "PidFilterContainsComponent", visible: false }],
conditions: [{ field: "status", value: "inactive", operator: "equals" }],
};

jest
.spyOn(usersService, "findOneAndUpdateUserSettings")
.mockResolvedValue(updatedSettings);

const userId = "user-id";
const result = await controller.updateSettings(
{ user: { _id: userId } },
userId,
updatedSettings,
);

expect(result).toEqual(updatedSettings);
expect(result.filters).toBeDefined();
expect(result.filters.length).toBe(1);
expect(result.conditions).toBeDefined();
expect(result.conditions.length).toBe(1);
});
});
44 changes: 43 additions & 1 deletion src/users/users.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
Body,
ForbiddenException,
HttpCode,
CanActivate,
} from "@nestjs/common";
import {
ApiBearerAuth,
Expand All @@ -31,7 +32,10 @@
import { JWTUser } from "../auth/interfaces/jwt-user.interface";
import { UserSettings } from "./schemas/user-settings.schema";
import { CreateUserSettingsDto } from "./dto/create-user-settings.dto";
import { PartialUpdateUserSettingsDto } from "./dto/update-user-settings.dto";
import {
PartialUpdateUserSettingsDto,
UpdateUserSettingsDto,

Check warning on line 37 in src/users/users.controller.ts

View workflow job for this annotation

GitHub Actions / eslint

'UpdateUserSettingsDto' is defined but never used
} from "./dto/update-user-settings.dto";
import { User } from "./schemas/user.schema";
import { CreateUserSettingsInterceptor } from "./interceptors/create-user-settings.interceptor";
import { AuthService } from "src/auth/auth.service";
Expand All @@ -44,6 +48,8 @@
import { AuthenticatedPoliciesGuard } from "../casl/guards/auth-check.guard";
import { ReturnedUserDto } from "./dto/returned-user.dto";
import { ReturnedAuthLoginDto } from "src/auth/dto/returnedLogin.dto";
import { PoliciesGuard } from "src/casl/guards/policies.guard";
import { DefaultUserSettingsInterceptor } from "./interceptors/default-user-settings.interceptor";

@ApiBearerAuth()
@ApiTags("users")
Expand Down Expand Up @@ -307,6 +313,42 @@
return this.usersService.findOneAndDeleteUserSettings(id);
}

@UseInterceptors(DefaultUserSettingsInterceptor)
@UseGuards(
class ByPassAuthenticatedPoliciesGuard
extends PoliciesGuard
implements CanActivate
{
async canActivate(): Promise<boolean> {
return Promise.resolve(true);
}
},
)
Comment on lines +317 to +326
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the endpoint should open to unauthenticated users, we can just use PoliciesGuard or remove UseGuards at all I suppose. Since we are not checking policies here

@Get("/settings/default")
async getDefaultSettings(): Promise<UserSettings> {
return Promise.resolve(new UserSettings());
}

@UseGuards(AuthenticatedPoliciesGuard)
@CheckPolicies((ability: AppAbility) =>
ability.can(Action.UserCreateAny, User),
)
@Post("/settings/default")
async postDefaultSettings(
@Req() request: Request,
@Body() createUserSettingsDto: CreateUserSettingsDto,
): Promise<UserSettings> {
await this.checkUserAuthorization(
request,
[Action.UserCreateAny],
createUserSettingsDto.userId,
);
return this.usersService.createUserSettings(
"default",
createUserSettingsDto,
);
}

@UseGuards(AuthenticatedPoliciesGuard)
@CheckPolicies((ability: AppAbility) => {
return (
Expand Down
4 changes: 3 additions & 1 deletion test/Users.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe("2360: Users settings", () => {
);
});

it("0010: Update users settings with valid value should sucess ", async () => {
it("0010: Update users settings with valid value should success ", async () => {
return request(appUrl)
.put(`/api/v3/Users/${userIdUser1}/settings`)
.set("Accept", "application/json")
Expand All @@ -64,6 +64,8 @@ describe("2360: Users settings", () => {
res.body.should.have.property("userId", userIdUser1);
res.body.should.have.property("datasetCount");
res.body.should.have.property("jobCount");
res.body.should.have.property("filters");
res.body.should.have.property("conditions");
});
});
});
Loading