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

#168781679 User Login #19

Merged
merged 2 commits into from
Oct 10, 2019
Merged

#168781679 User Login #19

merged 2 commits into from
Oct 10, 2019

Conversation

alainmateso
Copy link
Contributor

@alainmateso alainmateso commented Oct 8, 2019

What does this PR do?

Add user login functionality.

Description of Task to be completed?

  • creates an endpoint for a user to login
  • gives user a token upon successful signin
  • provides tests for login functionality

How should this be manually tested?

  • Clone the repository from here
  • Browse to the repo directory
  • navigate to ft-user-login-168781679 branch
  • Run npm install
  • run sequelize db:migrate
  • run sequelize db:seed:all
  • run npm run dev
  • In Postman Provide email and password in the body
  • send a POST request to http://localhost:3000/api/v1/users/login

Any background context you want to provide?

N/A

What are the relevant pivotal tracker stories?

#168781679

Screenshots (if appropriate)

Screen Shot 2019-10-10 at 20 09 32

Screen Shot 2019-10-10 at 20 10 01

Screen Shot 2019-10-10 at 20 10 22

Questions:

N/A

@alainmateso alainmateso added the WIP Work In Progress label Oct 8, 2019
@alainmateso alainmateso self-assigned this Oct 8, 2019
@alainmateso alainmateso force-pushed the ft-user-login-168781679 branch 5 times, most recently from 63f3f8b to e77d5b3 Compare October 10, 2019 09:35
@alainmateso alainmateso added Ready For Review and removed WIP Work In Progress labels Oct 10, 2019
@alainmateso alainmateso force-pushed the ft-user-login-168781679 branch 4 times, most recently from 409cfbe to 3765702 Compare October 10, 2019 10:48
@alainmateso alainmateso added WIP Work In Progress and removed Ready For Review labels Oct 10, 2019
- creates endpoint for user to login
- gives user a token upon signin
[Finishes#168781679]
Copy link
Contributor

@nignanthomas nignanthomas left a comment

Choose a reason for hiding this comment

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

Great job @alainmateso so far
Just a few changes though

  • Remove that unncessary variable in .env.example
  • Update the PR body to the new login endpoint url /api/v1/users/login
  • Update the screenshots too

.env.example Outdated Show resolved Hide resolved
Copy link
Contributor

@Cheza-Dzabala Cheza-Dzabala left a comment

Choose a reason for hiding this comment

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

Consider refactoring your controller


static async signIn(req, res) {
const { email, password } = req.body;
const user = await models.users.findOne({ where: { email } });
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line here

const user = await models.users.findOne({ where: { email } });
if (!user) {
return responseError(res, 400, strings.users.error.LOGIN_FAILURE);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line here

import app from '../index';
import strings from '../utils/stringsUtil';
import mockData from './mockData/mockData';

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

Copy link
Contributor

@nakiwuge nakiwuge left a comment

Choose a reason for hiding this comment

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

Good job @alainmateso . The feature works as expected just a few changes are required to improve readability.

- creates endpoint for user to login
- gives user a token upon signin
- adheres to the current structre
[Finishes#168781679]
@nakiwuge nakiwuge merged commit 65a76d3 into develop Oct 10, 2019
@alainmateso alainmateso deleted the ft-user-login-168781679 branch November 13, 2019 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants