Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fixes #14 - replace http codes by constants
  • Loading branch information
Angeluz-07 committed Mar 25, 2020
commit 587855b661b8f836b0ca0006c4c64c9c57e24ee0
4 changes: 1 addition & 3 deletions time_tracker_api/activities/activities_model.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
from flask import Flask

from time_tracker_api.database import CRUDDao


class ActivityDao(CRUDDao):
pass


def create_dao(app: Flask) -> ActivityDao:
def create_dao() -> ActivityDao:
from time_tracker_api.sql_repository import db
from time_tracker_api.sql_repository import SQLCRUDDao, AuditedSQLModel, SQLModel

Expand Down
30 changes: 15 additions & 15 deletions time_tracker_api/activities/activities_namespace.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from faker import Faker
from flask_restplus import fields, Resource, Namespace
from flask_restplus._http import HTTPStatus

from time_tracker_api import flask_app
from time_tracker_api.api import audit_fields
from time_tracker_api.activities.activities_model import create_dao

Expand Down Expand Up @@ -42,49 +42,49 @@
activity_response_fields
)

activity_dao = create_dao(flask_app)
activity_dao = create_dao()


@ns.route('')
class Activities(Resource):
@ns.doc('list_activities')
@ns.marshal_list_with(activity, code=200)
@ns.marshal_list_with(activity, code=HTTPStatus.OK)
Copy link
Contributor

Choose a reason for hiding this comment

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

by default it is OK. So this is redundant.

def get(self):
"""List all activities"""
return activity_dao.get_all(), 200
return activity_dao.get_all(), HTTPStatus.OK

@ns.doc('create_activity')
@ns.response(400, 'Bad request')
@ns.response(HTTPStatus.BAD_REQUEST, 'Bad request')
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to put a message, lets better put one customized, like:

Invalid format or structure of the attributes of the activity

Also, it would be nice to specify that a CONFLICT may occur if there is already an activity with the same name

    @ns.response(HTTPStatus.CONFLICT, 'This activity already exists')

@ns.expect(activity_input)
@ns.marshal_with(activity, code=201)
@ns.marshal_with(activity, code=HTTPStatus.CREATED)
def post(self):
"""Create an activity"""
return activity_dao.create(ns.payload), 201
return activity_dao.create(ns.payload), HTTPStatus.CREATED


@ns.route('/<string:id>')
@ns.response(404, 'Activity not found')
@ns.response(HTTPStatus.NOT_FOUND, 'Activity not found')
@ns.param('id', 'The activity identifier')
class Activity(Resource):
@ns.doc('get_activity')
@ns.response(422, 'The id has an invalid format')
@ns.marshal_with(activity, code=200)
@ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The id has an invalid format')
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of here, or you will have to repeat it every time.

@ns.marshal_with(activity, code=HTTPStatus.OK)
def get(self, id):
"""Get an activity"""
return activity_dao.get(id)
return activity_dao.get(id), HTTPStatus.OK
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary to specify OK as a result. By default it returns 200.


@ns.doc('update_activity')
@ns.response(422, 'The data has an invalid format.')
@ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The data has an invalid format.')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to remove UNPROCESSABLE_ENTITY (it would be in the class) and add:

    @ns.response(HTTPStatus.BAD_REQUEST, 'Invalid format or structure of the activity')
    @ns.response(HTTPStatus.CONFLICT, 'An activity already exists with this new data')

@ns.expect(activity_input)
@ns.marshal_with(activity)
def put(self, id):
"""Update an activity"""
return activity_dao.update(id, ns.payload)

@ns.doc('delete_activity')
@ns.response(422, 'The id has an invalid format')
@ns.response(204, 'Activity deleted successfully')
@ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The id has an invalid format')
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the UNPROCESSABLE_ENTITY. It will be in the class.

@ns.response(HTTPStatus.NO_CONTENT, 'Activity deleted successfully')
def delete(self, id):
"""Delete an activity"""
activity_dao.delete(id)
return None, 204
return None, HTTPStatus.NO_CONTENT