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

Cria model forms dinâmicos para poder validar inputs de querystring #455

Merged
merged 16 commits into from
Oct 3, 2020

Conversation

berinhard
Copy link
Contributor

@berinhard berinhard commented Sep 29, 2020

Fixes #242
Fixes #109

@turicas esse PR ainda está como WIP e prova de conceito de que conseguimos usar os model forms dinâmicos para sanitizar esses input. Como esse caminho mostra que é possível, acho que vale uma refatoração grande em como a rota de detalhes de um dataset (da interface web, não da API) renderiza o form e manipula a querystring.

Acho que daria para construir esse form dinâmico fora desse método tão específico em que o coloquei agora. Poderíamos, por exemplo, construílo-na view e usá-lo no template do django para renderizar os inputs/valores selecionados. Eu fiz dessa forma bruta para conseguir validar no ponto mais próximo da filtragem sem me preocupar com muitas integrações. Queria alinhar contigo e ver se você acha que vale a pena seguir o caminho de uma refatoração grande no template.

Apesar do DRF não possui nenhuma API muito explícita como o modelform_factory podemos seguir a mesma estratégia por lá também. No DRF vai ser ainda mais fácil porque é simplesmente só levantar a exceção de validação do serializer que ele já dá conta de retornar a resposta 400 com os erros.

@turicas
Copy link
Owner

turicas commented Oct 1, 2020

Acho que o caminho é esse mesmo, mas não poderíamos colocar esse código em core/forms.py?

@berinhard
Copy link
Contributor Author

Show @turicas! Sim, vou mover isso pro form com certeza. Esse PR era só prova de conceito pra garantir que dava para criar o form.

@berinhard berinhard changed the title [WIP] Cria model forms dinâmicos para poder validar inputs de querystring Cria model forms dinâmicos para poder validar inputs de querystring Oct 1, 2020
@berinhard
Copy link
Contributor Author

@turicas agora esse PR tá liberado para review com a validação rolando tanto no frontend quanto na API. Dá uma assistida neste vídeo de demo e diz o que você acha das questões que trago no final dele.

@turicas
Copy link
Owner

turicas commented Oct 2, 2020

Alguns comentários gerais sobre o vídeo (se puder, cria issue pra endereçá-los? Acho que nem tudo é do escopo desse PR):

  • Faz total sentido colocarmos mais detalhes nos metadados da tabela na API, como os choices. Acho que podemos seguir um caminho onde vamos descrever melhor o schema que a tabela está atualmente utilizando, daí podemos ter vários schemas (para fazer o versionamento deles) - mas isso é algo que seria melhor discutirmos para depois definir as alterações que precisarão ser feitas.
  • Falando em API, esquecemos também de colocar a URL de download de arquivos de cada tabela depois que implementamos o /files/ (talvez já funcione por conta do download_url, mas acho legal checarmos).
  • Com relação aos testes, podemos testar uma instância do form dinâmico: a gente cria um Model com fields específicos durante o teste e inspeciona o formulário criado para ver se está de acordo; não é o ideal, mas pelo menos garantimos que tem algum teste passando pelo fluxo de criação do form/validação, que tal?
  • Agora que temos um Form do Django, talvez faça sentido usarmos o django-filter em vez do método apply_filtering -- eu lembro de ter tentado usá-lo (mas não testei muito) e por algum motivo não funcionou com os modelos dinâmicos para filtragem. Uma das coisas que não temos hoje (e que na minha visão é bastante ruim não termos) são os filtros por faixa (exemplo: datas e números de X a Y, em vez do valor exato X) e o django-filter talvez possa ajudar nisso.

core/views.py Outdated Show resolved Hide resolved
core/forms.py Show resolved Hide resolved
core/views.py Show resolved Hide resolved
api/views.py Outdated Show resolved Hide resolved
@berinhard
Copy link
Contributor Author

Faz total sentido colocarmos mais detalhes nos metadados da tabela na API, como os choices. Acho que podemos seguir um caminho onde vamos descrever melhor o schema que a tabela está atualmente utilizando, daí podemos ter vários schemas (para fazer o versionamento deles) - mas isso é algo que seria melhor discutirmos para depois definir as alterações que precisarão ser feitas.

Ao meu ver isso tem um pouco a ver com a issue #440 também, certo? Concordo em reestruturar os schemas para sermos mais versáteis no nosso sistema. Mas você acha que já não vale serializarmos esses dados hoje do jeito que a modelagem está e depois refatorarmos. Eu penso em algo como:

{
    "fields": [
        {
            "name": "date",
            "type": "string"
        },
        {
            "name": "state",
            "type": "string"
        },
        {
            "name": "url",
            "type": "string"
        },
        {
            "name": "notes",
            "type": "string"
        }
    ],
    "filters": {
        "date": {"type": "string", "choices": ["2020-10-01", "2020-09-30"]}
    },
    "name": "boletim",
    "data_url": "http://localhost:8000/api/dataset/covid19/boletim/data/",
    "import_date": "2020-09-27T13:22:41.225767-03:00"
}

Acho que, nesse PR atualizar o serializer do endpoint /api/datasets/covid19/ para apresentar essa informação e criar uma issue nova para criar o endpoint específico.

Falando em API, esquecemos também de colocar a URL de download de arquivos de cada tabela depois que implementamos o /files/ (talvez já funcione por conta do download_url, mas acho legal checarmos).

Putz, pode crer! Criei a issue #458

Com relação aos testes, podemos testar uma instância do form dinâmico: a gente cria um Model com fields específicos durante o teste e inspeciona o formulário criado para ver se está de acordo; não é o ideal, mas pelo menos garantimos que tem algum teste passando pelo fluxo de criação do form/validação, que tal?

Show! Vou atualizar o PR com isso então 👍

Agora que temos um Form do Django, talvez faça sentido usarmos o django-filter em vez do método apply_filtering -- eu lembro de ter tentado usá-lo (mas não testei muito) e por algum motivo não funcionou com os modelos dinâmicos para filtragem. Uma das coisas que não temos hoje (e que na minha visão é bastante ruim não termos) são os filtros por faixa (exemplo: datas e números de X a Y, em vez do valor exato X) e o django-filter talvez possa ajudar nisso.

Teria que olhar mais a fundo o django-filter para entender como combinar isso. Posso fazer isso depois de terminar de atualizar esse PR e, se for o caso, abrir uma nova issue.

@berinhard
Copy link
Contributor Author

@turicas testes adicionados =)

@turicas
Copy link
Owner

turicas commented Oct 2, 2020

  • Schema: talvez possamos usar o formato do datapackage diretamente (criei uma tarefa para discutirmos mais pra frente sobre).
  • django-filter: já existe a issue Criar filtros mais flexíveis (frontend e API) #110, comentei lá sobre
  • Talvez tenha faltado teste para o 4xx, não?

@turicas
Copy link
Owner

turicas commented Oct 3, 2020

Criei a issue #460 para endereçar o teste, vou fazer o merge.

@turicas turicas merged commit 760adce into develop Oct 3, 2020
@turicas turicas deleted the feature/validate-filter-data branch October 3, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants