Accelerate - Risha A. #13

rishallen · 2021-06-15T23:10:43Z

No description provided.

testing commit messaging

anselrognlie

Nice job!

Waves 1 through 4 all look good. The tests pass, and the code is clear and understandable. The main thing in these waves to look at is how to reduce the repetitive code we need to write for CRUD operations on our models. There is a lot of similarity in input handling and validation, as well as the output that could be moved either to helper functions in the routes file, or into the model class itself.

Wave 5 was really close to being done. The main issue was with the method in goal for outputting the model as a dict. There was a small issue in the method itself, and the routes were calling the wrong name.

Wave 6 needed conditional goal_id handling in the task output, which you were really close on with the to_json method in Task, then adding tasks to goals and displaying tasks for goals would finish it off. You had the relationship set up in Task and Goal in a way that would work (one of the helper methods added by sSqlAlchemy would have had a misleading name, but the relationship itself was fine, including the foreign key relationship).

Try revisiting this, as most projects we work on will have more than a single table, and getting comfortable with these relationships will be really useful. You could also take a look at the retro-video-store project we started live coding to think about an example of a many-to-many relationship, which is also a common need in applications.

So I'd look at the feedback here, and see if you can get those last few bits working, since this will be helpful going forward.

Overall, good work.

anselrognlie · 2021-06-26T23:52:32Z

app/__init__.py

+    from .routes import task_bp
+    from .routes import goals_bp
+    app.register_blueprint(task_bp)
+    app.register_blueprint(goals_bp)


👍

One thing we started to touch on in the video store live code was that we can split routes into multiple files. We can make a routes folder, and put routes for each endpoint into separate files, named for their model. Then we can use the name bp for the blueprint in each file since it would be the only blueprint in the file. Then these imports might look like:

from .routes import task, goal app.register_blueprint(task.bp) app.register_blueprint(goal.bp)

anselrognlie · 2021-06-27T00:00:10Z

app/models/goal.py

@@ -1,6 +1,14 @@
 from flask import current_app
 from app import db
-
+from app.models.task import Task


We don't need to import the Task class name here. We only use the string "Task" in the relationship declaration. We only need to import Task if we actually use the Task constructor function here in this file.

anselrognlie · 2021-06-27T00:05:53Z

app/models/goal.py


 class Goal(db.Model):
    goal_id = db.Column(db.Integer, primary_key=True)
+    title = db.Column(db.String)
+    tasks = db.relationship("Task", backref="task", lazy=True)


The backref argument is used to provide a name that SqlAlchemy will use to add a property to the target model (Task in this case) to get back to the current model Goal. goal would be an appropriate name.

Then, if we have a Task instance, we could write task.goal to get a reference to the Goal with which the Task is associated (or None if the Task isn't associated with a Goal).

The tasks association itself would let us write goal.tasks to get the collection of all the Tasks associated with a particular Goal.

Suggested change

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

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

anselrognlie · 2021-06-27T00:08:51Z

app/models/goal.py

+    def goal_json(self):
+        return( {
+            "id": self.goal_id,
+            "title": self.title
+        }, 200)


This method is the main reason wave 5 has some failures. Your routes logic is trying to call a method called goal_to_json but the method here is called goal_json. Also, we should only be returning the dictionary here, not the status code.

The following change would allow Wave 5 to pass!

Suggested change

def goal_json(self):

return( {

"id": self.goal_id,

"title": self.title

}, 200)

def goal_to_json(self):

return {

"id": self.goal_id,

"title": self.title

}

anselrognlie · 2021-06-27T00:12:29Z

app/models/task.py

@@ -1,6 +1,29 @@
 from flask import current_app
 from app import db
-
+# from app.models.goal import Goal


This line is not necessary (it's commented, so you probably noticed that) 😄

We only need to import thee Goal identifier if we literally use the Goal constructor in this file. Even if we had a parameter that received a Goal, we wouldn't need to import the Goal constructor!

Additionally. since there was an import in goal.py for task.py, this import would cause a circular import dependency. Avoiding circular dependencies is why we have imports in strange places in app/__init__.py too!

anselrognlie · 2021-06-27T18:22:08Z

app/routes.py

+
+# goals route
+@goals_bp.route("", methods = ["POST", "GET"], strict_slashes = False)
+def handle_goals():


Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for Task will also apply to Goal. These are common themes for any model with CRUD operations.

anselrognlie · 2021-06-27T18:24:13Z

app/routes.py

+        db.session.add(new_goal)
+        db.session.commit()
+        response_body = {
+            "goal": new_goal.goal_to_json()


The main issue with wave 5 was that your equivalent "to json" method was defined with the name goal_json instead. With the updated version of goal_to_json provided earlier, this should start working!

anselrognlie · 2021-06-27T18:33:12Z

app/routes.py

+        task_ids = request_body["task_ids"]
+
+        for task_id in task_ids:
+            goal_id.task_id = goal_id


goal_id is a simple string (of read directly from query params) or number, so it doesn't have a field called task_id. Even a Goal instance (like goal) wouldn't. A Goal has the field tasks (created by our relationship code) to which we can append Task instances (not IDs). On the other side of the relationship each Task instance has a field called goal_id that can be used to associate a Task with a Goal. With the changes suggested in Goal, they also have a field called goal to which we can assign an actual Goal instance.

For any of these cases, we really need to lookup each passed task id to get an instance, then either set the goal/goal_id on each task, or update the tasks collection on the goal. Something like

task = Task.query.get_or_404(task_id) task.goal = goal

This wouldn't remove any existing tasks from the goal (we should think about what to do for those tasks).

Additionally, to ensure that the change to the relationships occur transactionally (either they all apply, or none of them do), we can move the commit outside the loop, so that it gets called once at the end.

anselrognlie · 2021-06-27T18:34:51Z

app/routes.py

+
+            db.session.commit()
+
+        return {"id":(goal_id), "task_ids": task_ids},200


The test is expecting a numeric version of the goal id, so we can get the version stored in the goal itself (or force the conversion with int).

Suggested change

return {"id":(goal_id), "task_ids": task_ids},200

return {"id":(goal.goal_id), "task_ids": task_ids},200

anselrognlie · 2021-06-27T18:36:40Z

app/routes.py

+
+            db.session.commit()
+
+        return {"id":(goal_id), "task_ids": task_ids},200


The last bit that would be needed for wave 6 would be to handle GET requests for the tasks of a goal. Something like the following would be a good start.

else if request.method == "GET": tasks = [] for task in goal.tasks: tasks.append(task.to_json()) return { "id": goal.goal_id, "title": goal.title, "tasks": tasks }, 200

clear added 13 commits June 10, 2021 11:47

building the task model

def3a08

beginning of routes

083c255

adds migrations

1f1a276

wave one completed

9d24258

wave 2 completed, wave 3 done

a586ca8

wave 3 updated

af0f34e

added Slack API connection

9dc49c2

corrected 2 tests in wave 3

8443b7e

added goal migration

ffa798e

wave 5 completed

8f4eb91

working on wave 6

4c444ff

worked on wave 6

c7bdf78

Merge branch 'master' of https://github.com/rishallen/task-list-api

65b3ba2

testing commit messaging

anselrognlie reviewed Jun 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Accelerate - Risha A. #13

Accelerate - Risha A. #13

Uh oh!

rishallen commented Jun 15, 2021

Uh oh!

anselrognlie left a comment

Uh oh!

anselrognlie Jun 26, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

anselrognlie Jun 27, 2021

Uh oh!

Uh oh!

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


		db.session.commit()

		return {"id":(goal_id), "task_ids": task_ids},200

	return {"id":(goal_id), "task_ids": task_ids},200
	return {"id":(goal.goal_id), "task_ids": task_ids},200

Accelerate - Risha A. #13

Are you sure you want to change the base?

Accelerate - Risha A. #13

Uh oh!

Conversation

rishallen commented Jun 15, 2021

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!