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

[SRT-20]: add basic models and CRUD #13

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Jus1x-by
Copy link
Contributor

@Jus1x-by Jus1x-by commented Jun 17, 2023

Что сделано:

  • Добавлены ДТО Profile
  • CRUD Profile
  • Миграция для создания NeighbourProfile

Информация для тестирования

Тестирование бекенда | Тестирование фронтенда

  • [ ]
  • [ ]
  • [ ]

Жду изменений в SRT-6, так как завязан и на них

@notion-workspace
Copy link

@sintell
Copy link
Contributor

sintell commented Jun 18, 2023

А поребейзись на main пожалуйста, а то я туда немного накоммитил, у нас пересеклось. И давай пока не будем удалять тестовую миграцию, я ее в отдельной задаче удалю вместе с тестовым сервисом и прочими штуками

@Jus1x-by
Copy link
Contributor Author

rebased

@sintell sintell force-pushed the SRT-20_neighbour_prof branch from 3f3795c to 65c0a0a Compare June 19, 2023 13:58
@@ -0,0 +1,24 @@
import { Expose } from 'class-transformer';
Copy link
Contributor

Choose a reason for hiding this comment

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

А поубирай лишние импорты, пожалуйста

return res;
}

@MustBe(UserRole.user)
Copy link
Contributor

Choose a reason for hiding this comment

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

Удали этот декоратор, пожалуйста, я добавлю в своей заадче

Copy link
Contributor

Choose a reason for hiding this comment

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

И везде ниже тоже

return (
await this.neighbourProfileRepository.update(
{ id: existingProfile.id },
{ ...body, country, city },
Copy link
Contributor

Choose a reason for hiding this comment

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

А так точно работает с county и city? У тебя же в профиле просто id должны быть, а связь one-to-one, можно просто проверить, что страна и город существуют и не апдейтить их совсем

@Column({ default: 'now()', select: false })
createdAt: Date;

@Column({ default: 'now()', select: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

Я тут обнаружил, что мы неправильно создавали такие колонки, надо было вот так: https://github.com/typeorm/typeorm/blob/master/docs/decorator-reference.md#updatedatecolumn

},
});

let country = existingProfile.country;
Copy link
Contributor

Choose a reason for hiding this comment

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

тут стоит логику сделать такой:
если указана страна, то город должен быть null, а если указан город, то страна должна быть null
иначе получится так, что кто-то может прислать город и страну, причем город может быть вне этой страны

Copy link
Contributor

Choose a reason for hiding this comment

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

Стоит сделать вот так: https://stackoverflow.com/a/64881956/1576604 и сделать обе колонки nullable в базе

return (await this.neighbourProfileRepository.delete({ user })).affected;
}

async getNeighbourProfile(id: number): Promise<NeighbourProfile | never> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Может тут должен быть метод для получения профиля по id пользователя? отдельный id профиля же никому особо не нужен, причем я бы подумал о том, чтобы сделать dto сразу с данными о User и Profile, чтобы фронт мог одним зарпосом получить данные

@OneToOne(() => Area)
@JoinColumn()
@ValidateIf((u) => u.city)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ValidateIf((u) => !u.city)

Проверяем, что страна не нулл в том случае, когда город не указан

Copy link
Contributor

Choose a reason for hiding this comment

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

Типа у нас должна быть указана или страна или город, обязательно что-то одно

@OneToOne(() => Area)
@ValidateIf((u) => u.country)
Copy link
Contributor

Choose a reason for hiding this comment

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

А город нужно проверять, если страна не указана

Copy link
Contributor Author

Choose a reason for hiding this comment

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

вроде понял что хочешь

Comment on lines 45 to 47
async getNeighbourProfile(@Param('id') id: number) {
return await this.neighbourProfileService.getNeighbourProfile(id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Забыл в контроллере переделать, тут дожно быть что-то типа:

Suggested change
async getNeighbourProfile(@Param('id') id: number) {
return await this.neighbourProfileService.getNeighbourProfile(id);
}
async getCurrentUserProfile(@Param('id') userId: number) {
return await this.neighbourProfileService. getNeighbourProfileByUserId(userId);
}

throw new NeighbourProfileAlreadyExists();
}

const country = await this.areaService.findOne(countryId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут и в обновлении нужно поменять логику:

  • если указан город, то достаем его по cityId, и берем, который пользователь передал, а countryId подставляем из parentId города, чтобы город и страна всегда совпадали
  • если город не указан, то проверяем, что указана страна и она существует (если не существует, то нужно кинуть ошибку), cityId указываем null, а countryId ту, что пользователь передал

@OneToOne(() => Area)
@JoinColumn()
@ValidateIf((u) => u.city)
Copy link
Contributor

Choose a reason for hiding this comment

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

Типа у нас должна быть указана или страна или город, обязательно что-то одно

@@ -4,9 +4,9 @@ export class TestMigration1684945119367 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
queryRunner.query(
`CREATE TABLE IF NOT EXISTS
app (id serial PRIMARY KEY, "lastRequestedAt" TIMESTAMP NOT NULL)`,
app (id serial PRIMARY KEY, lastRequestedAt TIMESTAMP NOT NULL)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Изменение в этом файле нужно откатить и вернуть кавычки, иначе запрос будет неправльно работать

@MapErrorToHTTP(NeighbourProfileAlreadyExists, UnauthorizedException)
async createNeighbourProfile(
@Body() createNeighbourProfile: CreateNeighbourProfileInput,
@Req() { user }: Request,
Copy link
Contributor

Choose a reason for hiding this comment

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

у тебя же и пользователя пока нет, передай просто id, как будто его нужно явно указать в запросе, я переделаю, иначе у тебя код не собирается, тут tscheck ругается

Copy link
Contributor

Choose a reason for hiding this comment

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

и ниже, везде где user

@sintell
Copy link
Contributor

sintell commented Jun 26, 2023

@Jus1x-by, чот код не работает)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants