Tigers - Maria S.

[mariasilvadev]

No description provided.

[chimerror]

Good work!

Even though I left several comments, including a few bugs and deviations from the spec this code is generally correctly implemented, cleanly written, and straightforward to read, which is good enough for a Green!

[chimerror]

This doesn't match the specification, as it means that the response will always have "is_complete" set to False, when it should be True if self.completed_at is not None.

While you fix this in your response to PATCH /tasks/<task_id>/mark_complete, this means that if I called PATCH /tasks/13/mark_complete and then GET /tasks/13/mark_complete, the second call will say that "is_complete" is false in contradiction to the first call which says it's true. So even though it has just been marked complete and has an completed_at column in the database, a user would be led to believe that it has not been completed though they marked it as such.

There are a few different ways you could have set "is_complete" here, a very succinct way is bool(self.completed_at).

[chimerror]

Other than that issue I pointed out, this model class for Task otherwise looks good!

[chimerror]

In this case where you are returning a dictionary, you don't need jsonify because Flask will convert dictionaries to JSON objects by default. You would need it if you were returning, say, a list, like you do for GET /tasks.

Likewise, if your status code is going to be 200, you don't need to supply it in your return statement, as that is the default. Now in that case, an argument could be made that being explicit here has benefits in... well, being explicit, but I think the benefit of that is marginal here.

[chimerror]

Also, watch spacing! There's an extra space between jsonify and its argument list, which is very minor but can cause a bit of confusion, as the standard style is no space between a function call and its argument list.

[chimerror]

There are a few other places where you use jsonify on a dictionary, but I'll not point them all out to avoid repeating myself.

[chimerror]

This block is a bit more complicated than you really need. Putting this code in a try-except block would make sense if goal.tasks could be None but that actually won't happen as Flask returns an empty list in the case that no tasks are associated with the goal. This means that [task.to_dict() for task in goal.tasks] (good use of a list comprehension, by the way!) will resolve to an empty list itself, so no error will be thrown.

Like I said, this would be different if goal.tasks would return None, which then would necessitate this code. It could be defended as defensive programming, though, though I feel that would be a bit over-defensive.

Likewise, your Task.to_dict() method already sets the "goal_id" key, so the for loop that follows is unnecessary.

So doing both of those changes brings this down to:

response_body = {
    "id": goal.goal_id,
    "title": goal.title,
    "tasks": [task.to_dict() for task in goal.tasks]

return response_body

[chimerror]

This code causes a 500 error when calling GET /goals/<goal_id> if the goal has tasks associated with it. My guess is you meant something like [task.title for task in self.tasks], but your code here means you're trying to access the title attribute of the list (really an InstrumentedList) rather than its elements.

That being said, rather than returning the title of the tasks here, if you used this for , I would recommend returning the tasks themselves, using the code you wrote for GET /goals/<goal_id>/tasks, [task.to_dict() for task in self.tasks]. Handling this here would have greatly reduced the amount of code needed for your GET /goals/<goal_id>/tasks route as you could just return goal.to_dict.

I do recognize that you cannot return the "tasks" key here as it will cause some failures in the tests you wrote for GET /goals/<goal_id> that don't expect this key. However, I think that could be better fixed by updating the test or writing the test in a more future-proof way I'll describe in a comment on the test.

[chimerror]

Good extra work on getting these other columns!

[chimerror]

I originally had a comment here that your file seemed to lack organization, but as I've read more, I now get that it's just these two /goals/<goal_id>/tasks functions that are out-of-place. I would recommend putting these after your /tasks/<task_id>/mark_(in)complete routes.

A common pattern is to arrange routes by increasing size of endpoint URL then by HTTP verbs, which you do seem to follow below. Moving this to after the task completion routes would maintain that organization, which makes your code more maintainable as there's an obvious pattern methods are organized in.

(Though it can be hard to get a good organization that works, I admit!)

[chimerror]

Minor style preference: Just delete these unneeded decorators rather than commenting them out. They are mainly there as scaffolding code, and now that the code works, we can remove them.

[chimerror]

So, as mentioned in your Goal model class, this test fails if you fill in the "tasks" key. Rather than not returning this information, I would recommend updating this test.

A simple way would be to add the "tasks" key:

assert response_body = {
    "goal": {
        "id": 1,
        "title": "Updated Goal Title",
        "tasks": []
     }
}

Which is OK in our case. But what would happen if we added a new key? And then another. It could be tiring to constantly update here, especially since the "title" key is the key we really care about for this test. So we could just check that directly with assert response_body["goal"]["title"] == "Updated Goal Title", and then this test will pass no matter what additional keys we add.

However, there is value in not just testing that the "title" updated but that the other keys did not change. In that case, rather than comparing to a literal here, I would do something like this, where I compare it to the goal as it appears before updating:

get_response = client.get("/goals/1/")
EXPECTED_TITLE = "Updated Goal Title"
expected_response = get_response.get_json()
expected_response["title"] = EXPECTED_TITLE

put_response = client.put("/goals/1/", json={
    "title": EXPECTED_TITLE
})
put_response_body = put_response.get_json()

assert put_response.status_code == 200
assert "goal" in put_response_body
assert put_response_body == expected response

goal = Goal.query.get(1)
assert goal.title == EXPECTED_TITLE

But there's an even better way! You'd have to change the one_goal fixture in conftest.py to return the goal that got created:

@pytest.fixture
def one_goal(app):
    new_goal = Goal(title="Build a habit of going outside daily")
    db.session.add(new_goal)
    db.session.commit()
    return new_goal

Then , that new_goal will be passed in to your test as one_goal. Which you can then use instead of having to make a call to GET /goals/1:

EXPECTED_TITLE = "Updated Goal Title"
expected_response = one_goal.to_dict()
expected_response["title"] = EXPECTED_TITLE

response = client.put("/goals/1/", json={
    "title": EXPECTED_TITLE
})
response_body = put_response.get_json()

assert response.status_code == 200
assert "goal" in response_body
assert response_body == expected response

goal = Goal.query.get(1)
assert goal.title == EXPECTED_TITLE

Of course we instructors (well, not me personally) wrote the conftest.py file so this isn't really on you.

[chimerror]

Otherwise, I do like the test design here, especially going to the database to verify that it updated there.