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

creating API to search the amount of doujins in database #2

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Boulkien
Copy link
Collaborator

No description provided.

Copy link
Owner

@AlexandreSenpai AlexandreSenpai left a comment

Choose a reason for hiding this comment

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

Por favor, efetue as alterações indicadas na pull request.
@Boulkien


// return the data from document (in this case, the amount of doujins in database)
if (!doujins.exists) {
return res.send("There's nothing here")
Copy link
Owner

Choose a reason for hiding this comment

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

Faltou a definição de status code de retorno em cada condicional.

  • Se doujin não existir: retornar 404
  • Se doujin existir: retornar 200

return res.send("There's nothing here")
} else {
return res.json({
"amount of doujins": doujins.data()
Copy link
Owner

Choose a reason for hiding this comment

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

chave muito change e não semântica, retorne chaves simples, pois o frontend precisará acessar. nunca use espaços em branco

incorreto: amount of doujins

resultado.amount of doujins // erro

correto: amountOfDoujins

resultado.amountOfDoujins // ok


const routes = (app) => {
app.route("/").get((req, res) => {
res.status(200).send({ título: "O Alexandre está me fazendo de escravo"})
Copy link
Owner

Choose a reason for hiding this comment

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

rota de healthcheck precisa ser
/healthcheck

res.status(200).send({ título: "O Alexandre está me fazendo de escravo"})
})

app.use(
Copy link
Owner

Choose a reason for hiding this comment

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

o correto no arquivo de rotas é importar do express Router

const { Router } = require("express");

const router = Router()

router.get(...)

module.exports = router;

Copy link
Owner

Choose a reason for hiding this comment

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

exclua todos os app.use dessa rota, eles devem estar definidos no arquivo do servidor.

Comment on lines 5 to 9

app.use(express.json());

routes(app);

Copy link
Owner

Choose a reason for hiding this comment

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

O uso correto da instauração das rotas no aplicativo seria:

const routes = require("./routes");
app.use(express.json({
   types: ["application/json"]
}));
app.use(routes);

backend/metadata/index.js Show resolved Hide resolved
const serviceAccount = require("../../eroneko-c890a77e5039.json")

initializeApp({
credential: cert(serviceAccount)
Copy link
Owner

Choose a reason for hiding this comment

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

isso só funcionará localmente, uma vez que não subiremos as credenciais quando implantarmos o código no GCP.

faça a verificação da existência do arquivo baseado no caminho provido, caso o arquivo não exista, utilize as credenciais padrões do sistema.

ApplicationDefault()

https://firebase.google.com/docs/admin/setup#initialize-sdk

@@ -1,10 +1,12 @@
const express = require("express");
const routes = require("./routes/index.js")
const routes = require("./routes/Routes.js")
Copy link
Owner

Choose a reason for hiding this comment

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

nome do arquivo em maiúsculo, alterar para routes.js.
importante seguir um padrão.

initializeApp({
credential: cert(serviceAccount)
})
applicationDefault()
Copy link
Owner

Choose a reason for hiding this comment

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

Não era para excluir a forma anterior, estava correta para o ambiente de desenvolvimento.
É necessária a criação de uma lógica para a utilização condicional de uma das duas dependendo do ambiente em que a aplicação está rodando.

Copy link
Owner

@AlexandreSenpai AlexandreSenpai left a comment

Choose a reason for hiding this comment

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

Fiz alguns comentários com poucas alterações. Uma vez que estiverem completas, podemos passar para a implantação e testar. Good Job 🤠

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

Successfully merging this pull request may close these issues.

2 participants