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

Scissors - Jittania Smith #55

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6973a7c
completed basic setup and registered blueprint
jittania May 6, 2021
85bd69c
Finished code for Wave 1, but need to debug
jittania May 7, 2021
af0d95d
Got all Wave 1 tests to pass
jittania May 7, 2021
7cdad2c
Passed Wave 2 tests
jittania May 7, 2021
c2b6584
Got Wave 3 tests to pass using hard-coded date:time data
jittania May 8, 2021
9a6f9ea
Added timestamp attribute, Wave 3 tests passed
jittania May 8, 2021
81b2989
added a test comment
jittania May 10, 2021
8ae4a3b
redid migrations
jittania May 10, 2021
583e9d5
testing that pushing from new branch still works
jittania May 11, 2021
ce69161
Refactored Wave 1
jittania May 11, 2021
bb3eb09
Refactored Waves 1-3
jittania May 11, 2021
1eb295b
Refactored Waves 1-3
jittania May 11, 2021
150e67d
Wave 4 completed
jittania May 12, 2021
fd5ca7f
Cleaned up comments
jittania May 12, 2021
c731228
Delete scratch_pad.md
jittania May 12, 2021
29a40f1
Delete scratch_pad.py
jittania May 12, 2021
86d8571
Sneaky trickery
jittania May 12, 2021
2d3c5e5
Merge branch 'master' of https://github.com/jittania/task-list-api in…
jittania May 12, 2021
f6b9fd8
Wave 5 tests passing
jittania May 12, 2021
70a0ebf
Set up models relationship for Wave 6
jittania May 12, 2021
6c1880e
started first endpoint for Wave 6
jittania May 12, 2021
02e1a64
Wave 1-6 tests passing
jittania May 13, 2021
e2238c5
All tests passing and code refactored
jittania May 13, 2021
7ac2e98
Refactored model instance methods
jittania May 13, 2021
371613e
started adding some comments to routes.py
jittania May 14, 2021
ce158a7
Just saw Jasmine's post in Slack about including comments in our subm…
jittania May 17, 2021
4d0ba85
Did a small refactor on a helper function - Jasmine said it was okay …
jittania May 17, 2021
e824c9f
Did a small refactor on a helper function - Jasmine said it was okay …
jittania May 17, 2021
baa567f
Updated comments to better reflect current understanding of project
jittania May 21, 2021
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
6 changes: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,8 @@ dmypy.json
.pytype/

# Cython debug symbols
cython_debug/
cython_debug/

# Xtra Scratch files
scratch_pad.md
scratch_pad.py
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()'
7 changes: 7 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,12 @@ def create_app(test_config=None):
migrate.init_app(app, db)

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

from .routes import goals_bp
app.register_blueprint(goals_bp)



return app
18 changes: 17 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,20 @@


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
goal_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
tasks = db.relationship('Task', backref='goal', lazy=True)

# This instance method returns a JSON dictionary of attribute values
# with an optional argument if the goal has tasks assigned to it
def convert_to_json(self, tasks_list=None):

Choose a reason for hiding this comment

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

Love this thoughtful, handy optional param here! That said, you shouldn't actually need to ask the caller to pass these in because they are already attached to the goal. See my comment below.


response_body = {
"id": self.goal_id,
"title": self.title
}

if tasks_list != None:
response_body["tasks"] = tasks_list
Comment on lines +14 to +20

Choose a reason for hiding this comment

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

By changing the code to make use of the tasks attribute you added to the Goal (line 8), you guarantee the list of tasks is always exactly what the database says it is, rather than allowing the caller of the function to specify whatever tasks they want to pass in.
Happy to talk more about this if this isn't clear!
(I should note, this will be slightly different behavior that what the current code does because it'll display an empty list for "tasks" when no tasks are present instead of omitting "tasks" from the response.)

Suggested change
response_body = {
"id": self.goal_id,
"title": self.title
}
if tasks_list != None:
response_body["tasks"] = tasks_list
response_body = {
"id": self.goal_id,
"title": self.title,
"tasks": self.tasks
}

Copy link
Author

@jittania jittania May 25, 2021

Choose a reason for hiding this comment

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

That's what I thought - but if I make this change, it causes my Wave 5/6 tests to fail? And I remember trying something like that initially and not being able to get it to work, which is why I went this route. I also had to use tasks = Task.query.filter_by(match_goal_id=goal_id) as opposed to just treating tasks like an attribute of a goal, which was not working. I'm guessing something else in my code needs to be changed as well, but I'm having trouble identifying where. In other words.....yes please, help me to understand!!!!! I'm confused on this one (also you'll probably notice I did a similar thing in my Video Store API because I was having the same issue)

Choose a reason for hiding this comment

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

You're totally right! This suggestion is not enough on its own. Sorry for the confusion and thanks for the thorough response!
There would need to be more code to convert the data retrieved into the appropriate format.

As far as the other confusion about needing to query manually, I'm not sure. I just made a change in your code (I'll add another comment to talk about it) and it passed the tests just fine.

For all of this though, I want to note that these are quite minor comments. The whole discussion we're having here can be fun and interesting to look into but none of it is very important or of high concern!


return response_body
23 changes: 22 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,25 @@


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)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)
match_goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'),nullable=True)


# This instance method checks if a task has been completed, and whether it
# has a matching goal id, then returns a JSON dictionary of attribute values
def convert_to_json(self):

response_body = {

Choose a reason for hiding this comment

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

Love the helper methods!

"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
}

if self.match_goal_id:
response_body["goal_id"] = self.match_goal_id

return response_body
Loading