Skip to content

New user registration route 2153372#4

Closed
dcDalin wants to merge 5 commits into
masterfrom
ch-setup-Flask-dev-environment-2153372
Closed

New user registration route 2153372#4
dcDalin wants to merge 5 commits into
masterfrom
ch-setup-Flask-dev-environment-2153372

Conversation

@dcDalin

@dcDalin dcDalin commented Mar 6, 2018

Copy link
Copy Markdown
Owner

What does this PR do?

Allows new users to create an account

Description of Task to be completed?

A new user should be able to fill in their credentials and create an account

How should this be manually tested?

Clone the repository, create your virtual environment, run requirements.txt then run app.py

Any background context you want to provide?

What are the relevant pivotal tracker stories?

#155732737


def __init__(self):
'''Constructor method, will have dictionaries in a list'''
self.users_counter = 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why have this? Can't you just get the length of the users list?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Performance I guess. Checking the value of the counter then adding 1 instead of checking for the length of the list each time.


from flask import request
from flask_restplus import Resource
from WeConnect_RestAPI.api.we_connect.serializers import new_user_structure, login_structure, logout_structure, reset_pass_structure

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have you installed pylint?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ensure your code adheres to PEP8 standards


from WeConnect_RestAPI.app import app as the_app, initialize_app

class TestSomething(unittest.TestCase):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use a more meaningful and descriptive test file name and testcase name

@mkiterian mkiterian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cached files should be added to gitignore


@api.expect(new_user_structure)
@api.doc(responses={403: 'Not Authorized', 404: 'Not Found', 201: 'User added', 400: 'Bad request'})
#@api.expect(NEW_USER_STRUCTURE, validate=True, code=201)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do not push commented out code

"""
Returns all users.
"""
return initWeConnectUsers.show_all_users() No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use snake_case not camelCase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants