Spruce - Sarah Olivas #82

sjolivas · 2021-11-06T06:23:50Z

No description provided.

anselrognlie

Great job!

All your tests are passing and your code is very readable.

One main thing I'd focus on going forward is identifying repetitive parts of the code and moving them to helper methods or class instance methods. We can create our own additional classes to help us organize common code and represent reusable ideas. Especially for CRUD models, there is a lot of very similar code. Even if it's not exactly the same, thinking about how to move that similar code into shared routines can help up organize and test our code. Our testing in this project focused on testing through the flask client helper, but we can write smaller unit tests similar to the tests we used in Video Party and Swap Meet, which are easier to write when the logic is pulled out into small units of code that focus on single responsibilities.

Specifically with respect to writing web apis with Flask and SqlAlchemy, keep looking at other examples and looking through the documentation for other ideas about helper methods and implementation patterns. Using decorator methods can also be a useful strategy for common tasks if we split our routes into separate methods.

Overall, you did really well, and the rest will come with continued practice and experimentation. Keep it up!!! 🎉

anselrognlie · 2021-11-17T16:23:11Z

app/__init__.py

@@ -15,12 +15,10 @@ def create_app(test_config=None):
    app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

    if test_config is None:
-        app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
-            "SQLALCHEMY_DATABASE_URI")
+        app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_DATABASE_URI")


I'm curious what prompted unwrapping these lines. Doing so exceeds the typical 80 column limit. Are you running a formatting tool?

anselrognlie · 2021-11-17T16:23:34Z

app/__init__.py

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


👍

One thing we can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a routes folder instead of a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so task.py and goal.py. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.

Then here, we could import and register the blueprints like

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

anselrognlie · 2021-11-17T16:23:58Z

app/models/goal.py

 class Goal(db.Model):
-    goal_id = db.Column(db.Integer, primary_key=True)
+    goal_id = db.Column(db.Integer, primary_key=True, autoincrement = True)


According to the SqlAlchemy docs about autoincrement:

The default value is the string "auto" which indicates that a single-column primary key that is of an INTEGER type with no stated client-side or python-side defaults should receive auto increment semantics automatically; all other varieties of primary key columns will not.

This means that here, since the primary is not a compound key, the type is integer, and there's no specified default value, the desired autoincrement behavior will apply even without listing it. It doesn't hurt anything to list it, but it's not required.

anselrognlie · 2021-11-17T16:24:18Z

app/models/goal.py

 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)


Should we be able to create a goal with a NULL title? Consider adding nullable=False here.

anselrognlie · 2021-11-17T16:31:33Z

app/models/goal.py

+    def goal_and_task_dict(self):
+        return{
+            "id": self.goal_id,
+            "title": self.title,
+            "tasks": [task.task_and_goal_dict() for task in self.tasks]
+        }


👍

Nice job having two different methods to provide the two different needed output styles. We could have used the goal_dict version to get the base values, then add in tasks, but given the size this seems ok.

Notice that as applications grow, we may end up with a large variety of endpoints that may need slightly different outputs. In that situation, rather than adding method after method to model class itself, we often will make helper classes whose sole responsibility is knowing how to render a model instance into a certain output configuration. Sometimes we call these helper classes data transfer objects (DTOs).

anselrognlie · 2021-11-17T22:17:17Z

app/routes.py

+
+
+@goals_bp.route("", methods=["GET", "POST"])
+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-11-17T22:21:25Z

app/routes.py

+        for task_id in request_body["task_ids"]:
+            task = Task.query.get(task_id)
+            if task is None:
+                return jsonify(None), 404
+            goal.tasks.append(task)
+            db.session.commit()


Good job checking whether the each requested task id lookup is successful before trying to append it to the task list. However, you may want to wait to perform the appending and commiting until after you have validated all the tasks, rather than task by task. With the task-by-task approach, we could end up partially modifying the tasks in a goal, but then fail partway through the list, leaving the goal partially modified. It's often desirable to ensure that either the entire change has been applied, or none of the change has.

Appending to the tasks realtionship is absolutely a valid way to associate a task with a goal. Notice that it's not entirely clear from the project description or tests whether POSTing to the task list is adding tasks to the goal (in addition to anything that was already there), or replacing the tasks in the goal. Which perspective does your implementation take? How could we implement the other approach?

anselrognlie · 2021-11-17T22:27:01Z

migrations/versions/106deb886b95_.py

@@ -0,0 +1,39 @@
+"""empty message


Don't forget to add a message to your migrations with -m when running the migration step.

anselrognlie · 2021-11-17T22:27:59Z

tests/test_wave_05.py

@@ -41,18 +42,17 @@ def test_get_goal(client, one_goal):
        }
    }

-@pytest.mark.skip(reason="test to be completed by student")
+# @pytest.mark.skip(reason="test to be completed by student")
 def test_get_goal_not_found(client):
    pass


We should remove the pass statement after supplying the test body.

anselrognlie · 2021-11-17T22:30:29Z

tests/test_wave_05.py

-    # ---- Complete Assertions Here ----
+    assert response.status_code == 404
+    assert response_body == None
+    assert Goal.query.all() == []


An alternative to directly accessing the DB here would be to make an additional GET request. Doing so keeps us at the same level of abstraction (exercising the web API) while still checking that a change has been made (deletion of the goal) without assuming how the deletion is occuring (maybe we want to use a deleted flag rather than actually deleting the row).

sjolivas added 14 commits October 27, 2021 20:04

Created Menv file and migration folder

cc9a62a

Created Task model and registered tasks_bp

ea55ed5

Built task routes at /tasks & /<task_id> endpoints

e61777d

Updated migration version

287aea8

Added asc and desc functionality to task routes

3e84b1b

Added two tasks endpoints & PATCH method to routes.py

360dd1c

Added slack bot functionality

773e638

Added model, registered bp, HTTP methods for Goals

6db250b

Completed wave 5 tests

5d790e5

Established Goal and Task relationship

c385230

Migrations for Goal and Task relationship update

004f704

Added endpoint /<goal_id>/tasks & verb functions

7c55119

Modified the Goal and Task realtionship

582c253

Added Procfile

0dc4ce6

anselrognlie reviewed Nov 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spruce - Sarah Olivas #82

Spruce - Sarah Olivas #82

sjolivas commented Nov 6, 2021

anselrognlie left a comment

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021

anselrognlie Nov 17, 2021



		@goals_bp.route("", methods=["GET", "POST"])
		def handle_goals():

Spruce - Sarah Olivas #82

Are you sure you want to change the base?

Spruce - Sarah Olivas #82

Conversation

sjolivas commented Nov 6, 2021

anselrognlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment