-
Notifications
You must be signed in to change notification settings - Fork 7
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
#165412925 #165412925 #165412938 Users follow and unfollow each other. #53
Conversation
8d90064
to
51c68f9
Compare
* @returns {object} it return true if account verified otherwise it return false | ||
*/ | ||
static async reset(req, res) { | ||
return res.status(status.OK).send({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought the reset link is sent via email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the reset link sent via email, I added this controller to return the message when user click on link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
src/helpers/isActiveUser.js
Outdated
if (checkUser.isActive) { | ||
return true; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean to remove return false or all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
40216c9
to
e374415
Compare
69f3b28
to
6f46aa0
Compare
de51da0
to
30b2b35
Compare
package.json
Outdated
"scss": "npm-sass templates/sass/style.scss templates/css/style.css", | ||
"db:migrate": "./node_modules/.bin/sequelize db:migrate", | ||
"db:migrate:seed": "./node_modules/.bin/sequelize db:seed:all", | ||
"db:seed": "./node_modules/.bin/sequelize db:seed --seed 20190605075225-permissions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the seeding done form one file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only thing that we need to seed for now is permissions, but we can still add other seed when they are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just have it as seed all and remove other seeder files.
@@ -7,11 +7,11 @@ | |||
"test": "NODE_ENV=test sequelize db:migrate:undo:all && npm run db:migrate:test && npm run db:migrate:seed:test && NODE_ENV=test nyc mocha --require @babel/register --timeout 30000 src/tests/index.js", | |||
"cover": "npm test && nyc report --reporter=text-lcov | coveralls", | |||
"coveralls": "cat coverage/lcov.info | coveralls", | |||
"start": "npm run db:migrate && babel-node src/index.js", | |||
"dev": "nodemon src/index.js --exec babel-node --presets @babel/preset-env", | |||
"start": "concurrently \"npm run db:migrate && npm run db:seed\" \"babel-node src/index.js\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we need these because we need to run multiple commands concurrently and skip other when failing
* @param {object} res | ||
* @returns {object} it return true if account verified otherwise it return false | ||
*/ | ||
static async reset(req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the token should be sent to user email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the token sent to the user email however, I added this function to return it on the body I can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be returned. Just send email.
* @param {object} res server response | ||
* @returns {object} true | ||
*/ | ||
static async follow(req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the follow functionality under auth controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I am going to migrate it to user controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved, know the follow's functionality is located to the user file
followers: followers.map(follower => delete follower.get().followedUser && follower) | ||
}) | ||
: res.status(status.NOT_FOUND).json({ | ||
errors: { follows: 'You do not have followers for know' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeo. Just have it as you don't have followers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
return followers.length | ||
? res.status(status.OK).json({ | ||
message: 'Followers', | ||
followers: followers.map(follower => delete follower.get().followedUser && follower) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we deleting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the followed user here because I wont to display only the user that follows me in this controller and in the model I get both followers and following
src/controllers/UserController.js
Outdated
const role = 'normal'; | ||
const findAll = await User.getAllUser({ role }); | ||
static async getAll(req, res) { | ||
const role = req.user.role === 'normal' ? 'normal' : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have the role check here. we have to keep them in a helper file
templates/html/follow.html
Outdated
|
||
switch (verb) { | ||
case 'PATCH': | ||
console.log(URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
2aca6c4
to
5989bbc
Compare
* @return {object} return all users in database | ||
*/ | ||
static async getAll(req, res) { | ||
const [offset, limit] = [req.query.offset || 0, req.query.limit || 20]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines are not covered in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
3d3753a
to
4e7181d
Compare
a413ecc
to
8bf2e68
Compare
8bf2e68
to
0961e22
Compare
What does this PR do?
Description of Task to be completed?
creating the following endpoints:
POST /api/v1/users/follow
POST /api/v1/users/unfollow
GET /api/v1/users/followers
GET /api/v1/users/following
POST /api/v1/users/ to register new user
DELETE /api/v1/users/:id to delete a user
UPDATE /api/v1/users/:id to update a user information
GET /api/v1/users/ to fetch all users
GET /api/v1/users/:id to fetch a single user
1. Send activation link to the provided email to confirm registration
2. when user click to the activation link then account activated
How should this be manually tested?
clone the repo locally and user postman to test endpoints
Any background context you want to provide?
What are the relevant pivotal tracker stories?