-
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?
Changes from all commits
761ca1e
fbad619
ab7f0a7
499b569
dfb7fe7
60f95ce
7f00ae6
73fc51b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
node_modules | ||
.env | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
import app from "../app.js"; | ||
import supertest from "supertest"; | ||
|
||
import * as helper from "../helper.js"; | ||
|
||
const request = supertest(app); | ||
|
||
describe("POST /weather", () => { | ||
// Tests for empty city string | ||
describe("Given an empty city string", () => { | ||
it("should return 400 with message 'City name is required.'", async () => { | ||
const cityName = ""; | ||
await request | ||
.post("/weather") | ||
.send({ cityName }) | ||
.expect(400) | ||
.expect({ message: "City name is required." }); | ||
}); | ||
}); | ||
|
||
// Tests for non-existent city name | ||
describe("Given a gibberish city name", () => { | ||
it("should return 404", async () => { | ||
const cityName = "ahfeiueriuter"; | ||
await request.post("/weather").send({ cityName }).expect(404); | ||
}); | ||
|
||
it("should return message 'City is not found!'", async () => { | ||
const cityName = "ahfeiueriuter"; | ||
await request | ||
.post("/weather") | ||
.send({ cityName }) | ||
.expect((res) => { | ||
expect(res.body).toHaveProperty("weatherText", "City is not found!"); | ||
}); | ||
}); | ||
}); | ||
|
||
// Tests for valid city name | ||
describe("Given a valid city name", () => { | ||
it("should return 200", async () => { | ||
const cityName = "Addis Ababa"; | ||
await request.post("/weather").send({ cityName }).expect(200); | ||
}); | ||
|
||
it("should return weather data containing city name", async () => { | ||
const cityName = "Addis Ababa"; | ||
const response = await request | ||
.post("/weather") | ||
.send({ cityName }) | ||
.expect(200); | ||
|
||
expect(response.body).toHaveProperty( | ||
"weatherText", | ||
expect.stringContaining(cityName) | ||
); | ||
}); | ||
}); | ||
|
||
// Tests for wrong domain | ||
describe("Given a wrong domain", () => { | ||
it("should return 400", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongDomain(cityName); | ||
|
||
await request.post("/weather").send({ cityName, wrongUrl }).expect(400); | ||
}); | ||
|
||
it("should return message 'Invalid domain name.'", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongDomain(cityName); | ||
|
||
await request | ||
.post("/weather") | ||
.send({ cityName, wrongUrl }) | ||
.expect({ message: "Invalid domain name." }); | ||
}); | ||
}); | ||
|
||
// Tests for invalid API key | ||
describe("Given a wrong API key", () => { | ||
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."); | ||
}); | ||
}); | ||
}); | ||
Comment on lines
+82
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I get it now 🥇
|
||
|
||
// Tests for wrong endpoint | ||
describe("Given a wrong end point", () => { | ||
it("should return 404", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongEndPoint(cityName); | ||
await request.post("/weather").send({ cityName, wrongUrl }).expect(404); | ||
}); | ||
|
||
it("should return message 'Resource not found.'", async () => { | ||
const cityName = "Addis Ababa"; | ||
const wrongUrl = helper.getWeatherUrlWithWrongEndPoint(cityName); | ||
|
||
await request | ||
.post("/weather") | ||
.send({ cityName, wrongUrl }) | ||
.expect((res) => { | ||
expect(res.body).toHaveProperty("message", "Resource not found."); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import express from "express"; | ||
import { API_KEY, WRONG_API_KEY } from "./sources/keys.js"; | ||
import fetch from "node-fetch"; | ||
import * as helper from "./helper.js"; | ||
|
||
const PORT = 3000; | ||
|
||
const app = express(); | ||
app.use(express.json()); | ||
|
||
app.get("/", (req, res) => { | ||
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 commentThe 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. |
||
const cityName = req.body.cityName; | ||
if (!cityName) { | ||
return res.status(400).send({ message: "City name is required." }); | ||
} | ||
|
||
let url = null; | ||
if (req.body.wrongUrl) { | ||
url = req.body.wrongUrl; | ||
} else { | ||
url = helper.getWeatherUrl(cityName); | ||
} | ||
|
||
try { | ||
const data = await fetchData(url); | ||
|
||
if (data.cod === 200) { | ||
return res.status(200).send({ | ||
weatherText: `The temperature in ${data.name} is ${data.main.temp}°`, | ||
}); | ||
|
||
} | ||
|
||
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."}) | ||
} | ||
Comment on lines
+38
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. love the early returns.
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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:
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 commentThe 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. |
||
|
||
if(data.cod >= 400 && data.cod < 600){ | ||
return res.status(data.cod).send({message: data.message}) | ||
} | ||
|
||
return res.status(500).send({message: "Unexpected error occurred."}) | ||
|
||
} catch (error) { | ||
if (error.message.includes("ENOTFOUND") || error.message.includes("ERR_NAME_NOT_RESOLVED")) { | ||
return res.status(400).send({ message: "Invalid domain name." }); | ||
} | ||
return res.status(500).send({ message: "Unable to fetch data." }); | ||
} | ||
}); | ||
|
||
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); | ||
} | ||
}; | ||
Comment on lines
+64
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💐 |
||
|
||
export default app; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
module.exports = { | ||
presets: [ | ||
[ | ||
// This is a configuration, here we are telling babel what configuration to use | ||
"@babel/preset-env", | ||
{ | ||
targets: { | ||
node: "current", | ||
}, | ||
}, | ||
], | ||
], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { API_KEY, WRONG_API_KEY } from "./sources/keys.js"; | ||
|
||
|
||
const units = "metric"; | ||
|
||
export const getWeatherUrl = (cityName) => { | ||
const base = "https://api.openweathermap.org/data/2.5/weather"; | ||
return `${base}?q=${cityName}&units=${units}&appid=${API_KEY}`; | ||
}; | ||
|
||
export const getWeatherUrlWithWrongApiKey = (cityName) => { | ||
const base = "https://api.openweathermap.org/data/2.5/weather"; | ||
return `${base}?q=${cityName}&units=${units}&appid=${WRONG_API_KEY}`; | ||
} | ||
|
||
export const getWeatherUrlWithEmptyApiKey = (cityName) => { | ||
const base = "https://api.openweathermap.org/data/2.5/weather"; | ||
return `${base}?q=${cityName}&units=${units}&appid=${EMPTY_API_KEY}`; | ||
} | ||
|
||
export const getWeatherUrlWithWrongDomain = (cityName) => { | ||
const base = "https://api.openweahermap.org/data/2.5/weather"; | ||
return `${base}?q=${cityName}&units=${units}&appid=${API_KEY}`; | ||
} | ||
|
||
export const getWeatherUrlWithWrongEndPoint = (cityName) => { | ||
const base = "https://api.openweathermap.org/data/2.5/weat"; | ||
return `${base}?q=${cityName}&units=${units}&appid=${API_KEY}`; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default { | ||
// Tells jest that any file that has 2 .'s in it and ends with either js or jsx should be run through the babel-jest transformer | ||
transform: { | ||
"^.+\\.jsx?$": "babel-jest", | ||
}, | ||
// By default our `node_modules` folder is ignored by jest, this tells jest to transform those as well | ||
transformIgnorePatterns: [], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
{ | ||
"name": "hackyourtemperature", | ||
"version": "1.0.0", | ||
"type": "module", | ||
"main": "server.js", | ||
"scripts": { | ||
"start": "nodemon server.js", | ||
"dev": "node --watch server", | ||
"test": "jest" | ||
}, | ||
Comment on lines
+6
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in production it can be different, but good thinking. 💯 |
||
"keywords": [], | ||
"author": "", | ||
"license": "ISC", | ||
"description": "", | ||
"dependencies": { | ||
"express": "^4.21.2", | ||
"express-handlebars": "^8.0.1", | ||
"node-fetch": "^3.3.2" | ||
}, | ||
"devDependencies": { | ||
"@babel/preset-env": "^7.26.9", | ||
"babel-test": "^0.2.4", | ||
"jest": "^29.7.0", | ||
"supertest": "^7.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* File: server.js | ||
* Description: Sets up a server to listen on port 3000 for incoming requests. | ||
*/ | ||
|
||
|
||
import app from "./app.js" | ||
|
||
const PORT = 3000; | ||
|
||
app.listen(PORT); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 |
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 likePORT
and localhost and use them in my code usingprocess.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. ✌️