-
Notifications
You must be signed in to change notification settings - Fork 11
Assignment week 2 Konjit #21
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work,
You've made some good enough use case as required in the assignment requirements, with your clean structure you have covered key test scenarios.
you did great in error handling as well, I like your testing cases, nice one.
LGTM; keep it up. 🎉
"scripts": { | ||
"start": "nodemon server.js", | ||
"dev": "node --watch server", | ||
"test": "jest" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in production it can be different, but good thinking. 💯
@@ -0,0 +1,2 @@ | |||
export const API_KEY = 'cb13636c87c0e5ab5d6f74d5dec66670'; | |||
export const WRONG_API_KEY = 'cb13636c87c0e5ab5d6f74d5decf6d6c670'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
it("should return 401", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongApiKey(cityName); | ||
await request.post("/weather").send({ cityName, wrongUrl }).expect(401); | ||
}); | ||
|
||
it("should return message 'Unauthorized access.'", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongApiKey(cityName); | ||
|
||
await request | ||
.post("/weather") | ||
.send({ cityName, wrongUrl }) | ||
.expect((res) => { | ||
expect(res.body).toHaveProperty("message", "Unauthorized access."); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I get it now 🥇
getWeatherUrlWithWrongApiKey
const fetchData = async (url) => { | ||
try { | ||
const response = await fetch(url); | ||
|
||
if (!response.ok) { | ||
const errorData = await response.json(); | ||
return errorData; | ||
} | ||
|
||
const data = await response.json(); | ||
return data; | ||
|
||
} catch (error) { | ||
throw new Error(error); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💐
if(data.cod === "404" && data.message === "city not found"){ | ||
return res.status(404).send({weatherText: "City is not found!"}) | ||
} | ||
|
||
if(data.cod === 401 && data.message.includes("API")){ | ||
return res.status(401).send({message: "Unauthorized access."}) | ||
} | ||
|
||
if(data.cod === "404" && data.message === "Internal error"){ | ||
return res.status(404).send({message: "Resource not found."}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love the early returns.
if(data.cod === "404" && data.message === "Internal error"){ | ||
return res.status(404).send({message: "Resource not found."}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting message, really can't answer for such, use case unless I see it in practice. 🤔
it however sounds closer to a server error to me as the user has nothing to do with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. The internal error suggests something is wrong with the server. But, I was confused because I intentionally made a mistake in here: https://api.openweathermap.org/data/2.5/weat
. I used weat
instead of weather
and I got this message when I tested it in a browser.
{
"cod": "404",
"message": "Internal error"
}
The code 404 indicates url problem and the message indicates problem from the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it's confusing, I however (personally) would keep the end user in mind as he is the one that's gonna read the message, therefore should be meaningful to them.
res.status(200).send("hello from backend to frontend!"); | ||
}); | ||
|
||
app.post("/weather", async (req, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually when we have this amount of checks we make what's called a middleware, please feel free to research it, however in basic terms it's a function that will be called before the actual response function called, finally we will either give the 200 response or branch it off to be handled with the appropriate error endpoint.
I believe you will learn more about this later in this course, or possibly during the project module.
node_modules | ||
.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit curios why haven't you used dotenv
? as it will be a bit safer and more reliable way?
Am asking this here as you have ignored .env
file. it's okay if you copied, you can look it up if you feel a bit of curiosity, you will need it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello prof-xed. Thank you so much for taking the time to review and give feedback. Sorry for my late reply I just saw your messages. I used a .env
file to store values like PORT
and localhost and use them in my code using process.env.PORT
. But, I forgot about this setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, it's not required, I just wanted to mention it as you have it ignored and used a different method for env vars.
Still nice to know, and am really happy that you are aware of its usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to you too for your kind reply as well. ✌️
No description provided.