backend icon indicating copy to clipboard operation
backend copied to clipboard

feature/supply-priority-expiration

Open gustavocs opened this issue 9 months ago • 11 comments

Task: https://trello.com/c/Ihjztzs9/22-backend-ap%C3%B3s-4horas-trocar-a-prioridade-de-precisa-urgente-para-precisa

BLOCKED: Depende de #60.

gustavocs avatar May 09 '24 19:05 gustavocs

cumpre o requisito, apenas deve fazer a change sugerida em #20 (comment)

@filipepacheco @AndersonCRocha concordo, dava pra fazer com um updateMany. mas vou ter que dividir em duas queries de qualquer forma porque por alguma razão os campos createdAt e updatedAt no BD são varchar e não consigo usar os operadores de menor ou igual (lte). então faria essa lógica no próprio método. alguma objeção?

gustavocs avatar May 11 '24 01:05 gustavocs

cumpre o requisito, apenas deve fazer a change sugerida em #20 (comment)

@filipepacheco @AndersonCRocha concordo, dava pra fazer com um updateMany. mas vou ter que dividir em duas queries de qualquer forma porque por alguma razão os campos createdAt e updatedAt no BD são varchar e não consigo usar os operadores de menor ou igual (lte). então faria essa lógica no próprio método. alguma objeção?

Rapazz, eu acho q o momento de ajustar o campo no banco pro tipo correto seria agora q tá no início heim, não faz sentido ser varchar, @filipefraga sabe a motivação?

AndersonCRocha avatar May 11 '24 02:05 AndersonCRocha

ok, vou abrir uma issue pra migrar os campos de string pra timestamp.

@gustavocs o prisma não tem suporte a operadores de cast? o operador gt usando Date() não funcionaria com essa string de timestamp?

se não, vamos manter assim porque de fato vai seguir sendo 2 queries.

filipepacheco avatar May 11 '24 02:05 filipepacheco

ok, vou abrir uma issue pra migrar os campos de string pra timestamp.

@gustavocs o prisma não tem suporte a operadores de cast? o operador gt usando Date() não funcionaria com essa string de timestamp?

se não, vamos manter assim porque de fato vai seguir sendo 2 queries.

@filipepacheco Não consigo converter a Date() pra fazer a query porque os parâmetros da query do prisma são tipados. Tentei sem fazer o cast pra Date e usar lte e não executa conforme o esperado. Acho que foi justamente por isso que a primeira lógica tinha feito assim e não lembrava bem.

Sobre a migração: pelo que vi, não é só nesta tabela. Várias outras estão usando varchar pra createdAt e updatedAt.

gustavocs avatar May 11 '24 03:05 gustavocs

@filipepacheco Realmente o prisma n oferece o suporte no query builder pro cast, tá deixando a desejar demais, TB n oferece suporte pra inserir uma RAW Clause no meio do query builder, forçando converter toda a query para nativa com $queryRaw, ou então fazer alguma gambiarra. Que foi o caso do unnaccent do outro PR

AndersonCRocha avatar May 11 '24 03:05 AndersonCRocha

@AndersonCRocha sim, estamos descobrindo vários pontos negativos do Prisma...

Mas sem problema. Podemos subir assim caso essa entrega seja requisitada para deploy e depois da migração em todas as tabelas passamos por aqui pra arrumar.

Vou aprovar.

filipepacheco avatar May 11 '24 03:05 filipepacheco

@gustavocs é importante ressaltar que na sua implementação não está sendo 2 queries não, tá sendo 1 query + N updates em que N é a quantidade de registros q deverão ser alterados, se for continuar com essa abordagem ao invés de corrigir o tipo no banco de dados, sugiro vc fazer um filter() na lista pra pegar os expirados, depois um map() para obter os ida e por fim um único update com where IN

Edit 1: PutzZz, acabei de ver que ShelterSupply é chave composta, deu ruim, desisto!!!

AndersonCRocha avatar May 11 '24 03:05 AndersonCRocha

Antes de aprovar, vou deixar 3 sugestões

  1. Aumentar a periodicidade do CRON (10 segundos é muito pouco)
  2. Colocar a periodicidade no .env pra que seja de fácil ajuste sem a necessidade de deploy 2.1. Essa aqui tá sendo usado as constantes do Nest. Será que tem como usar o .env de maneira amigável nesse caso?
  3. Colocar o intervalo de tempo pra prioridade ser diminuida no .env também

Obrigado @gustavocs

filipepacheco avatar May 11 '24 03:05 filipepacheco

@gustavocs é importante ressaltar que na sua implementação não está sendo 2 queries não, tá sendo 1 query + N updates em que N é a quantidade de registros q deverão ser alterados, se for continuar com essa abordagem ao invés de corrigir o tipo no banco de dados, sugiro vc fazer um filter() na lista pra pegar os expirados, depois um map() para obter os ida e por fim um único update com where IN

Edit 1: PutzZz, acabei de ver que ShelterSupply é chave composta, deu ruim, desisto!!!

Sim, eu tava implementando um filter e percebi que não teria como fazer com whereIn. Tá difícil. Vou fazer as mudanças sugeridas pelo @filipepacheco amanhã cedo. Valeu pelo review <3

gustavocs avatar May 11 '24 03:05 gustavocs

Aguardando #60 para corrigir implementação.

gustavocs avatar May 11 '24 19:05 gustavocs

por gentileza corrige o erro de lint

Corrigido. O erro de lte vai vai seguir até os campos serem migrados pra timestampz. Já fiz o teste local e funciona. Esse PR tá dependendo do #60, vou adicionar na descrição.

gustavocs avatar May 12 '24 00:05 gustavocs