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

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

Open
wants to merge 29 commits into
base: v.2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8dfee0f
feat: added routes and queries in be
martacolombas Mar 13, 2020
1c6c27f
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 13, 2020
08a679f
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 13, 2020
051bc60
fix: server is connected to db
martacolombas Mar 13, 2020
4d53feb
merge from master
martacolombas Mar 13, 2020
43ad358
feat: added router in be
martacolombas Mar 13, 2020
b60baa7
feat: added queries with promise handling
martacolombas Mar 14, 2020
60fc209
feat: added error handling in BE API
martacolombas Mar 14, 2020
442731c
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 18, 2020
2ae910a
feat: added routes folder in server
martacolombas Mar 18, 2020
b834e05
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 18, 2020
26b0764
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 18, 2020
be9fdce
fix: sorted prs pinned and not pinned
martacolombas Mar 19, 2020
657c5f7
chore: organized server files into new, well named, folders
martacolombas Mar 19, 2020
43c044a
chore: removed unnecessary logs
martacolombas Mar 19, 2020
89cf812
feat: connected to mongodb
martacolombas Mar 19, 2020
71f9ce4
feat: Added React Router in frount end
martacolombas Mar 19, 2020
3513793
feat: added oauth routes with redirection
martacolombas Mar 20, 2020
d760a02
feat: added PrivateRoute
martacolombas Mar 22, 2020
5490e7e
feat: refreshes token after logout
martacolombas Mar 22, 2020
b0c97f5
feat: update temporary token
martacolombas Mar 24, 2020
48e5911
chore: deleted unnecessary routes
martacolombas Mar 24, 2020
8f2f509
fix: removed debugger ...
martacolombas Mar 24, 2020
b6e5e6e
Merge branch 'master' of https://github.com/martacolombas/gitrank int…
martacolombas Mar 24, 2020
1cf184a
feat: added modifications required as per the review in PR First merg…
martacolombas Mar 24, 2020
f89f0ca
fix: mongoose methods updated to comply with upciming deprecations of…
martacolombas Mar 24, 2020
071d978
chore: removed unnecessary endpoints
martacolombas Mar 24, 2020
cb8c7e8
feat: Added Redirect to Dashboard when homepage is accessed with cred…
martacolombas Mar 25, 2020
5361434
chore: removed unnecessary dependencies
martacolombas Mar 25, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
.vscode
/server
.DS_Store

17 changes: 11 additions & 6 deletions client/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"react-router-dom": "^5.1.2",
"react-scripts": "3.4.0",
"react-select": "^3.0.8",
"react-social-login-buttons": "^3.0.0",
"react-transition-group": "^4.3.0",
"use-debounce": "^3.3.0",
"use-interval": "^1.2.1"
Expand Down
73 changes: 52 additions & 21 deletions client/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,70 @@ import React, { useState, useEffect } from 'react';
import './App.css';
import Dashboard from './Components/Dashboard/Dashboard';
import LoginPage from './Components/LoginPage/LoginPage';
import PrivateRoute from './Components/PrivateRoute/PrivateRoute';
import {
BrowserRouter as Switch,
Route,
BrowserRouter,
Comment on lines +7 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,

Redirect,
} from 'react-router-dom';
import { getIdFromLocation } from './helperFunc';

function App() {
const [token, setToken] = useState('');
const [username, setUsername] = useState('');
const [offline, setOffline] = useState(!navigator.onLine);
const [token, setToken] = useState(localStorage.getItem('token') || '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

const [username, setUsername] = useState(
localStorage.getItem('username') || ''
);

/* 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 */
Comment on lines +20 to +22
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)


useEffect(() => {
if (localStorage.getItem('token')) {
setToken(localStorage.getItem('token'));
setUsername(localStorage.getItem('username'));
// in case that the user is authenticated through github, an id will be attached to the url.
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}`)
martacolombas marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}`)

.then(res => {
return res.text();
})
Comment on lines +30 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

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

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())

.then(data => {
try {
const parsedData = JSON.parse(data);
// save the token in localstorage
localStorage.setItem('token', parsedData.token);
localStorage.setItem('username', parsedData.username);
// we change the state of the component since now the user is authenticated
assignCredentials(parsedData.username, parsedData.token);
// we redirect to the dashboard
window.location.href = '/dashboard';
} catch (e) {
console.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This process is quite important since we're setting the session for the user. It could be interesting to have error tracking on this part of the code since could be a source of error. New Relic allow you to manually send caught errors to the browser agent:

// `newrelic` is a global variable declared by the New Relic snippet loaded in the HTML.
newrelic && newrelic.noticeError(error)

I recommend you to "safely" call noticeError checking first if newrelic exists. Actually I'd create some utils to notice those errors so we can change the monitoring tool easily without go to all the lines where we're using New Relic to replace it for the new tool. Something like

import {noticeError} from '../utils/monitor';

noticeError(error)

`Error in parsing json data while requesting user by Id : ${e}`
);
}
});
}

window.addEventListener('online', setOfflineStatus);
window.addEventListener('offline', setOfflineStatus);
return function cleanup() {
window.removeEventListener('online', setOfflineStatus);
window.removeEventListener('offline', setOfflineStatus);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏

};
}, []);

function setOfflineStatus() {
setOffline(!navigator.onLine);
}

function assignCredentials(usernameVal, tokenVal) {
setUsername(usernameVal);
setToken(tokenVal);
}

return token ? (
<Dashboard token={token} username={username} offline={offline} />
) : (
<LoginPage assignCredentials={assignCredentials} offline={offline} />
return (
<BrowserRouter>
<Switch>
<Redirect from='/' to='/dashboard' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

🏆

<PrivateRoute path='/dashboard'>
<Dashboard token={token} username={username} />
</PrivateRoute>
<Route path='/login'>
<LoginPage assignCredentials={assignCredentials} />
</Route>
</Switch>
</BrowserRouter>
);
}

Expand Down
6 changes: 5 additions & 1 deletion client/src/Components/Assign/Assign.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Button from '../Button/Button';
import cx from 'classnames';

function Assign({ userId, prId, className, isAssigned, currentAssignees }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is the button, what do you think about be more specific with the name of the component? I didn't guess the first time what the component was about 😄 Something like AssignButton maybe? But this is totally up to you

// if there's previous assigness we keep their id
let prevAssignees = [];
if (currentAssignees.length) {
prevAssignees = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to spread the map? Map returns a new array so probably the spread is not needed

Expand All @@ -22,13 +23,16 @@ function Assign({ userId, prId, className, isAssigned, currentAssignees }) {

function handleAssignment(event) {
if (!isAssigned) {
// if the pr is not yet assigned to the user, we add the userId to the previous assignees' ids
assignId.push(userId);
event.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we move the event.prefentDefault() out of the conditional block? We're duplicating the code since we're calling it in both condition cases.

// perform mutation
assignToMe({ variables: { pullRequestId: prId, assigneeIds: assignId } });
if (data) {
console.log('Succesfully assigned to you');
}
} else {
// otherwsise we filter all the ids except the user's and we perform the mutation
event.preventDefault();
assignToMe({
variables: {
Expand All @@ -47,7 +51,7 @@ function Assign({ userId, prId, className, isAssigned, currentAssignees }) {
icon='user-check'
title='Assign to me'
onClick={handleAssignment}
className={isAssigned ? `${classnames}--isFavorite` : classnames}
className={isAssigned ? `${classnames}--isFavorite` : classnames} // isFavorite needs renaming as the same modifications are applied for the favoriting feature
/>
);
}
Expand Down
2 changes: 2 additions & 0 deletions client/src/Components/Avatar/Avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ function Avatar({
}) {
const [imageError, setImageError] = useState(false);
const classnames = cx('Avatar', className);
/* we found that using github enterprise returned error when rendering images coming from the company's servers.
We are proactively handling this error here */
return !imageError ? (
<div className={classnames} style={{ width: size, height: size }}>
<img
Expand Down
1 change: 1 addition & 0 deletions client/src/Components/Badge/Badge.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import cx from 'classnames';
import Emoji from 'a11y-react-emoji';

function Badge({ className, type = '', ...props }) {
// we want to keep the different states as we want to show them if the PR is favorited in the future
const emojis = {
OPEN: <Emoji symbol='👋' label='open' className='emoji' />,
MERGED: <Emoji symbol='👍' label='open' className='emoji' />,
Expand Down
7 changes: 5 additions & 2 deletions client/src/Components/Dashboard/Dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import NavBar from '../NavBar/NavBar';

library.add(fas);

function Dashboard({ className, username }) {
function Dashboard({ className, username, ...props }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the rest of the prods here?

// STATES
const [pinnedItems, setPinnedItems] = useState(
localStorage.getItem('pinnedItems')
Expand All @@ -35,11 +35,13 @@ function Dashboard({ className, username }) {
const [isOpen, toggleSidebar] = useState(false);
let uid;

// helper Function
function toggleBar() {
toggleSidebar(!isOpen);
}

// API CALLS
// PR Data
const { loading, data, error } = useQuery(GET_PRS, {
variables: {
login: `${username}`,
Expand All @@ -53,6 +55,7 @@ function Dashboard({ className, username }) {
uid = data.user.id;
}

// Repos data
const {
loading: reposLoading,
data: reposData,
Expand All @@ -62,7 +65,7 @@ function Dashboard({ className, username }) {
login: `${username}`,
},
});

// PR authors data
const {
loading: authorsLoading,
data: authorsData,
Expand Down
8 changes: 5 additions & 3 deletions client/src/Components/Dashboard/utils.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
// groups prs from the user
function getPRs(repos) {
return repos.reduce((acc, element) => {
return acc.concat(element.pullRequests.nodes);
}, []);
}

// gets repos from org
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes when a function is self-descriptive, the comments are not very useful. On the other hand, I personally, when I have a collection on utils, I usually write documentation on them.
Have you checked about JS Docs format to add them? It's quite useful since you can generate static sites with your API. Besides this, it's the common standard for JS.

function getReposFromOrgs(orgs) {
return orgs.reduce(
(acc, element) => acc.concat(element.repositories.nodes),
[]
);
}

// gets als prs together
export function groupPRs(queryData) {
const userReposPRs = getPRs(queryData.user.repositories.nodes);
const orgsReposPRs = getPRs(
Expand All @@ -19,6 +20,7 @@ export function groupPRs(queryData) {

return [...userReposPRs, ...orgsReposPRs];
}
//helper function to filter by repos
export function filterByRepos(allPRs, repos, authors) {
const firstFiltering =
Array.isArray(repos) && repos.length > 0
Expand All @@ -32,7 +34,7 @@ export function filterByRepos(allPRs, repos, authors) {
)
: firstFiltering;
}

//groups all repos
export function groupAllRepos(queryData) {
const userRepos = queryData.user.repositories.nodes;
const orgRepos = getReposFromOrgs(queryData.user.organizations.nodes);
Expand Down
19 changes: 17 additions & 2 deletions client/src/Components/Login/Login.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import './Login.css';
import Button from '../Button/Button';
import cx from 'classnames';
import Checkbox from '../Checkbox/Checkbox';
import { GithubLoginButton } from 'react-social-login-buttons';
import { useHistory, useLocation } from 'react-router-dom';

//TODO(marta) create a form component

Expand All @@ -14,7 +16,12 @@ function Login({ className, assignCredentials }) {
const [enterpriseUrl, setEnterpriseUrl] = useState('');
const [isFormEnabled, setEnableForm] = useState(false);

useEffect(() => {});
// gets access to the browser history object
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not wrong, this access to the React Router History object, not the one from the browser. I remember to see it here

let history = useHistory();
// returns the current location object
let location = useLocation();

let { from } = location.state || { from: { pathname: '/dashboard' } };
Comment on lines +20 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use const since the variable is not reassigned to a different value. WDYT?


function handleTokenChange({ target }) {
setToken(target.value);
Expand All @@ -33,6 +40,8 @@ function Login({ className, assignCredentials }) {
}

function handleSubmit(event) {
/* if the user accesses the app through the form,
all the data will be handled by the above functions and stored in localstorage*/
event.preventDefault();
localStorage.setItem('username', username);
localStorage.setItem('token', token);
Expand All @@ -41,6 +50,8 @@ function Login({ className, assignCredentials }) {
localStorage.setItem('enterpriseUrl', JSON.stringify(enterpriseUrl));
}
assignCredentials(username, token);
// we redirect to the dashboard
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not wrong, I think this will redirect you to the previous route that has been redirected to the login.

history.replace(from);
}

useEffect(() => {
Expand All @@ -55,7 +66,7 @@ function Login({ className, assignCredentials }) {
} else if (isFormEnabled) {
setEnableForm(false);
}
}, [username, token, isEnterprise, enterpriseUrl]);
}, [username, token, isEnterprise, enterpriseUrl, isFormEnabled]);

return (
<div className={classnames}>
Expand All @@ -64,6 +75,10 @@ function Login({ className, assignCredentials }) {
className='pic'
alt='login femalecodercat'
></img>
<a href='http://localhost:8080/oauth/github'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you plan to move this to the production URL once you deploy the backend but I'd like to propose you a way I usually do it to avoid a second PR:

Suggested change
<a href='http://localhost:8080/oauth/github'>
<a href={`${process.env.API_URL}/oauth/github`}>

<GithubLoginButton />
</a>
<p className='Login-link'>or</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<p className='Login-link'>or</p>
<p className='Login-separator'>or</p>

<form onSubmit={handleSubmit} className='Login-form'>
<input
placeholder='Username'
Expand Down
17 changes: 17 additions & 0 deletions client/src/Components/Login/auth-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
export const isLoggedIn = () => {
if (
localStorage.getItem('isEnterprise') &&
localStorage.getItem('enterpriseURL')
) {
if (localStorage.getItem('token') && localStorage.getItem('username')) {
return true;
}
} else if (
localStorage.getItem('token') &&
localStorage.getItem('username')
) {
return true;
} else {
return false;
}
Comment on lines +2 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (
localStorage.getItem('isEnterprise') &&
localStorage.getItem('enterpriseURL')
) {
if (localStorage.getItem('token') && localStorage.getItem('username')) {
return true;
}
} else if (
localStorage.getItem('token') &&
localStorage.getItem('username')
) {
return true;
} else {
return false;
}
const isValidEnterprise = localStorage.getItem('isEnterprise') ? localStorage.getItem('enterpriseURL') : true;
return isValidEnterprise && localStorage.getItem('token') && localStorage.getItem('username') ;

};
2 changes: 0 additions & 2 deletions client/src/Components/LoginPage/LoginPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ import React from 'react';
import './LoginPage.css';
import Login from '../Login/Login';
import cx from 'classnames';
import Badge from '../Badge/Badge';

function LoginPage({ className, assignCredentials, offline }) {
const classnames = cx('LoginPage', className);
return (
<div className={classnames}>
{offline && <Badge type={offline} className='LoginPage-offlineBadge' />}
<Login assignCredentials={assignCredentials} />
</div>
);
Expand Down
3 changes: 2 additions & 1 deletion client/src/Components/NavBar/NavBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import TransitionPage from '../TransitionPage/TransitionPage';
function NavBar({ className, ...props }) {
const classnames = cx('NavBar', className);
const username = localStorage.getItem('username');

const { loading, data, error } = useQuery(GET_USERINFO, {
variables: {
login: `${username}`,
Expand Down Expand Up @@ -61,7 +62,7 @@ function NavBar({ className, ...props }) {
className='NavBar-user-pic'
/>
</button>
{data.user.login}
{username}
</div>
<Button
icon={faGithub}
Expand Down
Loading