First merge if Backend branch vs v.2.x #22

martacolombas · 2020-03-24T11:28:47Z

This pull request aims to clean up all the commits and code added in the last couple of days, before opening a formal pull request.

…o backend

martacolombas

Fix changes, comment code and resend PR

client/src/App.js

client/src/Components/Login/Login.js

server/config/db-config.js

server/config/passport-config.js

martacolombas · 2020-03-24T11:43:03Z

server/queries/basic-queries.js

+    User.findOne({ githubId: profile.id }).then((errors, currentUser) => {
+      if (currentUser) {
+        currentUser.overwrite({ token: accessToken }).save();
+        response.status(201).send(rows);


we are not declaring rows here. Remove

server/routes/oauth-routes.js

…o backend

…e if Backend branch vs v.2.x #22

… the API and modified Github Strategy to save the new token every time the api returns a new authentication code

martacolombas · 2020-03-25T09:39:51Z

First of all, I'd like to apologize for the magnitude of this PR. It is totally unacceptable and I will do my best to avoid this kind of situation next time.

What does this PR include?

Frontend (/client/src)

We've added the logic to understand whether a user is coming from manually fulfilling the form or through the Github authentication, since, in the latter case, we need to authenticate the user by id and bring the token from the DB to the frontend
We've added a Private route component, that is part of the Router component. The library used for the routes is react-router-dom

Backend (/server)

We've added the file structure for the project
The backend is built with Express.js and connected to a Mongo DB through mongoose
The authentication is done with passport.js
The backend includes two types of different routes (as of now): basic routes (getUsers and getUsersById) and oauth-routes (necessary routes to successfully fulfill the authentication through github)

…entials

ivanortegaalba

I tried to review everything but since it's a quite long PR I could miss some things 😅

This is an amazing job and this will set up a good base in order to move forward with the gitRank v2. So excited for that! 👏 💯

ivanortegaalba · 2020-03-30T08:40:28Z

client/src/App.js

+  BrowserRouter as Switch,
+  Route,
+  BrowserRouter,


I think we're importing BrowserRouter twice. Maybe what we wanted is something like the basic example of React Router
In that case, we should modify:

Suggested change

BrowserRouter as Switch,

Route,

BrowserRouter,

Switch,

Route,

BrowserRouter,

ivanortegaalba · 2020-03-30T08:41:43Z

client/src/App.js

-  const [token, setToken] = useState('');
-  const [username, setUsername] = useState('');
-  const [offline, setOffline] = useState(!navigator.onLine);
+  const [token, setToken] = useState(localStorage.getItem('token') || '');


I can see a very common pattern where you want to Load/Save state from the local storage and set it in the state. I'd think about having a custom hook for that. What do you think?

ivanortegaalba · 2020-03-30T08:43:45Z

client/src/App.js

+  /* the user can access the app through two different options:
+  1. indicating username and token manually. In the case of GitHub Enterprise the user also needs to indicate the endpoint.
+  2. Github authentication */


It's a very common practice to use block comment like this:

Suggested change

/* the user can access the app through two different options:

1. indicating username and token manually. In the case of GitHub Enterprise the user also needs to indicate the endpoint.

2. Github authentication */

/**

* the user can access the app through two different options:

* 1. indicating username and token manually. In the case of GitHub Enterprise the user also needs to indicate the endpoint.

* 2. Github authentication

**/

But this is totally up to you :)

ivanortegaalba · 2020-03-30T08:46:20Z

client/src/App.js

+    const userId = getIdFromLocation();
+    if (userId) {
+      // if there's id, we need to look for the token stored in the ddbb
+      fetch(`http://localhost:8080/users/${userId}`)


Have you thought about using environment variables to set the API_URL? I think it could be a good step to upload your code without needing to app another PR to change this :)

Suggested change

fetch(`http://localhost:8080/users/${userId}`)

fetch(`${process.env.API_URL}/users/${userId}`)

ivanortegaalba · 2020-03-30T08:47:16Z

client/src/App.js

+        .then(res => {
+          return res.text();
+        })


Could we use arrow return syntax instead? I think we can save a couple of lines :)

Suggested change

.then(res => {

return res.text();

})

.then(res => res.text())

ivanortegaalba · 2020-03-30T21:48:25Z

server/package.json

@@ -0,0 +1,33 @@
+{
+  "name": "server",


Suggested change

"name": "server",

"name": "gitRank API",

ivanortegaalba · 2020-03-30T21:52:50Z

server/routes/basic-routes.js

+router.get('/users', queries.getUsers);
+router.get('/users/:id', queries.getUserById);


This must be private routes. Otherwise, the server will be accessible.

ivanortegaalba · 2020-03-30T21:54:26Z

server/routes/oauth-routes.js

+/* callback route for github to redirect to
+hand control to passport to use code to grab profile info */
+router.get('/github/redirect', passport.authenticate('github'), (req, res) => {
+  res.redirect(`http://localhost:3000/dashboard?id=${req.user.githubId}`);


Suggested change

res.redirect(`http://localhost:3000/dashboard?id=${req.user.githubId}`);

res.redirect(`${process.env.BASE_URL}/dashboard?id=${req.user.githubId}`);

ivanortegaalba · 2020-03-30T22:03:53Z

server/queries/basic-queries.js

+    const users = await User.find();
+    response.status(200).send(users);
+  } catch (error) {
+    response.status(500);


It's important to send the error message to the response in order to give context to the API client.

Suggested change

response.status(500);

response.status(500).send(error);

ivanortegaalba · 2020-03-30T22:05:06Z

server/queries/basic-queries.js

+      const user = await User.findOne({ githubId: id });
+      response.status(200).send(user);
+    } else {
+      response.status(204);


Suggested change

response.status(204);

response.status(204).send(`User with id ${id} not found`);

martacolombas added 23 commits March 13, 2020 12:49

feat: added routes and queries in be

8dfee0f

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

1c6c27f

…o backend

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

08a679f

…o backend

fix: server is connected to db

051bc60

merge from master

4d53feb

feat: added router in be

43ad358

feat: added queries with promise handling

b60baa7

feat: added error handling in BE API

60fc209

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

442731c

…o backend

feat: added routes folder in server

2ae910a

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

b834e05

…o backend

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

26b0764

…o backend

fix: sorted prs pinned and not pinned

be9fdce

chore: organized server files into new, well named, folders

657c5f7

chore: removed unnecessary logs

43c044a

feat: connected to mongodb

89cf812

feat: Added React Router in frount end

71f9ce4

feat: added oauth routes with redirection

3513793

feat: added PrivateRoute

d760a02

feat: refreshes token after logout

5490e7e

feat: update temporary token

b0c97f5

chore: deleted unnecessary routes

48e5911

fix: removed debugger ...

8f2f509

martacolombas commented Mar 24, 2020

View reviewed changes

martacolombas added 4 commits March 24, 2020 12:45

Merge branch 'master' of https://github.com/martacolombas/gitrank int…

b6e5e6e

…o backend

feat: added modifications required as per the review in PR First merg…

1cf184a

…e if Backend branch vs v.2.x #22

fix: mongoose methods updated to comply with upciming deprecations of…

f89f0ca

… the API and modified Github Strategy to save the new token every time the api returns a new authentication code

chore: removed unnecessary endpoints

071d978

martacolombas requested a review from ivanortegaalba March 25, 2020 09:39

martacolombas added 2 commits March 25, 2020 11:33

feat: Added Redirect to Dashboard when homepage is accessed with cred…

cb8c7e8

…entials

chore: removed unnecessary dependencies

5361434

ivanortegaalba requested changes Mar 30, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First merge if Backend branch vs v.2.x #22

First merge if Backend branch vs v.2.x #22

martacolombas commented Mar 24, 2020

martacolombas left a comment

martacolombas Mar 24, 2020

martacolombas commented Mar 25, 2020

ivanortegaalba left a comment

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

ivanortegaalba Mar 30, 2020

	fetch(`http://localhost:8080/users/${userId}`)
	fetch(`${process.env.API_URL}/users/${userId}`)

		router.get('/users', queries.getUsers);
		router.get('/users/:id', queries.getUserById);

	res.redirect(`http://localhost:3000/dashboard?id=${req.user.githubId}`);
	res.redirect(`${process.env.BASE_URL}/dashboard?id=${req.user.githubId}`);

	response.status(204);
	response.status(204).send(`User with id ${id} not found`);

First merge if Backend branch vs v.2.x #22

Are you sure you want to change the base?

First merge if Backend branch vs v.2.x #22

Conversation

martacolombas commented Mar 24, 2020

martacolombas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martacolombas commented Mar 25, 2020

What does this PR include?

Frontend (/client/src)

Backend (/server)

ivanortegaalba 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