From 6e006234b262d9ec1c24c18df70f734316888cc2 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Wed, 23 Jun 2021 07:50:28 +0300 Subject: [PATCH 1/8] Fix strange town code -> id missing transform when creating violations --- src/sections/entities/town.entity.ts | 2 +- src/sections/sections.module.ts | 1 + src/violations/api/violation.dto.ts | 4 ++- .../entities/violations.repository.ts | 26 ++++++++----------- src/violations/violations.module.ts | 2 ++ 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/sections/entities/town.entity.ts b/src/sections/entities/town.entity.ts index 7a0b357c..22a23408 100644 --- a/src/sections/entities/town.entity.ts +++ b/src/sections/entities/town.entity.ts @@ -19,7 +19,7 @@ export class Town { readonly id: number; @Column() - readonly code: number; + code: number; @Column() readonly name: string; diff --git a/src/sections/sections.module.ts b/src/sections/sections.module.ts index 2cec453c..4140e0af 100644 --- a/src/sections/sections.module.ts +++ b/src/sections/sections.module.ts @@ -54,6 +54,7 @@ import { CityRegionsRepository } from './entities/cityRegions.repository'; MunicipalitiesRepository, CountriesRepository, CityRegionsRepository, + TownsRepository, ], controllers: [ SectionsController, diff --git a/src/violations/api/violation.dto.ts b/src/violations/api/violation.dto.ts index 55db90e5..2053beef 100644 --- a/src/violations/api/violation.dto.ts +++ b/src/violations/api/violation.dto.ts @@ -46,7 +46,8 @@ export class ViolationDto { @Expose({ groups: ['read', 'create'] }) @Type(() => TownDto) @Transform( - ({ value: id }) => plainToClass(TownDto, { id }, { groups: ['create'] }), + ({ value: id }) => + plainToClass(TownDto, { code: id }, { groups: ['create'] }), { groups: ['create'] }, ) @IsNotEmpty({ groups: ['create'] }) @@ -111,6 +112,7 @@ export class ViolationDto { groups: ['create'], }, ); + violation.town.code = this.town.id; let sortPosition = 1; violation.pictures = (violation.pictures || []).map( diff --git a/src/violations/entities/violations.repository.ts b/src/violations/entities/violations.repository.ts index 4c35b696..a0078263 100644 --- a/src/violations/entities/violations.repository.ts +++ b/src/violations/entities/violations.repository.ts @@ -5,11 +5,13 @@ import { User } from '../../users/entities'; import { Violation } from './violation.entity'; import { ViolationUpdateType } from './violation-update.entity'; import { ViolationsFilters } from '../api/violations-filters.dto'; +import { TownsRepository } from 'src/sections/entities/towns.repository'; @Injectable() export class ViolationsRepository { constructor( @InjectRepository(Violation) private readonly repo: Repository, + private readonly townsRepo: TownsRepository, ) {} findOneOrFail(id: string): Promise { @@ -60,31 +62,25 @@ export class ViolationsRepository { qb.andWhere('town.code = :town', { town: filters.town }); } - if (filters.author || filters.organization) { + if (filters.organization) { qb.innerJoin('violation.updates', 'update_send'); qb.andWhere('update_send.type = :update', { update: ViolationUpdateType.SEND, }); - - if (filters.author) { - qb.andWhere('update_send.actor_id = :author', { - author: filters.author, - }); - } - - if (filters.organization) { - qb.innerJoin('update_send.actor', 'sender'); - qb.innerJoin('sender.organization', 'organization'); - qb.andWhere('organization.id = :organization', { - organization: filters.organization, - }); - } + qb.innerJoin('update_send.actor', 'sender'); + qb.innerJoin('sender.organization', 'organization'); + qb.andWhere('organization.id = :organization', { + organization: filters.organization, + }); } return qb; } async save(violation: Violation): Promise { + if (violation.town && !violation.town.id && violation.town.code) { + violation.town = await this.townsRepo.findOneByCode(violation.town.code); + } await this.repo.save(violation); return this.findOneOrFail(violation.id); diff --git a/src/violations/violations.module.ts b/src/violations/violations.module.ts index bebcd5fd..680ab39b 100644 --- a/src/violations/violations.module.ts +++ b/src/violations/violations.module.ts @@ -11,6 +11,7 @@ import { ViolationUpdate } from './entities/violation-update.entity'; import { Violation } from './entities/violation.entity'; import { ViolationsRepository } from './entities/violations.repository'; import { UsersModule } from 'src/users/users.module'; +import { SectionsModule } from 'src/sections/sections.module'; @Module({ imports: [ @@ -18,6 +19,7 @@ import { UsersModule } from 'src/users/users.module'; CaslModule, PicturesModule, UsersModule, + SectionsModule, ], controllers: [ ViolationsController, From 56832833225ac1aa043db3a38c82565f8236969c Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Wed, 23 Jun 2021 07:50:42 +0300 Subject: [PATCH 2/8] Add missing indices for filtering towns --- ...01383-AddTownsCountryMunicipalityIndices.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 src/migrations/1624337501383-AddTownsCountryMunicipalityIndices.ts diff --git a/src/migrations/1624337501383-AddTownsCountryMunicipalityIndices.ts b/src/migrations/1624337501383-AddTownsCountryMunicipalityIndices.ts new file mode 100644 index 00000000..478f5b3b --- /dev/null +++ b/src/migrations/1624337501383-AddTownsCountryMunicipalityIndices.ts @@ -0,0 +1,18 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddTownsCountryMunicipalityIndices1624337501383 + implements MigrationInterface { + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + create index "towns_country_id_key" on "towns" ("country_id"); + create index "towns_municipality_id_key" on "towns" ("country_id"); + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + drop index "towns_municipality_id_key"; + drop index "towns_country_id_key"; + `); + } +} From 25f0bc61eaa99ad3979184431846561bfecfe780 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sat, 26 Jun 2021 10:16:20 +0300 Subject: [PATCH 3/8] Add filters to violations --- src/sections/api/town.dto.ts | 8 ++++- src/violations/api/violations-filters.dto.ts | 14 +++++++- .../entities/violations.repository.ts | 35 +++++++++++++++++-- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/src/sections/api/town.dto.ts b/src/sections/api/town.dto.ts index 15ca77d1..03ec90b2 100644 --- a/src/sections/api/town.dto.ts +++ b/src/sections/api/town.dto.ts @@ -1,5 +1,11 @@ import { ApiProperty } from '@nestjs/swagger'; -import { Exclude, Expose, plainToClass, Type } from 'class-transformer'; +import { + Exclude, + Expose, + plainToClass, + Transform, + Type, +} from 'class-transformer'; import { IsInt, IsNotEmpty, IsNumber, Min } from 'class-validator'; import { StreamDto } from 'src/streams/api/stream.dto'; import { Town } from '../entities'; diff --git a/src/violations/api/violations-filters.dto.ts b/src/violations/api/violations-filters.dto.ts index b100ff03..1d64cde9 100644 --- a/src/violations/api/violations-filters.dto.ts +++ b/src/violations/api/violations-filters.dto.ts @@ -13,11 +13,23 @@ export class ViolationsFilters extends PageDTO { status: ViolationStatus; @Optional() - author: string; + electionRegion: string; + + @Optional() + municipality: string; + + @Optional() + country: string; @Optional() town: number; + @Optional() + cityRegion: string; + @Optional() organization: number; + + @Optional() + published: boolean; } diff --git a/src/violations/entities/violations.repository.ts b/src/violations/entities/violations.repository.ts index a0078263..70696085 100644 --- a/src/violations/entities/violations.repository.ts +++ b/src/violations/entities/violations.repository.ts @@ -34,7 +34,6 @@ export class ViolationsRepository { ): SelectQueryBuilder { const qb = this.repo.createQueryBuilder('violation'); - qb.leftJoinAndSelect('violation.section', 'section'); qb.innerJoinAndSelect('violation.town', 'town'); qb.innerJoinAndSelect('violation.updates', 'update'); qb.innerJoinAndSelect('update.actor', 'actor'); @@ -49,17 +48,47 @@ export class ViolationsRepository { } if (filters.section) { + qb.innerJoinAndSelect('violation.section', 'section'); qb.andWhere('section.id LIKE :section', { section: `${filters.section}%`, }); + } else { + qb.leftJoinAndSelect('violation.section', 'section'); + } + + if (filters.electionRegion) { + qb.innerJoin('town.municipality', 'municipality'); + qb.innerJoin('municipality.electionRegions', 'electionRegions'); + qb.andWhere('electionRegions.code = :electionRegion', { + electionRegion: filters.electionRegion, + }); + + if (filters.municipality) { + qb.andWhere('municipality.code = :municipality', { + municipality: filters.municipality, + }); + } + + if (filters.country) { + qb.innerJoin('town.country', 'country'); + qb.andWhere('country.code = :country', { country: filters.country }); + } + + if (filters.town) { + qb.andWhere('town.code = :town', { + town: filters.town, + }); + } } if (filters.status) { qb.andWhere('violation.status = :status', { status: filters.status }); } - if (filters.town) { - qb.andWhere('town.code = :town', { town: filters.town }); + if (filters.published) { + qb.andWhere('violation.isPublished = :published', { + published: filters.published, + }); } if (filters.organization) { From 41725c1b166e158aeb0cd541b9aca1fcb68f8121 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sat, 26 Jun 2021 16:53:08 +0300 Subject: [PATCH 4/8] Translate generic 202 accepted status --- src/i18n/bg/status.json | 3 ++- src/i18n/en/status.json | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/i18n/bg/status.json b/src/i18n/bg/status.json index 0716dfec..27d38bca 100644 --- a/src/i18n/bg/status.json +++ b/src/i18n/bg/status.json @@ -13,5 +13,6 @@ "BROADCAST_PENDING": "В изчакване", "BROADCAST_PROCESSING": "Обработва се", "BROADCAST_PUBLISHED": "Публикувано", - "BROADCAST_DISCARDED": "Отхвърлено" + "BROADCAST_DISCARDED": "Отхвърлено", + "OBJECT_ACCEPTED": "Прието" } diff --git a/src/i18n/en/status.json b/src/i18n/en/status.json index e1d0a905..643b814e 100644 --- a/src/i18n/en/status.json +++ b/src/i18n/en/status.json @@ -13,5 +13,6 @@ "BROADCAST_PENDING": "Pending", "BROADCAST_PROCESSING": "Processing", "BROADCAST_PUBLISHED": "Published", - "BROADCAST_DISCARDED": "Discarded" + "BROADCAST_DISCARDED": "Discarded", + "OBJECT_ACCEPTED": "Accepted" } From 1507f2d13162de42b1d78a023adc15c75b764d74 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sat, 26 Jun 2021 16:59:35 +0300 Subject: [PATCH 5/8] Restrict listing violations to lawyers and admins only --- src/violations/api/violations.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/violations/api/violations.controller.ts b/src/violations/api/violations.controller.ts index cf0a97d2..02cffd51 100644 --- a/src/violations/api/violations.controller.ts +++ b/src/violations/api/violations.controller.ts @@ -38,7 +38,7 @@ export class ViolationsController { @Get() @HttpCode(200) @UseGuards(PoliciesGuard) - @CheckPolicies((ability: Ability) => ability.can(Action.Read, Violation)) + @CheckPolicies((ability: Ability) => ability.can(Action.Manage, Violation)) @UsePipes(new ValidationPipe({ transform: true })) async index( @Query() query: ViolationsFilters, From afc889f0114364340a1e2ed4f0eb5b956025f242 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sat, 26 Jun 2021 17:20:15 +0300 Subject: [PATCH 6/8] Correct and simplify joins for violations filtering --- src/violations/entities/violations.repository.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/violations/entities/violations.repository.ts b/src/violations/entities/violations.repository.ts index 70696085..f00bd083 100644 --- a/src/violations/entities/violations.repository.ts +++ b/src/violations/entities/violations.repository.ts @@ -35,9 +35,13 @@ export class ViolationsRepository { const qb = this.repo.createQueryBuilder('violation'); qb.innerJoinAndSelect('violation.town', 'town'); - qb.innerJoinAndSelect('violation.updates', 'update'); - qb.innerJoinAndSelect('update.actor', 'actor'); - qb.innerJoinAndSelect('actor.organization', 'organization'); + qb.innerJoinAndSelect('violation.updates', 'update_send'); + qb.andWhere('update_send.type = :update', { + update: ViolationUpdateType.SEND, + }); + qb.innerJoinAndSelect('update_send.actor', 'sender'); + qb.innerJoinAndSelect('sender.organization', 'organization'); + qb.innerJoinAndSelect('violation.updates', 'updates'); qb.leftJoinAndSelect('violation.pictures', 'picture'); if (filters.assignee) { @@ -92,12 +96,6 @@ export class ViolationsRepository { } if (filters.organization) { - qb.innerJoin('violation.updates', 'update_send'); - qb.andWhere('update_send.type = :update', { - update: ViolationUpdateType.SEND, - }); - qb.innerJoin('update_send.actor', 'sender'); - qb.innerJoin('sender.organization', 'organization'); qb.andWhere('organization.id = :organization', { organization: filters.organization, }); From e237050ef568e8eb95bfb872721c0535b4cd88b2 Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sat, 26 Jun 2021 17:20:36 +0300 Subject: [PATCH 7/8] Validate violations filters query params --- src/sections/api/town-exists.constraint.ts | 2 +- .../api/organization-exists.constraint.ts | 2 +- src/users/api/user-exists.constraint.ts | 6 +-- src/utils/ulid-constraint.ts | 33 ++++++++++++ src/violations/api/violations-filters.dto.ts | 51 +++++++++++++++---- 5 files changed, 78 insertions(+), 16 deletions(-) create mode 100644 src/utils/ulid-constraint.ts diff --git a/src/sections/api/town-exists.constraint.ts b/src/sections/api/town-exists.constraint.ts index 726a0ce5..9e2f72a1 100644 --- a/src/sections/api/town-exists.constraint.ts +++ b/src/sections/api/town-exists.constraint.ts @@ -34,7 +34,7 @@ export class IsTownExistsConstraint implements ValidatorConstraintInterface { } export function IsTownExists(validationOptions?: ValidationOptions) { - return function (town: TownDto, propertyName: string) { + return function (town: TownDto | object, propertyName: string) { registerDecorator({ target: town.constructor, propertyName: propertyName, diff --git a/src/users/api/organization-exists.constraint.ts b/src/users/api/organization-exists.constraint.ts index 8add9631..dd65b31e 100644 --- a/src/users/api/organization-exists.constraint.ts +++ b/src/users/api/organization-exists.constraint.ts @@ -32,7 +32,7 @@ export class IsOrganizationExistsConstraint } export function IsOrganizationExists(validationOptions?: ValidationOptions) { - return function (object: OrganizationDto, propertyName: string) { + return function (object: OrganizationDto | object, propertyName: string) { registerDecorator({ target: object.constructor, propertyName: propertyName, diff --git a/src/users/api/user-exists.constraint.ts b/src/users/api/user-exists.constraint.ts index 3afbf9c0..ada33ee5 100644 --- a/src/users/api/user-exists.constraint.ts +++ b/src/users/api/user-exists.constraint.ts @@ -24,14 +24,14 @@ export class IsUserExistsConstraint implements ValidatorConstraintInterface { } defaultMessage?(): string { - return `User with $property "$value" does not exist!`; + return `User with ID "$value" does not exist!`; } } export function IsUserExists(validationOptions?: ValidationOptions) { - return function (user: UserDto, propertyName: string) { + return function (target: UserDto | object, propertyName: string) { registerDecorator({ - target: user.constructor, + target: target.constructor, propertyName: propertyName, options: validationOptions, constraints: [], diff --git a/src/utils/ulid-constraint.ts b/src/utils/ulid-constraint.ts new file mode 100644 index 00000000..b4a61141 --- /dev/null +++ b/src/utils/ulid-constraint.ts @@ -0,0 +1,33 @@ +import { Inject, Injectable } from '@nestjs/common'; +import { + registerDecorator, + ValidationArguments, + ValidationOptions, + ValidatorConstraint, + ValidatorConstraintInterface, +} from 'class-validator'; + +@ValidatorConstraint({ async: true }) +@Injectable() +export class IsULIDConstraint implements ValidatorConstraintInterface { + async validate(id?: string): Promise { + return typeof id === 'string' && /[A-Z0-9]{26}/.test(id); + } + + defaultMessage?(context: ValidationArguments): string { + console.debug(context.object); + return `${context.property} identifier is not an ULID`; + } +} + +export function IsULID(validationOptions?: ValidationOptions) { + return function (entity: object, propertyName: string) { + registerDecorator({ + target: entity.constructor, + propertyName: propertyName, + options: validationOptions, + constraints: [], + validator: IsULIDConstraint, + }); + }; +} diff --git a/src/violations/api/violations-filters.dto.ts b/src/violations/api/violations-filters.dto.ts index 1d64cde9..d0bc6680 100644 --- a/src/violations/api/violations-filters.dto.ts +++ b/src/violations/api/violations-filters.dto.ts @@ -1,35 +1,64 @@ -import { Optional } from '@nestjs/common'; +import { Type } from 'class-transformer'; +import { + IsBoolean, + IsBooleanString, + IsInt, + IsNumber, + IsNumberString, + IsOptional, + Length, +} from 'class-validator'; +import { IsTownExists } from 'src/sections/api/town-exists.constraint'; +import { IsOrganizationExists } from 'src/users/api/organization-exists.constraint'; +import { IsUserExists } from 'src/users/api/user-exists.constraint'; import { PageDTO } from 'src/utils/page.dto'; +import { IsULID } from 'src/utils/ulid-constraint'; import { ViolationStatus } from '../entities/violation.entity'; export class ViolationsFilters extends PageDTO { - @Optional() + @IsOptional() + @IsULID() + @IsUserExists() assignee: string; - @Optional() + @IsOptional() + @Length(1, 9) section: string; - @Optional() + @IsOptional() status: ViolationStatus; - @Optional() + @IsOptional() + @IsNumberString() + @Length(2, 2) electionRegion: string; - @Optional() + @IsOptional() + @IsNumberString() + @Length(2, 2) municipality: string; - @Optional() + @IsOptional() + @IsNumberString() + @Length(2, 2) country: string; - @Optional() + @IsOptional() + @IsTownExists() town: number; - @Optional() + @IsOptional() + @IsNumberString() + @Length(2, 2) cityRegion: string; - @Optional() + @IsOptional() + @IsInt() + @Type(() => Number) + @IsOrganizationExists() organization: number; - @Optional() + @IsOptional() + @IsBooleanString() published: boolean; } From a750ddf642af614490c85293509a35d6ec6a7c1c Mon Sep 17 00:00:00 2001 From: Haralan Dobrev Date: Sun, 27 Jun 2021 14:30:09 +0300 Subject: [PATCH 8/8] Fix style and ignore some warnings which we do not have a good way to work around --- .eslintrc.js | 49 ++++++++++++++++++++ src/sections/api/town.dto.ts | 8 +--- src/utils/ulid-constraint.ts | 2 +- src/violations/api/violations-filters.dto.ts | 2 - 4 files changed, 51 insertions(+), 10 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 24e94823..b07a6c3f 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -21,5 +21,54 @@ module.exports = { '@typescript-eslint/explicit-module-boundary-types': 'off', '@typescript-eslint/no-explicit-any': 'off', '@typescript-eslint/no-inferrable-types': 'off', + '@typescript-eslint/ban-types': [ + 'error', + { + types: { + String: { + message: 'Use string instead', + fixWith: 'string', + }, + Boolean: { + message: 'Use boolean instead', + fixWith: 'boolean', + }, + Number: { + message: 'Use number instead', + fixWith: 'number', + }, + Symbol: { + message: 'Use symbol instead', + fixWith: 'symbol', + }, + + Function: { + message: [ + 'The `Function` type accepts any function-like value.', + 'It provides no type safety when calling the function, which can be a common source of bugs.', + 'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.', + 'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.', + ].join('\n'), + }, + + // object typing + Object: { + message: [ + 'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + '{}': { + message: [ + '`{}` actually means "any non-nullish value".', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + object: false, + }, + }, + ], }, }; diff --git a/src/sections/api/town.dto.ts b/src/sections/api/town.dto.ts index 03ec90b2..15ca77d1 100644 --- a/src/sections/api/town.dto.ts +++ b/src/sections/api/town.dto.ts @@ -1,11 +1,5 @@ import { ApiProperty } from '@nestjs/swagger'; -import { - Exclude, - Expose, - plainToClass, - Transform, - Type, -} from 'class-transformer'; +import { Exclude, Expose, plainToClass, Type } from 'class-transformer'; import { IsInt, IsNotEmpty, IsNumber, Min } from 'class-validator'; import { StreamDto } from 'src/streams/api/stream.dto'; import { Town } from '../entities'; diff --git a/src/utils/ulid-constraint.ts b/src/utils/ulid-constraint.ts index b4a61141..f4b40f03 100644 --- a/src/utils/ulid-constraint.ts +++ b/src/utils/ulid-constraint.ts @@ -1,4 +1,4 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { registerDecorator, ValidationArguments, diff --git a/src/violations/api/violations-filters.dto.ts b/src/violations/api/violations-filters.dto.ts index d0bc6680..9c088fc5 100644 --- a/src/violations/api/violations-filters.dto.ts +++ b/src/violations/api/violations-filters.dto.ts @@ -1,9 +1,7 @@ import { Type } from 'class-transformer'; import { - IsBoolean, IsBooleanString, IsInt, - IsNumber, IsNumberString, IsOptional, Length,