As a User, I'd like to be able to have a customized Profile. #26

AlexLLKong · 2022-05-09T23:45:59Z

This is the user's profile page. They can edit their name, email, and password.

Figma wireframe

nok-ko · 2022-05-10T00:38:32Z

@AlexLLKong just changed the base branch on the PR to dev. 😅 It's easy to forget.

Co-authored-by: Alex Kong <[email protected]>

…o some file for footer and header Co-authored-by: Alex Kong <[email protected]>

…ogin page

nok-ko · 2022-05-11T20:40:30Z

main.js

+		bcrypt.hashSync(req.body.password, 10, async(err, pwd) => {
+			await db.collection('BBY-6_users')
+				.insertOne({name, emailAddress, pwd, avatarURL, date, achievements, admin, kit});
+		});


Can use await bcrypt.hash(…) here instead of .hashSync(…).

nok-ko · 2022-05-11T23:20:50Z

main.js

+	// Redirect to profile page if logged in
+	if (req.session.loggedIn) {
+		res.redirect("/profile");
+		return;
+	} 


Closes #21.

…/ShakeGuard into Jay-Alex-Profile-Page

…Profile Picture Upload.

…r to center

nok-ko

Review made on my phone, pardon some of the curt comments (and ask clarification if you need!)

Will come back with another review once I'm at a computer.

main.js

nok-ko · 2022-05-12T22:54:35Z

main.js

+
+	// based on what is present in the update query, 
+	const updateQuery = {};
+	if (req.body.email) {


You probably don't need the ifs here: IIRC nothing happens if you set an object's property to undefined.

First comment was sort of wrong, but this can be made terser:

const queryValues = { emailAddress: req.body.email, name: req.body.name, // admittedly, the next line is kind of silly, maybe keep the original password code: pwd: req.body.pwd !== undefined ? (await bcrypt.hash(req.body.pwd, 10)) : undefined }; const updateQuery = Object.fromEntries( Object.entries(queryValues).filter(e => e[1] !== undefined) );

nok-ko · 2022-05-12T23:07:00Z

main.js

+
+	// get user from db
+	const filterQuery = {emailAddress: req.session.email};
+	const userResults = await db.collection('BBY-6_users').find(filterQuery).toArray();


Wonder if it's better to skip this, and just do a single request to the database, catch this error case on the update failing. Reasoning being that two requests are slower than one, and that there's actually a weird race condition here, if someone deletes a user you want to update, just after you run the "user exists" query.

Tracked in #37, resolving this for now.

nok-ko · 2022-05-12T23:07:58Z

main.js

+	} 
+	const results = await db.collection('BBY-6_users').updateOne({emailAddress:req.session.email}, {
+		$set: {
+			avatarURL: {


Probably shouldn't call this “avatarURL” any more.

Tracked in #36, resolving for now.

nok-ko · 2022-05-12T23:10:08Z

public/css/index.css

@@ -119,3 +125,15 @@ body {
 .fail {
 	color: red;
 }
+
+#Footer-Links {


We should make sure all of our CSS follows the same naming convention sometime.

nok-ko · 2022-05-12T23:18:31Z

public/js/user-profile.js

+}
+
+async function executeUpdate(){
+	checkForInvalidInput(FullNameInput);


Could shorten this to:

[fooInput, barInput, bazInput].map(checkForInvalidInput);

nok-ko · 2022-05-12T23:19:13Z

public/js/user-profile.js

+    }
+}
+
+function checkForInvalidInput(input) {


Better name might be validate.

nok-ko · 2022-05-12T23:20:21Z

public/js/user-profile.js

+    x.value = '';
+}
+
+function editFullName(){


These editFoo functions could be generated by a higher order function, don't repeat yourself.

E.g:

function makeEditAction(inputEl, defaultValue){ return function () { if (inputEl.disabled){ inputEl.disabled= false; inputEl.focus(); inputEl.onfocus = clearInput(inputEl); return; } inputEl.disabled = true; inputEl.value = defaultValue; } } const editEmail = makeEditAction(EmailInput, userEmail); const editFullName = makeEditAction(FullNameInput, userName); const editPassword = makeEditAction(PasswordInput, "12345");

nok-ko · 2022-05-12T23:21:07Z

public/js/user-profile.js

+
+    const ResponseText = await response.text(); 
+	const feedback = document.getElementById("Update-Error");
+    switch(ResponseText){


Could replace the switch... case with an object mapping error IDs to strings/actions.

nok-ko · 2022-05-12T23:22:48Z

main.js

+}
+
+// Load a HTML component 
+// baseDOM is the return value of `new JSDOM(htmlFile)` 


Just use JSDoc comment style.

nok-ko · 2022-05-12T23:29:18Z

Also please try to make sure all of your functions have header comments describing what they do, ideally in JSDoc format

nok-ko

More substantial review. General recommendations aside from the specific stuff:

Look for reused or “magic” CSS values, put them in CSS custom properties.
Add doc comments to any functions longer than a couple of lines. (Not a big priority, but nice going forward.)
Avoid readFileSync, use the functions in the node:fs/promises package instead.

Other side stuff:

Default profile picture could be an SVG instead of what we have now. Okay for now, since it's just a placeholder.
Consider putting some of the clientside JS in separate ES6 modules, especially stuff we might re-use.
I don't quite understand how the error flag system works, so can't really offer cogent criticism… It seems to make sense, but I'm not sure.

Otherwise, I haven't looked at the actual UX of the thing, just the code… something to do later. 😅

nok-ko · 2022-05-13T02:20:13Z

html/user-profile.html

+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <link href="images/comp2800_logo_favicon.ico" rel="icon" type="image/x-icon" />
+    <link href="css/user-profile.css" rel="stylesheet"/>


Kind of confusing that we have both a user-profile.html + user-profile.css and a profile.html + profile.css.

Alex says they're not used anymore – will remove.

nok-ko · 2022-05-13T02:23:12Z

main.js

+const loadHTMLComponent = (baseDOM, placeholderSelector, componentSelector, componentLocation) => {
+	const document = baseDOM.window.document;
+	const placeholder = document.querySelector(placeholderSelector);
+	const html = fs.readFileSync(componentLocation, "utf8");


I think we ought to stop using fs.*Sync methods - we can just await the async versions from fs/promises when we need to, it's just as easy to use.

// at top of file: import {readFile} from 'node:fs/promises';

Suggested change

const html = fs.readFileSync(componentLocation, "utf8");

const html = await readFile(componentLocation, "utf8");

As for why: readFileSync() blocks the event loop, fs/promises' readFile() doesn't.

nok-ko · 2022-05-13T02:24:29Z

main.js

+	let index = new JSDOM(doc);
+
+	// Add the footer
+	index = loadHTMLComponent(index, "footer", "footer", "./html/footer.html");


Why reassign index? Just use two different variables:

Suggested change

let index = new JSDOM(doc);

// Add the footer

index = loadHTMLComponent(index, "footer", "footer", "./html/footer.html");

const dom = new JSDOM(doc);

// Add the footer

const index = loadHTMLComponent(dom, "footer", "footer", "./html/footer.html");

I can't really see a reason for the other way, maybe I'm missing something.

My preference is for that suggested version – then the types of the two variables are always the same as initialized.

nok-ko · 2022-05-13T02:26:56Z

main.js

 });
+app.get('/profile-details', async(req, res) => {


You don't seem to (explicitly) cover the case when the user is not logged in, in this endpoint.

nok-ko · 2022-05-13T03:07:25Z

main.js

+
+	// based on what is present in the update query, 
+	const updateQuery = {};
+	if (req.body.email) {


First comment was sort of wrong, but this can be made terser:

const queryValues = { emailAddress: req.body.email, name: req.body.name, // admittedly, the next line is kind of silly, maybe keep the original password code: pwd: req.body.pwd !== undefined ? (await bcrypt.hash(req.body.pwd, 10)) : undefined }; const updateQuery = Object.fromEntries( Object.entries(queryValues).filter(e => e[1] !== undefined) );

nok-ko · 2022-05-13T03:56:33Z

public/js/user-profile.js

+		payload['pwd'] = PasswordInput.value;
+	}
+	if(AvatarInput.files[0]) {
+		// Make the POST call seperatly


Suggested change

// Make the POST call seperatly

// Make the POST call separately

nok-ko · 2022-05-13T03:56:48Z

public/js/user-profile.js

+			body: formData
+		})
+		const responseText = await response.text();
+		console.log(responseText);


Suggested change

console.log(responseText);

nok-ko · 2022-05-13T04:06:11Z

public/js/user-profile.js

+    const ResponseText = await response.text(); 
+	const feedback = document.getElementById("Update-Error");
+    switch(ResponseText){
+        case "userUpdated":
+            // Successfully updated
+			feedback.innerText = "Successfully Updated Profile";
+			feedback.style.color = "green";
+            break;
+        case "emailInUse":
+            // Email already in use.
+			feedback.innerText = "That email is already in use";
+			feedback.style.color = "red";
+            break;
+        default:
+            feedback.innerText = "There was an issue on the server";
+			feedback.style.color = "red";
+    }


What I meant:

Suggested change

const ResponseText = await response.text();

const feedback = document.getElementById("Update-Error");

switch(ResponseText){

case "userUpdated":

// Successfully updated

feedback.innerText = "Successfully Updated Profile";

feedback.style.color = "green";

break;

case "emailInUse":

// Email already in use.

feedback.innerText = "That email is already in use";

feedback.style.color = "red";

break;

default:

feedback.innerText = "There was an issue on the server";

feedback.style.color = "red";

}

const ResponseText = await response.text();

const feedbackEl = document.getElementById("Update-Error");

const [newText, newStyle] = {

"userUpdated": ["Successfully updated profile.", { color: 'green' }],

"emailInUse": ["That email is already in use.", { color: 'red' }],

"_default": ["There was an issue on the server.", { color: 'red' }]

}[ResponseText ?? '_default'];

feedback.textContent = newText;

Object.assign(feedback.style, newStyle);

And the even better thing to do would be to put that error message style (color: red/color: green) into a CSS class so that we can edit it from the stylesheet and not have too much markup in the JavaScript.

nok-ko · 2022-05-13T04:11:34Z

public/js/user-profile.js

+    x.value = '';
+}
+
+function editFullName(){


E.g:

function makeEditAction(inputEl, defaultValue){ return function () { if (inputEl.disabled){ inputEl.disabled= false; inputEl.focus(); inputEl.onfocus = clearInput(inputEl); return; } inputEl.disabled = true; inputEl.value = defaultValue; } } const editEmail = makeEditAction(EmailInput, userEmail); const editFullName = makeEditAction(FullNameInput, userName); const editPassword = makeEditAction(PasswordInput, "12345");

public/js/user-profile.js

nok-ko · 2022-05-13T16:50:23Z

Workflow runs are failing because templates are not masked from the HTML Validator. Sigh, will fix later.

AlexLLKong · 2022-05-13T17:26:56Z

Post Code Review Changes

Move html components into separate folder and naming convention.
Credentials in the README.md for reg user and admin.
Strip out ready function.
Strip out profile.html and profile.css.
Use await for fs.
Add session check for /profile-details.
- Also refactor frequently-used redirect/error check code into functions: checkLoginError and redirectToLogin.
- Should really be Express middlewares.
Strip out /signup.
Check for console.logs.
Fix stretching on profile pictures

nok-ko · 2022-05-13T17:54:38Z

Code review with Alex

nok-ko

Wow, even made the checks pass! Awesome work, guys.

AlexLLKong · 2022-05-13T18:28:13Z

Wow, even made the checks pass! Awesome work, guys.

wait why did the checks pass? I don't think anything was done to make them pass

nok-ko · 2022-05-13T18:29:15Z

wait why did the checks pass? I don't think anything was done to make them pass

'Cause the workflow was only set up to check files in html/ :)

nok-ko · 2022-05-13T20:03:33Z

Code review and merge branches (1hr) (1) [4]

As a User, I'd like to be able to have a customized Profile.

AlexLLKong assigned AlexLLKong and ArtfulJW May 9, 2022

nok-ko changed the base branch from main to dev May 10, 2022 00:38

ArtfulJW and others added 3 commits May 10, 2022 09:07

Add HTML templating for profile page

c706575

Co-authored-by: Alex Kong <[email protected]>

Add basic footer, refactor HTML component loading, add placeholders t…

b971933

…o some file for footer and header Co-authored-by: Alex Kong <[email protected]>

Iteration 1 of Profile Page Complete.

bd06ee8

AlexLLKong force-pushed the Jay-Alex-Profile-Page branch from a90f076 to bd06ee8 Compare May 10, 2022 16:09

ArtfulJW and others added 7 commits May 10, 2022 11:54

Javascript Implementation 1 for Profile Page

011f149

Add endpoint patch /profile and link frontend and backend

cf36d36

Redirect user to profile page if logged in and trying to access the l…

91074fc

…ogin page

Cleaning up Profile Page and Login Page

252cd99

Iteration 2: Cleaning up CSS and HTML for Profile Page and Login Page

9060a63

Add change avatar functionality

8d834a8

Iteration 3: Cleaning up User Experience submitting form

3feeb41

nok-ko reviewed May 11, 2022

View reviewed changes

Add feedback for users when they enter invalid input

556d929

nok-ko reviewed May 11, 2022

View reviewed changes

This was referenced May 11, 2022

Give feedback to user on incorrect login #23

Closed

Make sure when user logs in, their name/info is displayed in a profile page. #32

Closed

ArtfulJW added 6 commits May 11, 2022 22:38

Issue #33: Implement Media Queries for all Pages

5d04f50

Merge branch 'Jay-Alex-Profile-Page' of https://github.com/ShakeGuard…

1e5065e

…/ShakeGuard into Jay-Alex-Profile-Page

Add more Media Query on Login Page

961e311

Profile Page: Finished Restructuring HTML. Finished User Feedback on …

026afa4

…Profile Picture Upload.

Adjusted the padding for 'update-error'

dd7aa42

Polished Profile Page, Adjusted Margins to fit mobile, Adjusted Foote…

c76a641

…r to center

AlexLLKong requested a review from nok-ko May 12, 2022 21:37

nok-ko reviewed May 12, 2022

View reviewed changes

nok-ko requested changes May 13, 2022

View reviewed changes

This was referenced May 13, 2022

Rename avatarURL field in schema #36

Open

Make one request instead of two when updating stuff in the DB #37

Open

Handle invalid fields in PATCHes #38

Open

Remove leftover comments.

f922b07

nok-ko mentioned this pull request May 13, 2022

Use one <form> element instead of many in profile-component.html #39

Closed

Use JSDoc style in long loadHeaderFooter comment.

48cae95

nok-ko mentioned this pull request May 13, 2022

Mask template HTML from the Validator Workflow #40

Closed

Solved image stretching issue on user uploaded profile picture

d541a81

AlexLLKong and others added 2 commits May 13, 2022 11:09

Post code review changes

32bbe22

Merge branch 'dev' into Jay-Alex-Profile-Page

bacf49e

AlexLLKong marked this pull request as ready for review May 13, 2022 18:13

AlexLLKong requested a review from nok-ko May 13, 2022 18:18

nok-ko approved these changes May 13, 2022

View reviewed changes

AlexLLKong merged commit 0c1f649 into dev May 13, 2022

nok-ko pushed a commit that referenced this pull request May 20, 2022

Merge pull request #26 from ShakeGuard/Jay-Alex-Profile-Page

14225e7

As a User, I'd like to be able to have a customized Profile.

	const html = fs.readFileSync(componentLocation, "utf8");
	const html = await readFile(componentLocation, "utf8");

	// Make the POST call seperatly
	// Make the POST call separately

As a User, I'd like to be able to have a customized Profile. #26

As a User, I'd like to be able to have a customized Profile. #26

Conversation

AlexLLKong commented May 9, 2022 • edited Loading

nok-ko commented May 10, 2022

Choose a reason for hiding this comment

nok-ko May 11, 2022 • edited Loading

Choose a reason for hiding this comment

nok-ko 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

nok-ko May 12, 2022 • edited Loading

Choose a reason for hiding this comment

nok-ko May 13, 2022 • edited Loading

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

nok-ko May 12, 2022 • edited Loading

Choose a reason for hiding this comment

nok-ko commented May 12, 2022

nok-ko 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nok-ko commented May 13, 2022

AlexLLKong commented May 13, 2022

Post Code Review Changes

nok-ko commented May 13, 2022

nok-ko left a comment

Choose a reason for hiding this comment

AlexLLKong commented May 13, 2022 • edited Loading

nok-ko commented May 13, 2022

nok-ko commented May 13, 2022

AlexLLKong commented May 9, 2022 •

edited

Loading

nok-ko May 11, 2022 •

edited

Loading

nok-ko May 12, 2022 •

edited

Loading

nok-ko May 13, 2022 •

edited

Loading

nok-ko May 12, 2022 •

edited

Loading

AlexLLKong commented May 13, 2022 •

edited

Loading