Added Reputation, Number of Posts, and User Icons #12

OlexG · 2021-06-14T19:28:58Z

No description provided.

OlexG · 2021-06-14T19:59:58Z

forumPR12.mp4

irenlian · 2021-06-18T14:12:29Z

client/src/Components/App/App.js

@@ -10,6 +10,8 @@ import PostMenu from '../PostMenu/PostMenu.js';
 import { POSTS_PER_PAGE } from '../../constants.js';
 import './styles.css';
 import useReactionsFetch from '../../Hooks/useReactionsFetch.js';
+import UserDashboard from '../UserDashboard/UserDashboard.js';


To avoid such type of import naming as /UserDashboard/UserDashboard.js, you can put index.js in the folder from where you do an export (like proxy export). And in this case, you may leave here only import UserDashboard from '../UserDashboard';

irenlian · 2021-06-18T14:17:15Z

client/src/Components/PostPage/PostPage.js

@@ -33,6 +33,10 @@ const PostPage = ({ match }) => {
 				(
 					<div className='post card mb-2' style={{ 'margin': '1em' }}>
 						<div className='card-body'>
+							<div className={styles.imgContainer}
+								style={{ 'background-image': `url("/api/v1/users/icon?username=${data.author}")` }}


What about changing API to /api/v1/users/:id/icon. It's like we're based on users -> getting the specific user -> the icon is related to this user.
I guess it would be more in line with REST naming than using params. What do you think?
Or you just don't have the id and only the name?

Here I only have the information about the post which only includes the author's name.

irenlian · 2021-06-18T14:22:48Z

client/src/Components/UserDashboard/UserDashboard.js

+	async function handleChange (e) {
+		const formData = new FormData();
+		formData.append('image', e.target.files[0]);
+		await api.sendChangeIconRequest(formData);


What if this request fails?

irenlian · 2021-06-18T14:41:32Z

client/src/Components/UserDashboard/UserDashboard.js

+
+	function handleClick () {
+		// trigger the click event of the hidden "choose image" input
+		hiddenInput.current.click();


This is the smart approach. But I'd suggest checking if the current exists.

irenlian · 2021-06-18T14:44:34Z

client/src/Components/UserDashboard/UserDashboard.js

+		hiddenInput.current.click();
+	}
+	return (
+		<>


Suggested change

<>

irenlian · 2021-06-18T14:48:55Z

client/src/Components/UserDashboard/UserDashboard.module.css

+.editIcon{
+	position: absolute; 
+	bottom:90%; 
+	left:0;
+	color: #ffffff;
+}


Please, pay attention to these details. I'd suggest using the pre-commit hook with eslint and typescript checks.

Suggested change

.editIcon{

position: absolute;

bottom:90%;

left:0;

color: #ffffff;

}

.editIcon {

position: absolute;

bottom: 90%;

left: 0;

color: #ffffff;

}

irenlian · 2021-06-18T15:02:18Z

server/routes/users/index.ts

+// get user data such as reputation and number of posts
+app.get('/api/v1/users', validateRefreshJWT, wrap(userController.getUserData));
+
+// change the user image
+app.post('/api/v1/users/change-icon', validateRefreshJWT, upload.single('image'), wrap(userController.changeUserIcon));
+
+// get the icon of a user
+app.get('/api/v1/users/icon', wrap(userController.getUserIcon));


Here you have several APIs and all of them are around a single user. And it is not clear from the route.
When you work with a single object you may specify the specific object in a route as users/:id/....
Because how would you create an endpoint to get the list of all users right now?

And one more topic related is where do you use cookies. You may use it for validation - if this user has the right to perform a particular action. But you can't make an assumption that the user wants to perform an action under his own instance.
It could be a situation where you may have the catalog of all users of the public user page, where I can go to the link and get your icon, post number, and reputation.

irenlian · 2021-06-18T15:06:50Z

server/routes/users/index.ts

+	fileFilter: function(req, file, cb) {
+		validateFile(file, cb);
+	}


You can adjust the function to use it directly and specify its type according to multer as well.

Suggested change

fileFilter: function(req, file, cb) {

validateFile(file, cb);

}

fileFilter: validateFile;

irenlian · 2021-06-18T15:10:10Z

server/controllers/userController.ts

+	const refreshToken = req.cookies.refreshToken;
+	const username = await userManager.findRefreshToken(refreshToken);


I would highly recommend looking closer at these two lines which you use everywhere. Maybe, you can figure out how to make it nicer.

irenlian

Good job and just a few comments to change.

And let's add tests for some of the code. For instance, for middlewares.

irenlian · 2021-06-22T05:06:25Z

client/src/Components/LoginPage/index.js

-import NavbarComponent from '../Navbar/Navbar.js';
+import NavbarComponent from '../Navbar';


Just want to comment additionally that you actually don't need to rename Navbar.js to index.js. But you should create index.js with something like:

export { default } from './Navbar';

irenlian · 2021-06-22T05:09:20Z

client/src/Components/UserDashboard/index.js

+		if (res.status === 200) {
+			window.location.reload();
+		} else if (res.status === 401) {
+			setPopup({ 'message': 'Something went wrong. Please re-login again.' });


Suggested change

setPopup({ 'message': 'Something went wrong. Please re-login again.' });

setPopup({ message: 'Something went wrong. Please re-login again.' });

irenlian · 2021-06-22T05:14:24Z

client/src/Components/UserDashboard/index.js

+	async function handleChange (e) {
+		const formData = new FormData();
+		formData.append('image', e.target.files[0]);
+		const res = await api.sendChangeIconRequest(formData);


Please, take into account that axios.post may throw an error and UI won't show an error properly.
Look for such cases, since it's really easy to catch them in the front-end and they may not be critical. But if you leave a possible error unhandled it could lead to an issue that may influence a lot and could be hard to find and debug.
And that's why TDD is a good practise which helps you to think about edge cases first.

irenlian · 2021-06-22T05:15:27Z

client/src/Components/UserDashboard/index.js

+				<p className={styles.editIcon} onClick={handleClick}>Edit Image</p>
+				<input ref={hiddenInput} type='file' id='fileField' name='file' accept='image/*' hidden='true' onChange={handleChange}/>
+				<div className={`mx-auto ${styles.imgContainer}`}
+					style={{ 'background-image': `url("/api/v1/users/${Cookies.get('username')}/icon")` }}


What if the cookie wasn't found? How the link would look like?

I believe that this component will only be loaded if the cookie exists.

@OlexG Actually, Cookies.get('username') will return undefined. And the link would look like /api/v1/users/undefined/icon`. But I suppose here should be a default icon in this case.

irenlian · 2021-06-22T05:19:41Z

client/src/Hooks/useReactionsFetch.js

+		api.sendReactionsRequest(username).then((res) => {
 			setReactions(res.data);
 			setLoading(false);
 		}).catch((error) => {
 			setError(error);
 		});


Suggested change

api.sendReactionsRequest(username).then((res) => {

setReactions(res.data);

setLoading(false);

}).catch((error) => {

setError(error);

});

api.sendReactionsRequest(username).then((res) => {

setReactions(res.data);

}).catch((error) => {

setError(error);

}).finally(() => {

setLoading(false);

});

I decided to switch to an await/async with try-catch model here

irenlian · 2021-06-22T05:36:48Z

server/db/dbManager.ts

@@ -17,6 +19,11 @@ export interface FilterObject {
 	search: string;
 }

+export interface PublicUserData{


This type of mistake means that eslint doesn't cover this file.

Suggested change

export interface PublicUserData{

export interface PublicUserData {

Actually, it looks like the "space-before-blocks" rule doesn't apply to interfaces because, for other blocks in the file, eslint throws an error if there is no space. I am not sure why this is the case I will try to find a fix.

I assume the tslint can fix it, not eslint. Am I right?

looks like the eslint-typescript plugin does not have a fix for this typescript-eslint/typescript-eslint#1606. I will use prettier in addition to eslint.

irenlian · 2021-06-22T05:38:13Z

package.json

+  "lint-staged": {
+    "**/*.{js, css}": [
+      "npx stylelint \"**/*.css\"",
+      "eslint **/*.js --fix-dry-run"


This doesn't cover server's typescript files.

irenlian · 2021-06-22T05:55:04Z

server/db/dbManager.ts

@@ -173,13 +180,14 @@ export class PostManager {
 	}

 	async upvotePost(postID: string, username: string, userManager: UserManager): Promise<boolean> {
-		if (await userManager.removePostDownvote(postID, username)) {
+		const { author } = await this.model.findOne({ _id: new ObjectId(postID) }).exec();
+		if (await userManager.removePostDownvote(postID, username, author)) {


Actually when you add the verb method in the if clause (like, if remove then update and if add then update), it makes the code less readable. And I can't understand what is the logic behind these steps.

So you have several options (in the order of preference):

change the result type from boolean to { hasBeenChanged/madeChange/removed/executed/...: boolean } and when you use this variable in the if clause it makes sense that you check whether the action has been actually performed

rename the verb method (I don't have suggestions unfortunately)

make removePostDownvote and get the result to a variable and use it in if clause

add comments

I would prefer the first option.

irenlian · 2021-06-22T05:58:09Z

server/db/dbManager.ts

 		});
 		return 'success';
 	}

+	async changeIconPath(username: string, path: string): Promise<string> {


Perhaps, update suits better?

Suggested change

async changeIconPath(username: string, path: string): Promise<string> {

async updateIconPath(username: string, path: string): Promise<string> {

irenlian · 2021-06-22T06:00:41Z

server/routes/users/index.ts

+app.get('/api/v1/users/:username', validateUsernameParam, wrap(userController.getUserData));
+
+// change the user image
+app.post('/api/v1/users/change-icon', validateRefreshJWT, validateUsernameCookie, upload.single('image'), wrap(userController.changeUserIcon));


This is the beauty of REST. You can and should use the same route for GET and POST of the same resource.

And this will help you to be consistent with validation as well.

Suggested change

app.post('/api/v1/users/change-icon', validateRefreshJWT, validateUsernameCookie, upload.single('image'), wrap(userController.changeUserIcon));

app.post('/api/v1/users/:username/icon', validateRefreshJWT, validateUsernameCookie, upload.single('image'), wrap(userController.changeUserIcon));

irenlian · 2021-06-23T15:59:10Z

.eslintrc.json

+		"plugin:@typescript-eslint/eslint-recommended",
+		"plugin:@typescript-eslint/recommended"


Something to look at: the most popular Eslint configuration is from Airbnb.

I see, I decided to go with the eslint standard configuration and I think the rules I have now are good but I will look at the airbnb configuration as well.

irenlian · 2021-06-23T16:08:41Z

client/src/Hooks/useReactionsFetch.js

+			try {
+				const res = await api.sendReactionsRequest(username);
+				if (res.status === 200) {
+					setReactions(res.data);
+				}
+				setLoading(false);
+			} catch (e) {
+				setPopup('Something went wrong when fetching reactions');
+			}


The similar question: what if an error happens, what would be with loading state?

if there is an error I don't want to set the state to false as then the UI will try to display the reactions when there is none

but I see the issue, loading, in this case, should be false since the request is done

irenlian · 2021-06-23T16:13:39Z

client/src/Hooks/useCommentsFetch.js

+			}
+		}
+		fetchData();
+	// eslint-disable-next-line react-hooks/exhaustive-deps


What dependencies have been omitted and why?

the setPopup function as this function will not change

Since this is the props, so you should add it in the dependency anyway. And if you're sure, that it won't be changed, it won't influence the behaviour. But if it did change, it would save you from bigger bugs.
And I would check inside the useEffect if comments exist already. It depends on what you're going to do if setPopup is undefined.

By the way, this issue won't be so important at all if you have Typescript. Because right now you might really reuse this function with the changing setPopup (initially undefined and them populated). And no one will catch this. You must write the code imaging that some other person gonna make changes without deeply reading the logic.

Right it is not so critical. But I really ask you to verify all your code in the future using this rule: QA will gonna break your code eventually. Don't let this happen, this is your reputation and this is how you will grow as a specialist.
And your future colleagues will reuse your code and make mistakes and spend hours to debug.

This is the example of bug I fixed yesterday - someone forgot to add error (one of the output that you should save, not only data from API) to the dependencies and fetching never stopped if there was an error. And I relied on this code supposing it is safe to use, but it is hard to debug the edge cases.

irenlian

How nice to see the code after prettier! 😍

OlexG added 4 commits June 11, 2021 09:30

User dashboard with reputation and # of posts

57c9388

Recieve user icon images

35b173a

Display images on the client

31f2258

Track number of posts

7958575

Fixed image path

752712e

irenlian reviewed Jun 18, 2021

View reviewed changes

OlexG added 4 commits June 18, 2021 18:19

Pre-commit hooks and changed to index.js

44f42b7

Handle request fail

9902243

Give function to multer directly

2dd9097

Switched to :username and username cookie

3bc4c2a

irenlian reviewed Jun 22, 2021

View reviewed changes

OlexG added 3 commits June 22, 2021 10:20

Style changes and error handling of axios requests

e43ef0f

ts eslint

9d18d4d

Change icon route change

fae4d9f

irenlian reviewed Jun 23, 2021

View reviewed changes

prettier styling

8b1ca84

irenlian approved these changes Jun 24, 2021

View reviewed changes

setpopup as a dependency

4dc04f2

OlexG merged commit 3577ae7 into main Jun 24, 2021

		const refreshToken = req.cookies.refreshToken;
		const username = await userManager.findRefreshToken(refreshToken);

		import NavbarComponent from '../Navbar/Navbar.js';
		import NavbarComponent from '../Navbar';

	setPopup({ 'message': 'Something went wrong. Please re-login again.' });
	setPopup({ message: 'Something went wrong. Please re-login again.' });

	export interface PublicUserData{
	export interface PublicUserData {

	async changeIconPath(username: string, path: string): Promise<string> {
	async updateIconPath(username: string, path: string): Promise<string> {

	app.post('/api/v1/users/change-icon', validateRefreshJWT, validateUsernameCookie, upload.single('image'), wrap(userController.changeUserIcon));
	app.post('/api/v1/users/:username/icon', validateRefreshJWT, validateUsernameCookie, upload.single('image'), wrap(userController.changeUserIcon));

		"plugin:@typescript-eslint/eslint-recommended",
		"plugin:@typescript-eslint/recommended"

Added Reputation, Number of Posts, and User Icons #12

Added Reputation, Number of Posts, and User Icons #12

Conversation

OlexG commented Jun 14, 2021

OlexG commented Jun 14, 2021

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

irenlian 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

OlexG Jun 22, 2021 • edited

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

irenlian Jun 22, 2021 • edited

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

OlexG Jun 23, 2021 • edited

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

irenlian left a comment

Choose a reason for hiding this comment

OlexG Jun 22, 2021 •

edited

irenlian Jun 22, 2021 •

edited

OlexG Jun 23, 2021 •

edited