feat: dados abertos senado - servidores #546

vitorialira92 · 2023-11-30T09:09:01Z

No description provided.

vercel · 2023-11-30T09:09:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
brasilapi	❌ Failed (Inspect)			Dec 1, 2023 1:45am

sonarcloud · 2023-12-01T01:45:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

alcidesmig · 2023-12-01T17:31:27Z

pages/api/senado/v1/servidores/pensionistas/[ano]/[mes].js

+import BadRequestError from '@/errors/BadRequestError';
+import { getRemuneracoesPensionistas } from '@/services/dados-publicos-senado/servidores/index';
+
+function isYearValid(year){


Essas funções podem ser úteis para mais alguém, acho que vale a pena colocar em uma pasta/arquivo de "utils". O que acham?

Eu utilizaria a services e criaria um subdir validators para colocar elas.

Porém acredito que elas estão repetidas no código, o primeiro arquivo do commit já é um service de validação para estas informações.

alcidesmig · 2023-12-01T17:32:05Z

pages/api/senado/v1/servidores/pensionistas/index.js

+import BadRequestError from '@/errors/BadRequestError';
+import { getPensioners } from '@/services/dados-publicos-senado/servidores/index';
+
+async function getAllPensioners(request, response) {


Aqui tem um nome em inglês, mas em outros lugares está em português. Acho que vale seguir o padrão do repositório!

Bem observado, apesar de não termos um padrão com o martelo batido ainda, temos um modelo em desenvolvimento que pode ser um guia: https://github.com/BrasilAPI/BrasilAPI/blob/6509fa5e96ed4544b193d6a56a5cfc1b48c29bf5/GOOD_PRACTICES.md

alcidesmig · 2023-12-01T17:43:14Z

pages/api/senado/v1/servidores/ativos.js

+        const result = await getServidoresAtivos();
+
+        response.status(200);
+        return response.json(result);


Uma coisa que percebi foi que vocês recebem os dados da API do senado e mandam direto para quem está chamando, se vocês observarem em outras partes do repositório eles fazem um mapeamento dos campos da resposta da API para campos próprios - isso daria algumas vantagens a implementação de vocês, fazendo com que o padrão (e os campos) da resposta pudesse ser controlado pela própria BrasilAPI, dando mais visibilidade no código dos campos respondidos, e permitindo contornar eventuais comportamentos estranhos ou quebras de contrato da API que vocês usaram.

RodriAndreotti

No geral o PR ficou muito bom, quero agradecer a contribuição e parabenizar pelo código, pois é uma integração grande e sei que deu bastante trabalho para criar.

Se me permitirem eu gostaria de sugerir algumas alterações:

1- Nomes: Notei que tanto no código quanto nos arquivos há uma mistura de nomenclaturas em inglês e português (por ex.: getAllInativos), é sempre recomendável manter uma padronização.

Apesar de não termos uma recomendação oficial ainda para as nomenclaturas temos um documento que estamos trabalhando para caminhar a isso, e acredito que seja uma boa tentar dar uma analisada e ver se faz sentido adequar:
https://github.com/BrasilAPI/BrasilAPI/blob/6509fa5e96ed4544b193d6a56a5cfc1b48c29bf5/GOOD_PRACTICES.md

2- Verificar repetição de código: Se um código é usado em mais de um lugar sem alteração talvez seja interessante extraí-lo para um arquivo externo.

3- Indentação do código: apesar de não ser block é sempre uma boa prática manter o código bem indentado, notei que alguns arquivos aqui a indentação poderia receber alguma atenção, apesar de não estarem ruins.

Novamente obrigado pela contribuição e seria muito legal para a gente se quiserem contribuir com outras implementações ;-)

RodriAndreotti · 2023-12-02T00:43:55Z

pages/api/senado/v1/servidores/ano-mes-verificacao.js

+  const currentMonth = new Date().getMonth();
+  month = parseInt(month, 10); 
+
+  return !isNaN(month) || month < currentMonth || month >= 1;


Acredito que tenha dois problemas de lógica nesta validação:
1- se alguém informar o mês 12 de 2015 será retornado o mês como inválido, pois é verificado se o mês é menor que o atual.
2- Acho que é interessante validar também se o mês está dentro do range de um ano (1 - 12), caso contrário teremos um erro quando corrigir o ponto 1

RodriAndreotti · 2023-12-02T00:46:52Z

pages/api/senado/v1/servidores/inativos.js

+        console.log(error.message);
+
+        if (error.response.status === 400) {
+          throw new BadRequestError({ message: 'Bad Request.' });


Se houve um bad request, não seria mais interessante informar ao client qual o motivo desse bad request?

RodriAndreotti · 2023-12-02T00:47:11Z

pages/api/senado/v1/servidores/lotacoes.js

+        console.log(error.message);
+
+        if (error.response.status === 400) {
+          throw new BadRequestError({ message: 'Bad Request.' });


Idem ao anterior

RodriAndreotti · 2023-12-02T00:49:14Z

pages/api/senado/v1/servidores/pensionistas/[ano]/[mes].js

+import BadRequestError from '@/errors/BadRequestError';
+import { getRemuneracoesPensionistas } from '@/services/dados-publicos-senado/servidores/index';
+
+function isYearValid(year){


Eu utilizaria a services e criaria um subdir validators para colocar elas.

Porém acredito que elas estão repetidas no código, o primeiro arquivo do commit já é um service de validação para estas informações.

RodriAndreotti · 2023-12-02T00:51:35Z

pages/api/senado/v1/servidores/pensionistas/index.js

+import BadRequestError from '@/errors/BadRequestError';
+import { getPensioners } from '@/services/dados-publicos-senado/servidores/index';
+
+async function getAllPensioners(request, response) {


Bem observado, apesar de não termos um padrão com o martelo batido ainda, temos um modelo em desenvolvimento que pode ser um guia: https://github.com/BrasilAPI/BrasilAPI/blob/6509fa5e96ed4544b193d6a56a5cfc1b48c29bf5/GOOD_PRACTICES.md

RodriAndreotti · 2023-12-02T00:59:26Z

services/dados-publicos-senado/servidores/index.js

+  var params = "";
+
+  if(tipoVinculoEquals != null)
+    params = `tipoVinculoEquals=${tipoVinculoEquals}`;
+
+  if(situacaoEquals != null)
+    params += (params == "") ? `tipoVinculoEquals=${situacaoEquals}` : `&tipoVinculoEquals=${situacaoEquals}`;
+
+  if(lotacaoEquals != null)
+    params += (params == "") ? `tipoVinculoEquals=${lotacaoEquals}` : `&tipoVinculoEquals=${lotacaoEquals}`;
+
+  if(cargoEquals != null)
+    params += (params == "") ? `tipoVinculoEquals=${lotacaoEquals}` : `&tipoVinculoEquals=${lotacaoEquals}`
+
+  if(params != "")
+    params = "?" + params;


Será que aqui não seria interessante o uso de URLSearchParams?
Acredito que o código se tornaria um pouco mais legível

RodriAndreotti · 2023-12-02T01:04:09Z

tests/senado-servidores-v1.test.js

Senti falta de testes para validar a algumas rotas aqui, por exemplo remunerações, horas extras etc.
Idealmente, para cada rota seria interessante termos ao menos um teste (melhor se for mais de um) para garantir que alguma alteração futura não a quebre.

RodriAndreotti · 2023-12-02T01:07:26Z

pages/docs/doc/senado.json

+                  "application/json": {
+                    "schema": {
+                        "type": "array",
+                         "content": {


Apesar de não ser um block, poderia ficar um pouco mais fácil para leitura se os schemas fossem declarados separadamente e só referenciados nos responses.
Isso, inclusive, facilita o reuso de um schema que por ventura seja usado por duas ou mais rotas.

Um exemplo: https://github.com/BrasilAPI/BrasilAPI/blob/main/pages/docs/doc/cptec.json

RodriAndreotti · 2023-12-02T01:09:07Z

pages/docs/doc/senado.json

+                  }
+                }
+              },
+              "400": {


Aqui nos possíveis erros recomendo o uso dos schemas que já temos:
https://github.com/BrasilAPI/BrasilAPI/blob/main/pages/docs/doc/error.json

RodriAndreotti · 2023-12-02T01:09:31Z

pages/docs/doc/senado.json

+                  }
+                },
+                "400": {
+                  "description": "Error",


Mesmo caso dos erros anteriores

LorhanSohaky

Muito obrigado pela contribuição! Além de tudo que comentei, peço que adicione testes para todas as rotas, pois só assim detectaremos caso alguma delas comece a dar problema

LorhanSohaky · 2023-12-02T01:20:42Z

pages/api/senado/v1/servidores/ano-mes-verificacao.js

+    const currentYear = new Date().getFullYear();
+    const yearN = parseInt(year, 10);   
+
+    return !isNaN(yearN) && year <= currentYear && yearN >= 2012;


Sugiro colocar o 2012 em uma constante para não ficar o número mágico no meio do código e se possível uma explicação do motivo para ser 2012. Por exemplo

const SENADO_MIN_DATE = 2012 // data mínima dos dados disponibilizados pela API do senado <link doc oficial>

LorhanSohaky · 2023-12-02T01:22:51Z

pages/api/senado/v1/servidores/ano-mes-verificacao.js

+function isMonthValid(month){
+  const currentMonth = new Date().getMonth();
+  month = parseInt(month, 10); 
+
+  return !isNaN(month) || month < currentMonth || month >= 1;
+}


Se entendi corretamente, essa função não funciona bem para datas passadas. Por exemplo, se hoje fosse 01/2023 e eu estivesse tentando acessar dados de 11/2022 daria como inválida

LorhanSohaky · 2023-12-02T01:24:11Z

pages/api/senado/v1/servidores/ano-mes-verificacao.js

Essas funções deveriam estar em outra pasta, pois tudo que está em pages/api é transformado em rotas de APIs

LorhanSohaky · 2023-12-02T01:26:51Z

pages/api/senado/v1/servidores/ativos.js

+
+        console.log(error.message);
+
+        if (error.response.status === 400) {
+          throw new BadRequestError({ message: 'Bad Request.' });
+        }
+
+        throw error;
+      }


Sugiro trocar todos os catch para algo mais ou menos assim:

Suggested change

console.log(error.message);

if (error.response.status === 400) {

throw new BadRequestError({ message: 'Bad Request.' });

}

throw error;

}

if (err instanceof BaseError) {

throw err;

}

throw new InternalError({

message: 'Erro ao buscar informações dos servidores ativos',

});

LorhanSohaky · 2023-12-02T01:29:59Z

pages/api/senado/v1/servidores/cargos.js

+
+        console.log(error.message);
+
+        if (error.response.status === 400) {
+          throw new BadRequestError({ message: 'Bad Request.' });
+        }
+
+        throw error;


mesmo sobre o catch

LorhanSohaky · 2023-12-02T02:04:01Z

pages/api/senado/v1/servidores/servidores.js

+function checkSituacao(situacao){
+  const possibleValues = ['APOSENTADO', 'INATIVO', 'ATIVO', 'DESLIGADO'];
+
+  return situacao == null || possibleValues.includes(situacao); 


Suggested change

return situacao == null || possibleValues.includes(situacao);

return situacao == null || possibleValues.includes(situacao.toUpperCase());

LorhanSohaky · 2023-12-02T02:04:38Z

pages/api/senado/v1/servidores/servidores.js

+
+      console.log(error.message);
+
+      if (error.response.status === 400) {
+        throw new BadRequestError({ message: 'Bad Request.' });
+      }
+
+      throw error;
+    }


mesmo sobre o catch

LorhanSohaky · 2023-12-02T02:09:16Z

services/dados-publicos-senado/servidores/index.js

+  if(params != "")
+    params = "?" + params;
+
+  return axios.get(API_URL +'/servidores' + params);


Sugiro fazer algo mais ou menos assim:

Suggested change

return axios.get(API_URL +'/servidores' + params);

return axios.get(API_URL +'/servidores', {

params: {

tipoVinculoEquals,

situacaoEquals,

lotacaoEquals,

cargoEquals,

}

});

https://masteringjs.io/tutorials/axios/get-query-params

LorhanSohaky · 2023-12-02T02:11:20Z

services/dados-publicos-senado/servidores/index.js

+
+export const getPensioners = async () => {
+  const endpoint = API_URL + '/pensionistas';
+   return axios.get(endpoint);


Suggested change

return axios.get(endpoint);

const {data} = await axios.get(endpoint);

return data

LorhanSohaky · 2023-12-02T02:14:08Z

tests/senado-servidores-v1.test.js

+            expect(response.data).toMatchObject({
+                sequencial: 3502872,
+                nome: "EVERTOM ALMEIDA DA SILVA",
+                vinculo: "EXERCICIO_PROVISORIO",
+                situacao: "DESLIGADO",
+                cargo: null,
+                padrao: null,
+                especialidade: null,
+                funcao: null,
+                lotacao:{
+                    sigla: "SEPCOM",
+                    nome: "Serviço de Cadastro Parlam. e Pessoal Comissionado"
+                },
+                categoria:{
+                    codigo: "EXERCÍCIO PROVISÓRIO",
+                    nome: "EXERCÍCIO PROVISÓRIO"
+                },
+                cedido:{
+                    tipo_cessao: "para SF",
+                    orgao_origem: "Instituto Federal de Educação, Ciência e Tecnologia de MT",
+                    orgao_destino: null
+                },
+                ano_admissao: 2020
+                },
+                {
+                sequencial: 3502775,
+                nome: "EVERTOM ALMEIDA DA SILVA",
+                vinculo: "EXERCICIO_PROVISORIO",
+                situacao: "DESLIGADO",
+                cargo: null,
+                padrao: null,


Sugiro deixar o teste mais genérico, validando apenas os tipos da resposta. Já tivemos problemas no passada validando o valor da resposta

feat: dados abertos senado - servidores

adaa17d

vercel bot had a problem deploying to Preview November 30, 2023 09:09 Failure

Merge branch 'main' into feat/bytegirls

32cccf0

vercel bot had a problem deploying to Preview December 1, 2023 01:45 Failure

LorhanSohaky requested review from lucianopf, RodriAndreotti, LorhanSohaky and murilohns December 1, 2023 01:45

alcidesmig reviewed Dec 1, 2023

View reviewed changes

RodriAndreotti requested changes Dec 2, 2023

View reviewed changes

LorhanSohaky requested changes Dec 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dados abertos senado - servidores #546

feat: dados abertos senado - servidores #546

vitorialira92 commented Nov 30, 2023

vercel bot commented Nov 30, 2023 •

edited

sonarcloud bot commented Dec 1, 2023

alcidesmig Dec 1, 2023

RodriAndreotti Dec 2, 2023

alcidesmig Dec 1, 2023

RodriAndreotti Dec 2, 2023

alcidesmig Dec 1, 2023

RodriAndreotti left a comment

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

RodriAndreotti Dec 2, 2023

LorhanSohaky left a comment

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

LorhanSohaky Dec 2, 2023

	return situacao == null \|\| possibleValues.includes(situacao);
	return situacao == null \|\| possibleValues.includes(situacao.toUpperCase());

-  return axios.get(API_URL +'/servidores' + params);
+  return axios.get(API_URL +'/servidores', {
+    params: {
+      tipoVinculoEquals,
+      situacaoEquals,
+      lotacaoEquals,
+      cargoEquals,
+    }
+  });

-   return axios.get(endpoint);
+   const {data} = await axios.get(endpoint);
+   return data

feat: dados abertos senado - servidores #546

Are you sure you want to change the base?

feat: dados abertos senado - servidores #546

Conversation

vitorialira92 commented Nov 30, 2023

vercel bot commented Nov 30, 2023 • edited

sonarcloud bot commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RodriAndreotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorhanSohaky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 30, 2023 •

edited