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: datetime_range.unit #627

Merged
merged 4 commits into from
Aug 1, 2024
Merged

feat: datetime_range.unit #627

merged 4 commits into from
Aug 1, 2024

Conversation

rdahis
Copy link
Member

@rdahis rdahis commented Jul 3, 2024

Iterando o #621.

  • Criar campo datetime_rande.unit como FK para column.
  • No campo datetime_rande.unit, exibir só colunas da mesma tabela.
  • Criar property table.datetime_range_unit.
  • Rodar migrations.

@rdahis rdahis self-assigned this Jul 3, 2024
backend/apps/api/v1/models.py Outdated Show resolved Hide resolved
@@ -1747,6 +1756,10 @@ class DateTimeRange(BaseModel):
end_minute = models.IntegerField(blank=True, null=True)
end_second = models.IntegerField(blank=True, null=True)
interval = models.IntegerField(blank=True, null=True)
unit = models.ForeignKey(
Copy link
Member Author

Choose a reason for hiding this comment

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

@jhonylucas74 sabe como colocar um filtro aqui para só aparecer opções de colunas que estejam na tabela (ligada através de table.coverage.datetime_range)? Agora fica pesado esse campo no Django Admin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Não tem um jeito fácil nesse caso devido a complexidade das relações do modelo. Uma coisa que daria pra fazer é, se column possuir algum valor, categoria, tipo que vc queria filtrar é fácil de fazer usando o limit_choices_to. Mas se precisar verificar as relações entres as instâncias para trazer com base nas relações é mais complicado. Vai ser necessário um custom form no admin do django.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Posso tentar ver isso amanhã, testar aqui e se eu tiver sucesso eu mando eu mando as alterações.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jhonylucas74 acho que isso foi aplicado no campo de observation level que está no formulário de coluna do django se precisar ed uma referência. ele só puxa as opções de observation level que estão na mesma tabela
Screenshot from 2024-07-04 16-56-52

Copy link
Member Author

Choose a reason for hiding this comment

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

Aqui:

def formfield_for_foreignkey(self, db_field, request, **kwargs):

@laura-l-amaral
Copy link
Contributor

laura-l-amaral commented Jul 5, 2024

@rdahis percebi um problema importante com essa implementação que vc sugeriu:
temos mais de uma cobertura temporal para tabelas que são parcialmente BD Pro, como que faz nesses casos? acho que vc só tava imaginando um único datetime range para cada tabela né? Vai ficar com uma mesma coluna conectada a 2 datetime_range? Acaba que fica uma informação duplicada e atrapalha a consulta a api

Qual que era o problema da implementação que eu tinha sugerido?

@rdahis
Copy link
Member Author

rdahis commented Jul 6, 2024

@rdahis percebi um problema importante com essa implementação que vc sugeriu: temos mais de uma cobertura temporal para tabelas que são parcialmente BD Pro, como que faz nesses casos? acho que vc só tava imaginando um único datetime range para cada tabela né? Vai ficar com uma mesma coluna conectada a 2 datetime_range? Acaba que fica uma informação duplicada e atrapalha a consulta a api

Não é um problema não. Quando tiver múltiplas coverage, pode ficar a mesma coluna ligada à duas. A property table.datetime_range_unit está pegando a "coluna mais comum" entre datetime_ranges. Na prática é pra ser sempre a mesma, porque diferentes coberturas na mesma tabela deveriam ter todas a mesma unidade temporal.

@laura-l-amaral
Copy link
Contributor

laura-l-amaral commented Jul 8, 2024

@rdahis Ok entendi, podemos seguir assim. O único "problema" é que equipe dados vai ter que preencher igual nas duas e fica aberto para esquecer ou errar e preencher diferente. Ter que preencher 2 vezes a mesma informação não é uma redundância no nosso modelo? Pra lidar com o problema que eu coloquei ali eu incluo no código uma verificação se caso tiver 2 coberturas, se elas estão ambas preenchidas e se estão iguais.

@rdahis
Copy link
Member Author

rdahis commented Jul 12, 2024

É uma certa redundância porque acho que nunca teremos uma tabela com duas unidades temporais diferentes (ex: parte com ano e parte com mes). Mas é isso aí mesmo. Eu deixaria passar sem validação mas vou tentar implementar aqui.

Se for crítico para a feature de tradução de tabelas, vamos aprovar logo e adicionar a validação depois? @laura-l-amaral

@laura-l-amaral
Copy link
Contributor

@rdahis não é crítico não pra tradução, é pra gente melhorar o sistema de atualização de metadados só. Ficar mais automático e reduzir os casos que os metadados ficam diferentes dos dados

@rdahis
Copy link
Member Author

rdahis commented Jul 16, 2024

Adicionei a validação. Testei local via backend do Django e ele apresenta uma barra de erro quando eu tento adicionar uma segunda unit diferente da primeira. Vejam abaixo. Ele só mostra essa barra vermelha, mas não exibe a mensagem exatamente do erro, que é "Datetime range units do not refer all to the same column.".

Screenshot 2024-07-16 at 13 09 47

De qualquer jeito, seria bom @laura-l-amaral e @jhonylucas74 testarem local antes de aprovar.

@rdahis
Copy link
Member Author

rdahis commented Aug 1, 2024

Ainda está aberta essa questão de exibir o erro corretamente no Django. Mas vou dar merge para começarmos a preencher.

@rdahis rdahis merged commit 83a039b into main Aug 1, 2024
3 of 4 checks passed
@rdahis rdahis deleted the feat/datetime_range_unit branch August 1, 2024 03:41
@rdahis
Copy link
Member Author

rdahis commented Aug 1, 2024

Parece que o deploy não foi. Sabe onde vejo os logs? @jhonylucas74 @gabriel-milan

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

Successfully merging this pull request may close these issues.

3 participants