Conversation
|
DTO/migracje:
Inne:
Do photoservice - narazie wykorzystajmy tego multera z nest.js: https://docs.nestjs.com/techniques/file-upload z podstawowym zapisywaniem do lokalnego folderu https://docs.nestjs.com/recipes/serve-static. Kiedyś pewnie przeniesiemy to na S3 czy tam minio wtedy front będzie robił upload a na backend wyśle tylko url do zdjęcia To co dawid napisał z tym |
|
@loudsheep Pisałem do Antka mówił, że to 1:N spoko wsm, narazie nie wyrzucałem tych arrayów stąd póki nie ma modułu do tego (I guess osobny moduł by się tu przydał?) No i testy jeszcze :p |
|
do walidacji pliku chyba narazie tylko size potrzebujemy to można dodać jakiś prosty cutomowy validator, tak jak tutaj to robią: |
|
@loudsheep wleciała walidacja plików, i osobna tabela dla linków i te powyższe pierdółki, więc z bazowych rzeczy wszystko jest co się dało na ten moment (dla bazowych endpointów, nie dodawałem jeszcze rzeczy typu endpoint do weryfikacji). Tych testów e2e tbh nie ogarniam zbytnio, a nie jestem pewien czy na jedynym innym branchu jest poprawnie zrobione żeby sobie podejrzeć |
loudsheep
left a comment
loudsheep
left a comment
There was a problem hiding this comment.
napisałem tam pare komentarzy, i jescze żeby nie pisać kilka razy to napiszę tutaj (w jednym miejscu): w kontrolerze zminił bym wszystkie wystąpienia eventUUID na eventID. Wydaje mi się że zostawnienie tylko ID zamiast UUID w tych nazwach chyba wystarczy bo frontend będzie wiedział że chodzi o uuid, a nie ujawniamy szczegółów implementacji
| forms Form[] @relation("EventForms") | ||
| adminPermissions AdminPermission[] | ||
| participants Participant[] | ||
| linksData EventLink[] |
There was a problem hiding this comment.
to bym raczej poprostu nazwał links - powinno być i tak zrozumiałe co to przechowuje
| @ApiPropertyOptional({ | ||
| description: "Photo URL of the event", | ||
| type: String, | ||
| example: "https://event-photo.com/photo.jpg", | ||
| }) | ||
| @IsOptional() | ||
| @IsString() | ||
| photoUrl?: string | null; |
There was a problem hiding this comment.
photoUrl by wywalił z tego bo jest imo trochę misleading. jeżeli user prześle i zdjęcie (plik) i url jako string to te url zostanie kompletnie zignoreowane. Chyba poprostu najlepiej zostawić tylko ten przesłany plik a na przesłanie photoUrl nawet nie pozwalać
| export class EventCreateDto { | ||
| @ApiProperty({ | ||
| description: "Name of the event", | ||
| type: String, | ||
| example: "Tech Conference 2023", | ||
| }) | ||
| @IsString() | ||
| readonly name: string; | ||
| } |
There was a problem hiding this comment.
tu chyba przy kopiowaniu mógła przypadkowo zostać definicja (ta sama co jest w innym pliku)
| @Get(":eventId") | ||
| @ApiOperation({ summary: "Get event by ID" }) | ||
| @Get("public") | ||
| @ApiOperation({ | ||
| summary: "Get list of public events with pagination and filtering", | ||
| }) | ||
| @ApiOkResponse({ description: "List of public events", type: PageDto<Event> }) | ||
| async findAllPublic(@Query() dto: EventListingDto): Promise<PageDto<Event>> { | ||
| return this.eventsService.findAllPublic(dto); | ||
| } |
There was a problem hiding this comment.
tak sie wsumie teraz zastanawiam, czy nie lepiej stworzyć nowy kontroller (w tym samym module) nazwać go np: PublicEventsController i tam poprostu zdefiniować te ścieżki dla tych eventów publicznych
| @UseInterceptors( | ||
| FileInterceptor("photo", { | ||
| storage: diskStorage({ | ||
| destination: (_request, _file, callback) => { | ||
| const uploadPath = "./uploads/events"; | ||
| if (!existsSync(uploadPath)) { | ||
| mkdirSync(uploadPath, { recursive: true }); | ||
| } | ||
| callback(null, uploadPath); | ||
| }, | ||
| filename: (_request, file, callback) => { | ||
| const now = Date.now().toString(); | ||
| const random = Math.round(Math.random() * 1e9).toString(); | ||
| const uniqueSuffix = `${now}-${random}`; | ||
| const extension = extname(file.originalname); | ||
| callback(null, `${uniqueSuffix}${extension}`); | ||
| }, | ||
| }), | ||
| limits: { fileSize: 10 * 1024 * 1024 }, | ||
| fileFilter: (_request, file, callback) => { | ||
| const allowedTypes = ["image/png", "image/jpeg", "image/jpg"]; | ||
| if (allowedTypes.includes(file.mimetype)) { | ||
| callback(null, true); | ||
| } else { | ||
| callback( | ||
| new BadRequestException( | ||
| "Invalid file type. Only PNG and JPEG are allowed.", | ||
| ), | ||
| false, | ||
| ); | ||
| } | ||
| }, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Widzę że i w create i w update w zasadzie jest ta sama logika. można ją wyciągnąć to jakiegoś wspólnego pliku konfiguracyjnego, albo jakiś customowy dekorator tylko dla tych plików zrobić.
Ale generalnie fajnie są te pliki obsugiwane
| if (photo !== undefined) { | ||
| photoUrl = `/uploads/events/${photo.filename}`; | ||
| } | ||
| eventDto.photoUrl = photoUrl; |
There was a problem hiding this comment.
DTO generalnie nie powinno się edytować po tym jak user nam je wyśle. więc the photoUrl bym przekazał jako drugi argument funkcji w service
this.eventsService.create(eventDto, photoUrl);
| return this.eventsService.findOnePublic(eventUUID); | ||
| } | ||
|
|
||
| @Put(":eventUUID") |
There was a problem hiding this comment.
zmienił bym na @Patch(":eventUUID"), głównie przez to że inne controllery kożystają z patch a nie put.
Ale jak dobrze kojarzę to chyba standard REST api coś o tym mówi że PUT ma podmienić cały obiekt, a PATCH może wysłać tylko część danych i je zmienić.
Generalnie tych standarów i wytycznych restful jest tak dużo xdddd, masakra, nie da się tego oarnąć wszystkiego
| @Get("public/:eventUUID") | ||
| @ApiOperation({ summary: "Get public event by UUID" }) | ||
| @ApiOkResponse({ description: "The public event", type: Event }) | ||
| async findOnePublic( | ||
| @Param("eventUUID", ParseUUIDPipe) eventUUID: string, | ||
| ): Promise<Event> { | ||
| return this.eventsService.findOnePublic(eventUUID); | ||
| } |
There was a problem hiding this comment.
Jak dobrze rozumiem to tutaj chyba powinno być wszędzie slug zamiast eventUUID bo service oczekuje w tej metodzie sluga. Jeżeli tutaj faktycznie jest po slugu, to pamiętaj też żeby usunąć ParseUUIDPipe z parametru żeby zwykły string też przechodził
| DATABASE_URL="prisma+postgres://" | ||
| PORT=3000 No newline at end of file |
There was a problem hiding this comment.
tego bym nie usuwał - przydaje się jak się konfiguruje repo lokalnie :)
| const same_slug_event = await this.prisma.event.findFirst({ | ||
| where: { slug: eventDto.slug, uuid: { not: uuid } }, | ||
| }); | ||
|
|
||
| if (same_slug_event !== null) { | ||
| throw new ConflictException( | ||
| `Event with slug ${same_slug_event.slug} already exists`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
to warto otoczyć jakimś if eventDro.slug czy coś w tym stylu, bo jak testowałem lokalnie to był taki błąd:
jak wysyłałem request PUT /events/:eventID i nie podałem w body slug to był on jako undefined i warunek where: { slug: eventDto.slug, uuid: { not: uuid } }, zmieniał się efektywnie w where: { uuid: { not: uuid } }, co oznaczało że w każdy inny event go spełniał i wyskakiwał błąd że slug istnieje.
2 participants
Notatki do Events Module:
DTO/migracje:
Participants Limit w v2 nie istniało, ale przyda się to zostawić, żeby wyznaczać defaultowe participantsLeft w formach (jak jest kilka formularzy),
Slug chyba powinien być required
Jest validated_at pytanie czy chcemy mieć pole is_validated czy po prostu sprawdzać czy data nie jest nullem
Czy links/policy links nie lepiej rozbić na relacje 1:N? (W chuj mnie teraz normalizacjami męczyli na PBD xd + rzeczy typu update dla pojedynczych linków będzie chyba dziwnie działać, chyba że front będzie wklejał w request to co jest + zmiany (???))
Do wyświetlania public-eventów: czy verified_at == event jest publiczny? (Chyba nie więc brakuje pola is_public czy coś takiego)
Inne:
Czy do Not Found Exception wrzucamy message, o konkretnym UUID? (imo do debugowania spoko)
Czy w niezweryfikowanym Evencie dajemy informacje, że istnieje, ale nie jest zweryfikowany/publiczny, czy po prostu notFound (imo pierwsza opcja przydatna do debugowania, ale mniej bezpieczna)
Jeżeli nie dzielimy na internal/external to jaki prefix? (Dałem public narazie)
W requescie podajemy “normalną” datę, ale zwraca nam obiekt datę (ten co ma “T” i “Z”), czy nie chcemy tego serializować, żeby zwracało taki sam format jak się podaje,
TODO (jest pokomentowane w kodzie):
Auth required
Inne
Dokumentacja wygląda okej, jak patrzyłem, troszkę posiedziałem, żeby ładnie to wyglądało (może jakieś response DTO można by dorobić).
Kody statusu do tego co jest zrobione też git.
Routing zależy jak chcemy, ale coś tam zaproponowałem.
Wiadomo, że to jeszcze nie do merge'a, chciałem zebrać co mi nie pasuje, żeby dokończyć.