Majd hussein w2 node #12
Conversation
|
Hey @Majoodeh , it appears the committed files are incomplete. I see you've added the Weather API key and removed the code from last week, but no further changes to the /weather endpoint show up. Tasks from here onwards are missing: https://github.com/HackYourAssignment/Node.js-Cohort54/blob/main/week2/MAKEME.md#312-fetch-it-from-our-api |
durw4rd
left a comment
durw4rd
left a comment
There was a problem hiding this comment.
Thanks for adding the missing files, and for your work on this assignment, Majd. I've added a few comments, and I would like you to review & implement the suggestions.
While you have implemented the instructions, there are just too many inaccuracies for me to accept the work as is.
| "devDependencies": { | ||
| "@babel/preset-env": "^7.28.5", | ||
| "babel-jest": "^30.2.0", | ||
| "express": "^5.1.0", |
There was a problem hiding this comment.
Since Express is required at app runtime, it should be installed as a dependency, not just devDependency.
| import fetch from "node-fetch"; | ||
| const app = express(); | ||
|
|
||
| export default app; |
There was a problem hiding this comment.
Best practice is to list exports at the end of the file. Exporting before the app's routes are defined means that at the time of the export, the app doesn't include any routes, etc.
Since ES modules are evaluated asynchronously, your app still works before the app is imported in the server module, and all routes have been added, but it's better to follow the generally used pattern.
|
|
||
| // --------------5.1 Adding a POST request---------------- | ||
| app.post("/weather", (req, res) => { | ||
| const { cityName } = req.body; |
There was a problem hiding this comment.
It would be good to add input validation, making sure a cityName is provided in the request.
if (!cityName || typeof cityName !== "string" || !cityName.trim()) {
return res.status(400).json({ weatherText: "Please provide a cityName." });
}
| // --------------5.1 Adding a POST request---------------- | ||
| app.post("/weather", (req, res) => { | ||
| const { cityName } = req.body; | ||
| const cityWeatherUrl = `http://api.openweathermap.org/data/2.5/forecast?q=${cityName}&appid=${keys.API_KEY}`; |
There was a problem hiding this comment.
It doesn't make much difference from the perspective of learning how to work with APIs, but note that the instructions ask to use a different endpoint: https://api.openweathermap.org/data/2.5/**weather**.
| try { | ||
| const response = await fetch(URL); | ||
| const data = await response.json(); | ||
|
|
There was a problem hiding this comment.
A good practice when working with APIs is to check for !response.ok with correct error handling.
| .post("/weather") | ||
| .send({ cityName: "London" }); | ||
|
|
||
| expect(response.status).toBe(200); |
There was a problem hiding this comment.
Should also check response.body.weatherText contains the requested city name and temperature.
| expect(response.status).toBe(200); | ||
| }); | ||
|
|
||
| it("Case2: (Inavalid city name ) >>> should return 500 error", async () => { |
There was a problem hiding this comment.
I'd argue that an invalid city is in fact a client error, not a server error. The response code I'd expect in this scenario is 404.
| .send({ cityName: "Incorrect_CityName" }); | ||
|
|
||
| expect(response.status).toBe(500); | ||
| expect(response.body).toHaveProperty("error", "City is not found"); |
There was a problem hiding this comment.
A more accurate statement is expect(response.body.weatherText).toBe("City is not found!");
| it("Case3: (Empty City Name) should return 500 error", async () => { | ||
| const response = await request.post("/weather").send({}); | ||
| expect(response.status).toBe(500); | ||
| expect(response.body).toHaveProperty("error", "City is not found"); |
There was a problem hiding this comment.
A more accurate statement is expect(response.body.weatherText).toContain("Please provide a cityName");
| expect(response.body).toHaveProperty("error", "City is not found"); | ||
| }); | ||
|
|
||
| it("Case3: (Empty City Name) should return 500 error", async () => { |
There was a problem hiding this comment.
Empty cityName should return 400 (Bad Request), not 500.
400 = client error (invalid input), 500 = server error (internal problem)
durw4rd
left a comment
durw4rd
left a comment
There was a problem hiding this comment.
Thanks for making the changes, Majd - it looks good to me now!
2 participants
No description provided.