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

Seaturtles_Ashley_Benson #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions Procfile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
web: gunicorn 'app:create_app()'

Choose a reason for hiding this comment

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

You have the Procfile required for deployment, but didn't do the deployment?

44 changes: 36 additions & 8 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,51 @@

def create_app(test_config=None):
app = Flask(__name__)
app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

if test_config is None:
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
if not test_config:
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False

Choose a reason for hiding this comment

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

This and line 22 are the same in both branches, so it it could be moved outside the conditions (it used to be line 15).

app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get(
"SQLALCHEMY_DATABASE_URI")
else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_TEST_DATABASE_URI")

# Import models here for Alembic setup
db.init_app(app)
migrate.init_app(app, db)

from app.models.task import Task
from app.models.goal import Goal
from app.routes import tasks_bp
from app.routes import goals_bp
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)
return app

db.init_app(app)
migrate.init_app(app, db)

# Register Blueprints here

return app
# def create_app(test_config=None):
# app = Flask(__name__)
# app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

# if test_config is None:
# app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
# "SQLALCHEMY_DATABASE_URI")
# else:
# app.config["TESTING"] = True
# app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
# "SQLALCHEMY_TEST_DATABASE_URI")
# print("SQLALCHEMY_DATABASE_URI is " + app.config["SQLALCHEMY_DATABASE_URI"]);
# # Import models here for Alembic setup
# from app.models.tasks import Task
# from app.models.goal import Goal

# db.init_app(app)
# migrate.init_app(app, db)

# # Register Blueprints here
# from .routes import tasks_bp
# app.register_blueprint(tasks_bp)

# return app
Comment on lines +39 to +62

Choose a reason for hiding this comment

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

What's this code doing here? It looks almost the same, but doesn't have goals. Try to clean up things like this before submitting, or add some other label for why it's here. Even better, practice using branches and move code that you'd like to hang onto for reference to a side branch.

Empty file added app/goal_routes.py
Empty file.
1 change: 1 addition & 0 deletions app/models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

19 changes: 19 additions & 0 deletions app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,24 @@
from app import db
from flask import abort, make_response

Choose a reason for hiding this comment

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

We'd like to avoid extra dependencies on flask in our model. If we find ourselves bringing in additional modules, that might mean we're starting to move beyond the single responsibility we want our class to have.



class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
# id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)

Choose a reason for hiding this comment

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

Consider setting nullable=False here. Should we be able to make a Goal with no title?

tasks = db.relationship("Task", backref="goal", lazy=True)

Choose a reason for hiding this comment

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

It turns out lazy=True is the default for a relationship, so we could leave that off.



def to_json(self):
return {
"id" : self.goal_id,
"title" : self.title
}

@classmethod
def create_task(cls, request_body):
try:
new_task = cls(title=request_body['title'])
return new_task
except:
return abort(make_response({"details": "Invalid data"}, 400))
Comment on lines +19 to +24

Choose a reason for hiding this comment

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

Make sure to rename things so they make sense for the class they're in. We're working with a goal here, not a task.

Choose a reason for hiding this comment

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

Consider leaving the error handling as part of the route rather than living here. That would allow us to avoid needing to import flask specific stuff in our model class files.

Also, we should avoid except statements that don't say what they're looking for. Here, we're looking for a KeyError, so we should include that in the except clause.

72 changes: 70 additions & 2 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,73 @@
from app import db

from flask import jsonify, abort, make_response
# from requests import requests

class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String, nullable=False)
description = db.Column(db.String, nullable=False)
completed_at = db.Column(db.DateTime)
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True)
goals = db.relationship("Goal")

Choose a reason for hiding this comment

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

Since you used backref in Goal, we don't need this relationship here. If you do want to have both entries in both places, prefer back_populates.


def to_json(self):
is_complete = self.completed_at != None

hash_map = {
"id" : self.task_id,
"title" : self.title,
"description" : self.description,
"is_complete" : is_complete
}
if self.goal_id:
hash_map["goal_id"] = self.goal_id
Comment on lines +22 to +23

Choose a reason for hiding this comment

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

👍 Nice way to selectively add the goal id key for tasks that have one.


return hash_map


@classmethod
def create_task(cls, request_body):
try:
try:
new_task = cls(
title=request_body['title'],
description=request_body['description'],
completed_at=request_body['completed_at']

Choose a reason for hiding this comment

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

This is the main line that we're checking for a KeyError for. The other two fields are required.

Rather than needing to repeat the whole constructor call on error, we could do something like

  completed_at=request_body.get("completed_at")

while will return None if the key isn't found rather than raising an error.

)
except KeyError:
new_task = cls(
title=request_body['title'],
description=request_body['description'],
)
return new_task
except:
return abort(make_response({"details": "Invalid data"}, 400))
Comment on lines +43 to +44

Choose a reason for hiding this comment

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

As in Goal, I'd leave the error handling related to how flask will tell the caller what went wrong out in the route logic. Our model type doesn't need to "know" about flask and reporting errors through routes (SRP).



# def to_dict(self):

Choose a reason for hiding this comment

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

Try to clean up stray code before turning in the project. If you want to keep something for reference, consider moving it to a branch.

# return {
# "task": {
# "id": self.id,
# "task_id": self.id,
# "title": self.title,
# "description": self.description,
# "is_complete": self.completed_at




# @classmethod
# def from_dict(cls, data_dict):
# return cls(
# title=data_dict["title"],
# description=data_dict["description"],
# completed_at=data_dict["completed_at"],
# )


#return dict(
# id=self.task_id,
# title = self.title,
# description=self.description,
# is_complete = True if self.completed_at else False
# )
Loading