#167484590: create user profiles upon successful registration #18

raymond42 · 2019-08-13T03:40:20Z

What does this PR do?

It implements:

getting user profile

update user's profile

Description of Task to be done?

Adding profile.js file in controller directory
Adding editProfile.js file in middleware directory
Adding userprofile.test.js file in test directory
Adding cloudinary to save our photos

How should this be manually tested?

Clone this branch.
Do npm install
Create an account by signing up using localhost:3000/api/v1/users using POST method.
4.- add this env variable for cloudinary into your dotenv
- cloudinarySecretKey
- cloudinaryApiKey
- cloudinaryName
Using your username and token update your profile using this route localhost:3000/api/v1/profile/username using PUT method.
using formdata you can update the following information with this example:

userName: "ray32",
bio: "Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old",
image: upload image file

What are the relevant pivotal tracker stories?

create user profiles upon successful registration

screen shots

src/tests/userprofile.test.js

hound · 2019-08-13T03:40:29Z

src/middleware/editProfile.js

@@ -0,0 +1,17 @@
+import model from '../sequelize/models';


Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

hound · 2019-08-13T03:40:29Z

src/controllers/profile.js

@@ -0,0 +1,110 @@
+import { config } from 'dotenv';


Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

hound · 2019-08-13T03:40:29Z

src/config/passport/passport.js

@@ -0,0 +1,48 @@
+import passport from 'passport';


Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

harerakalex · 2019-08-13T08:18:33Z

src/controllers/profile.js

+import { User } from '../sequelize/models';
+
+config();
+cloudinary.config({


This cloudinary config would be better if you put it in config folder. And rename the name of this controller to profileController

on it. thanks

src/helpers/schema/validationRules.js

malaba6 · 2019-08-13T08:41:51Z

src/middleware/editProfile.js

+
+  if (trueUsername !== suggestedUsername) {
+    return res.status(403).send({
+      error: 'sorry! you can not edit the profile that is not yours'


For this error message. I think it'd be better if the message started with a capital letter and instead of can not, cannot would be a better option in this case Reference to this article

on it. thanks

malaba6

I have noticed that actually, you can upload a picture twice. Well, that is disastrous for our account because we are using a limited subscription. If you could make sure that when the user uploads a picture, it checks if the picture already exists. if so, it just uses the uploaded one.

raymond42 · 2019-08-13T11:39:17Z

I have noticed that actually, you can upload a picture twice. Well, that is disastrous for our account because we are using a limited subscription. If you could make sure that when the user uploads a picture, it checks if the picture already exists. if so, it just uses the uploaded one.

yes, you are right. let me implement that. thanks

malaba6 · 2019-08-13T12:57:54Z

src/controllers/profile.js

+  cloud_name: process.env.cloudinaryName,
+  api_key: process.env.cloudinaryApiKey,
+  api_secret: process.env.cloudinarySecretKey,
+});


I think it would be good that we all adhere to the convention on naming environment variable by using _ instead of camel case like in this case.

So instead of cloudinaryName, u can use CLOUDINARY_NAME

I think it would be good that we all adhere to the convention on naming environment variable by using _ instead of camel case like in this case.

So instead of cloudinaryName, u can use CLOUDINARY_NAME

Thank you. On it now

placideirandora

You should use the same route parameter name for consistency. You should use the one in lower case: username.

You should capitalize the variable name: userfound as userFound because it is made up with 2 words.

I think you should protect the route for retrieving a user profile because I was able to retrieve the route without having logged in.

I think you should remove the status as we agreed. This error message got triggered when I tried to update my profile.

You should remove those purple characters for the error message to be easily read.

raymond42 · 2019-08-13T13:44:33Z

You should use the same route parameter name for consistency. You should use the one in lower case: username.

You should capitalize the variable name: userfound as userFound because it is made up with 2 words.

I think you should protect the route for retrieving a user profile because I was able to retrieve the route without having logged in.

I think you should remove the status as we agreed. This error message got triggered when I tried to update my profile.

You should remove those purple characters for the error message to be easily read.

thanks for the feedback. I am going to work on them. However, on feedback number 3, I think It's not necessary to protect the route because you are limiting the user to get the other profiles of the people. If a person could get or read other people's article also he should be able to get their profiles. And on feedback 4, because It's not me who worked on authentications, I did not want to touch into other people's file. So We agreed with the TTL to make the chore of cleaning up the codebase

hound · 2019-08-13T16:01:50Z

src/controllers/profileController.js

@@ -0,0 +1,110 @@
+import { config } from 'dotenv';


Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

placideirandora · 2019-08-13T19:04:14Z

You should use the same route parameter name for consistency. You should use the one in lower case: username.

You should capitalize the variable name: userfound as userFound because it is made up with 2 words.

I think you should protect the route for retrieving a user profile because I was able to retrieve the route without having logged in.

I think you should remove the status as we agreed. This error message got triggered when I tried to update my profile.

You should remove those purple characters for the error message to be easily read.

thanks for the feedback. I am going to work on them. However, on feedback number 3, I think It's not necessary to protect the route because you are limiting the user to get the other profiles of the people. If a person could get or read other people's article also he should be able to get their profiles. And on feedback 4, because It's not me who worked on authentications, I did not want to touch into other people's file. So We agreed with the TTL to make the chore of cleaning up the codebase

About the feedback 3, The user will be able to retrieve other user's profiles as long as they have logged in. Therefore, Protecting the route will not block the user from retrieving others' profiles.

About the feedback 4, You are allowed to do that because I also did it when I removed the status messages from Malaba's user controller. I think it is okay.

raymond42 · 2019-08-14T08:52:12Z

You should use the same route parameter name for consistency. You should use the one in lower case: username.

You should capitalize the variable name: userfound as userFound because it is made up with 2 words.

I think you should protect the route for retrieving a user profile because I was able to retrieve the route without having logged in.

I think you should remove the status as we agreed. This error message got triggered when I tried to update my profile.

You should remove those purple characters for the error message to be easily read.

thanks for the feedback. I am going to work on them. However, on feedback number 3, I think It's not necessary to protect the route because you are limiting the user to get the other profiles of the people. If a person could get or read other people's article also he should be able to get their profiles. And on feedback 4, because It's not me who worked on authentications, I did not want to touch into other people's file. So We agreed with the TTL to make the chore of cleaning up the codebase

About the feedback 3, The user will be able to retrieve other user's profiles as long as they have logged in. Therefore, Protecting the route will not block the user from retrieving others' profiles.

About the feedback 4, You are allowed to do that because I also did it when I removed the status messages from Malaba's user controller. I think it is okay.

@placideirandora I talked to TTL about this. and we agreed to make a chore of doing that

placideirandora · 2019-08-14T09:07:16Z

You should use the same route parameter name for consistency. You should use the one in lower case: username.

You should capitalize the variable name: userfound as userFound because it is made up with 2 words.

I think you should protect the route for retrieving a user profile because I was able to retrieve the route without having logged in.

I think you should remove the status as we agreed. This error message got triggered when I tried to update my profile.

You should remove those purple characters for the error message to be easily read.

thanks for the feedback. I am going to work on them. However, on feedback number 3, I think It's not necessary to protect the route because you are limiting the user to get the other profiles of the people. If a person could get or read other people's article also he should be able to get their profiles. And on feedback 4, because It's not me who worked on authentications, I did not want to touch into other people's file. So We agreed with the TTL to make the chore of cleaning up the codebase

About the feedback 3, The user will be able to retrieve other user's profiles as long as they have logged in. Therefore, Protecting the route will not block the user from retrieving others' profiles.
About the feedback 4, You are allowed to do that because I also did it when I removed the status messages from Malaba's user controller. I think it is okay.

@placideirandora I talked to TTL about this. and we agreed to make a chore of doing that

Ok. It is because when I received the feedback about email confirmation link, I asked if I could modify other's methods for me to be able to implement the feedback and he told that I could do that. For me, I removed them already. So, That chore will be only for removing that one that I have mentioned for you to remove. A chore to remove one status message.

raymond42 added the Need Reviews Feedback to PR raised label Aug 13, 2019

raymond42 requested review from mkiterian, Jkadhuwa, placideirandora, harerakalex and malaba6 August 13, 2019 03:40

hound bot reviewed Aug 13, 2019

View reviewed changes

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 13, 2019 03:40 Inactive

harerakalex suggested changes Aug 13, 2019

View reviewed changes

malaba6 reviewed Aug 13, 2019

View reviewed changes

placideirandora suggested changes Aug 13, 2019

View reviewed changes

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 13, 2019 16:01 Inactive

hound bot reviewed Aug 13, 2019

View reviewed changes

raymond42 force-pushed the ft-user-profile-#167484590 branch from 74ac92d to 301d266 Compare August 13, 2019 16:07

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 13, 2019 16:07 Inactive

raymond42 force-pushed the ft-user-profile-#167484590 branch from 301d266 to 45b5bd1 Compare August 13, 2019 16:16

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 13, 2019 16:16 Inactive

raymond42 force-pushed the ft-user-profile-#167484590 branch from 45b5bd1 to 1b8176f Compare August 13, 2019 16:24

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 13, 2019 16:24 Inactive

raymond42 force-pushed the ft-user-profile-#167484590 branch from 1b8176f to ab19b45 Compare August 14, 2019 15:35

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 14, 2019 15:35 Inactive

raymond42 changed the title ~~167484590: create user profiles upon successful registration~~ #167484590: create user profiles upon successful registration Aug 14, 2019

raymond42 force-pushed the ft-user-profile-#167484590 branch from ab19b45 to bdf4fbe Compare August 14, 2019 20:23

raymond42 temporarily deployed to codinggeeks-ah-backnd-st-pr-18 August 14, 2019 20:23 Inactive

harerakalex approved these changes Aug 15, 2019

View reviewed changes

raymond42 added 2 commits August 15, 2019 12:28

[Start #167484590] add user profile

20a340a

[Fixes #167484590] implementing feedback

c54cfb1

raymond42 force-pushed the ft-user-profile-#167484590 branch from bdf4fbe to c54cfb1 Compare August 15, 2019 10:29

raymond42 deployed to codinggeeks-ah-backnd-st-pr-18 August 15, 2019 10:29 View deployment

mkiterian merged commit 25b07ca into development Aug 15, 2019

mkiterian deleted the ft-user-profile-#167484590 branch August 15, 2019 12:29

placideirandora added finished and removed Need Reviews Feedback to PR raised labels Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#167484590: create user profiles upon successful registration #18

#167484590: create user profiles upon successful registration #18

raymond42 commented Aug 13, 2019 •

edited

Loading

hound bot Aug 13, 2019

hound bot Aug 13, 2019

hound bot Aug 13, 2019

harerakalex Aug 13, 2019

raymond42 Aug 13, 2019

malaba6 Aug 13, 2019

raymond42 Aug 13, 2019

malaba6 left a comment

raymond42 commented Aug 13, 2019 •

edited

Loading

malaba6 Aug 13, 2019

raymond42 Aug 13, 2019

placideirandora left a comment •

edited

Loading

raymond42 commented Aug 13, 2019 •

edited by placideirandora

Loading

hound bot Aug 13, 2019

placideirandora commented Aug 13, 2019

raymond42 commented Aug 14, 2019 •

edited by placideirandora

Loading

placideirandora commented Aug 14, 2019

#167484590: create user profiles upon successful registration #18

#167484590: create user profiles upon successful registration #18

Conversation

raymond42 commented Aug 13, 2019 • edited Loading

What does this PR do?

Description of Task to be done?

How should this be manually tested?

What are the relevant pivotal tracker stories?

screen shots

hound bot Aug 13, 2019

Choose a reason for hiding this comment

hound bot Aug 13, 2019

Choose a reason for hiding this comment

hound bot Aug 13, 2019

Choose a reason for hiding this comment

harerakalex Aug 13, 2019

Choose a reason for hiding this comment

raymond42 Aug 13, 2019

Choose a reason for hiding this comment

malaba6 Aug 13, 2019

Choose a reason for hiding this comment

raymond42 Aug 13, 2019

Choose a reason for hiding this comment

malaba6 left a comment

Choose a reason for hiding this comment

raymond42 commented Aug 13, 2019 • edited Loading

malaba6 Aug 13, 2019

Choose a reason for hiding this comment

raymond42 Aug 13, 2019

Choose a reason for hiding this comment

placideirandora left a comment • edited Loading

Choose a reason for hiding this comment

raymond42 commented Aug 13, 2019 • edited by placideirandora Loading

hound bot Aug 13, 2019

Choose a reason for hiding this comment

placideirandora commented Aug 13, 2019

raymond42 commented Aug 14, 2019 • edited by placideirandora Loading

placideirandora commented Aug 14, 2019

raymond42 commented Aug 13, 2019 •

edited

Loading

raymond42 commented Aug 13, 2019 •

edited

Loading

placideirandora left a comment •

edited

Loading

raymond42 commented Aug 13, 2019 •

edited by placideirandora

Loading

raymond42 commented Aug 14, 2019 •

edited by placideirandora

Loading