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

frontend refactoring and work on auth pages #8

Merged
merged 5 commits into from
Mar 18, 2021
Merged

Conversation

vaniri
Copy link
Owner

@vaniri vaniri commented Mar 7, 2021

Working on #5

  • I refactored some common parts of the login and signup pages out.
  • I noticed some differences between the spec you gave me and the original code, and changed what I could to closer match the spec.
  • I also implemented the authorization logic on the client side.

Copy link
Collaborator

@sina-jamshidi sina-jamshidi left a comment

Choose a reason for hiding this comment

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

A nice addition would also be a privateRoute type of component that wraps a regular route but checks if the user is logged in. That way if more pages are added to the app in the future, we don't have to manually check if the user is logged in each time.


const ButtonHeader = ({ href, questionText, buttonText, classes }) => {
return (
<Box p={1} alignSelf="flex-end" alignItems="center">
Copy link
Collaborator

Choose a reason for hiding this comment

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

typically you will want to use Grid instead of Box (Grid is a wrapper for Box) to make your life easier because of the built-in flexbox

Comment on lines 83 to 101
marginTop: theme.spacing(1)
},
label: { fontSize: 19, color: "rgb(0,0,0,0.4)", paddingLeft: "5px" },
submit: {
margin: theme.spacing(3, 2, 2),
padding: 10,
width: 160,
height: 56,
borderRadius: 3,
marginTop: 49,
fontSize: 16,
backgroundColor: "#3a8dff",
fontWeight: "bold"
},
inputs: {
marginTop: ".8rem",
height: "2rem",
padding: "5px"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have a variety of spacing types here.
theme.spacing, px, rem, and just integer pixels. It would be a good practice to make as much of it uniform as possible

};

const redirect = () => {
const user = localStorage.getItem("user");
Copy link
Collaborator

Choose a reason for hiding this comment

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

storing the user in localStorage will cause problems down the line. The idea behind using localStorage for things is to persist information between sessions. You shouldn't do that with user information. One example of a problem is if I sign in to multiple browsers, and change something in one browser, those changes will not be reflected in the second one.

if (reason === "clickaway") return;
setOpen(false);
const login = async ({ email, password }) => {
const res = await fetch('http://localhost:3001/user/login', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we have a proxy set up, you should be able to use relative paths /user/login

localStorage.setItem("userId", userId);
localStorage.setItem("username", username);
localStorage.setItem("userImage", userImage);
localStorage.setItem("token", token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We prefer using httpOnly cookies to localStorage, since it is less accessible

Comment on lines 50 to 55
<ButtonHeader
classes={classes}
href="/signup"
questionText="Don't have an account?"
buttonText="Create account"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice abstraction

Comment on lines 26 to 27
localStorage.setItem("userId", userId);
localStorage.setItem("username", username);
Copy link
Collaborator

Choose a reason for hiding this comment

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

see notes above

React.useEffect(() => {
const user = localStorage.getItem("user");
useEffect(() => {
const user = 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.

this variable should probably be called token or loggedIn. You also have to be careful. What if the token exists in localStorage but is expired?

Copy link
Collaborator

@emad-darabeh emad-darabeh left a comment

Choose a reason for hiding this comment

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

Code needs some more organization. moving the logic of checking the user is logged in or not to the PrivateRoute and remove this logic from other components.

Comment on lines 15 to 17
const [userId, setUserId] = useState("");
const [username, setUsername] = useState("");
const [userImage, setUserImage] = useState("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Storing each field of the user information in its own state is not the best choice here, what if the user had 10 or 20 key/value pairs in it. Why not store an object of the user data

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great catch, thanks!

Comment on lines 20 to 34

const refreshAuth = async () => {
const res = await fetch('/user/refresh', { method: 'POST' });

if (res.status === 200) {
const { username, userImage } = await res.json();
setSignedIn(true);
setUsername(username);
setUserImage(userImage);
} else {
setSignedIn(false);
history.push("/login");
}
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not let the PrivateRoute handle the logic of check if the user is logged in or not. And also handle the logic of forwarding the user to a certain route if they are not logged in.

Comment on lines 35 to 45
const logout = async () => {
await fetch('/user/logout', { method: 'POST' });

setLoggingOut(true);
try {
history.push('/login');
} finally {
setLoggingOut(false);
setSignedIn(false);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this logic to a more relevant component, perhaps the component that is responsible for logging out the user.

Comment on lines 3 to 7

const PrivateRoute = ({ component: Component, isSignedIn, refreshAuth, path, ...rest }) => {
return (
<Route
path={path}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component could encapsulate the logic related to checking the user is logged in or not, and based on that forwarding the user to a certain route.

Comment on lines 43 to 44
bgcolor: "background.paper",
// minHeight: "100vh",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not leave commented-out code in PRs, you can always get code from old commits.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That accidentally slipped in, will remove indeed!

Copy link
Collaborator

@emad-darabeh emad-darabeh left a comment

Choose a reason for hiding this comment

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

The code is a lot cleaner than it was before. be careful bout edge cases that cause lots of render or unnecessary requests.

Comment on lines 191 to 55
Welcome back!
<p className={classes.welcome} component="h1" variant="h5">
Welcome back!
</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use Material UI's Typography instead of p and h tags

Comment on lines 2 to 4
import { useEffect } from "react";
import { Button, CssBaseline, TextField, Paper, Grid, Typography } from "@material-ui/core";
import { Formik } from "formik";
Copy link
Collaborator

Choose a reason for hiding this comment

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

great way of using destructuring

Comment on lines 25 to 27
} else if (res.status === 401) {
throw new Error("Invalid password");
} else if (res.status !== 200) {
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 better from a security perspective not to point out what the user did wrong when they are trying to login.
So a more generic message saying wrong credentials would be better.

Comment on lines +17 to +18
setSignedIn(false);
history.push("/login");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also empty the local storage from any flag that indicates that the user is authenticated if this fails

Copy link
Owner Author

Choose a reason for hiding this comment

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

After my last changed we don't use local storage anymore, so this should be good!

Comment on lines 24 to 21
</BrowserRouter>
<Route path="/login" component={Login} />
<Route path="/signup" component={Signup} />
<PrivateRoute path="/dashboard" component={Dashboard} />
<Route exact path="/">
<Redirect to="/signup" />
</Route>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a way to make sure an authenticated user does not get back to signup or login pages unless they logout.


const GreetingSideBar = ({ classes }) => {
return (
<Grid item xs={false} sm={4} md={5} className={classes.image}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like some spacing issues here, do you happen to use a linter? It would probably make these issues much more easy in the future :)

onClose={handleClose}
message={message}
action={
<React.Fragment>
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm do you need this extra React.Fragment wrapping wrong IconButton?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great catch! We don't need it here!


if (res.status === 200) {
setSignedIn(true);
setUserInfo(await res.json());
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to pull out await res.json() in it's own line of code

@vaniri vaniri merged commit 1d11a98 into master Mar 18, 2021
@vaniri vaniri deleted the frontend_auth_pages branch March 18, 2021 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants