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

Zoisite - L. Pollard #120

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Zoisite - L. Pollard #120

wants to merge 6 commits into from

Conversation

LilLoveBytes
Copy link

thanks for the extra time :)

Copy link

@apradoada apradoada left a comment

Choose a reason for hiding this comment

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

Overall, well done, L! There are a few spots where code can be cleaned up and modified but this looks good! GREEN!

app.register_blueprint(task_bp)

from .goal_routes import goal_bp
app.register_blueprint(goal_bp)

return app

Choose a reason for hiding this comment

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

I love what you have done organizationally in terms of separating out the goal_routes from the task_routes. One small tweak would be to make a new directory called "routes" to house those two files along with your helper.py.

Also, don't forget that if you do that, you will need an empty init.py file inside the routes directory. You also will need an empty init.py file inside your models directory!

abort(make_response(
jsonify({"message": f"{cls.__name__} {model_id} not found"}), 404))

return model

Choose a reason for hiding this comment

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

Great job making the validate model method class agnostic!


}
if tasks:
goal_dict["tasks"]= [task.to_dict() for task in self.tasks]

Choose a reason for hiding this comment

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

Great list comprehension here!

request_body = request.get_json()

try:
request_body["title"]

Choose a reason for hiding this comment

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

I love the idea to include some error handling here. We unfortunately cannot simply try to see if the request body exists. Instead, lines 36-43 should be included in the try with line 32 removed!

app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_TEST_DATABASE_URI")
"RENDER_DATABASE_URI")

Choose a reason for hiding this comment

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

Just something small to point out here. Lines 20 and 26 are currently the exact same which means that the line itself could be pulled out of the conditional entirely. This wasn't an issue before when you were differentiating between the testing and the development database, but now that it has all been updated to the Render database, we have that collision.

if not model:
abort(make_response(jsonify({"message":f"{cls.__name__} {model_id} not found"}), 404))

return model

Choose a reason for hiding this comment

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

Great job with this validate model method!

request_body["title"] and request_body["description"]
except:
abort(make_response(jsonify({"details":"Invalid data"}), 400))

Choose a reason for hiding this comment

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

Great attempt at error handling. Just a reminder however that lines 30-37 should be included in the try block here. The code on line 26 won't actually do anything besides returning true or false. A more appropriate approach might look like the following:

 try:
    new_task = Task.from_dict(request_body)

    db.session.add(new_task)
    db.session.commit()

    return make_response(jsonify({"task": new_task.to_dict()}), 201)

response_body = {"task":task.to_dict()}
db.session.commit()

return response_body

Choose a reason for hiding this comment

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

The put method could also use some error handling in case the necessary update attributes are not included in the request!

# assertion 1 goes here
# assertion 2 goes here
assert response_body["message"] == "Goal 1 not found"
assert response.status_code == 404

Choose a reason for hiding this comment

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

Looks good!

}
}
goal = Goal.query.get(1)
assert goal.title == "Build a habit of going outside daily"

Choose a reason for hiding this comment

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

This looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants