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

Sharks-Xiomara R. #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Sharks-Xiomara R. #93

wants to merge 4 commits into from

Conversation

xiomaraR
Copy link

Hey Claire, I'm still working on finishing wave 6 and deploying to Heroku. My VsCode crashed yesterday and was having technical difficulties

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Great job, Xiomara! You did a great job separating out helper functions and helper methods! Most of your route return statements were very consistent, but there were a few that could be altered. Consistent returns in an API is key.

Your logic and routes were very clean and organized! Woo hoo!

@@ -0,0 +1,102 @@
from flask import Blueprint, jsonify, make_response, request
from requests import session

Choose a reason for hiding this comment

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

This session is not the same as db.session. They serve completely separate purposes, so we can get rid of this import completely

Suggested change
from requests import session

# Create goal


@goal_bp.route("", methods=["POST"])

Choose a reason for hiding this comment

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

👍

for goal in goals:
goals_response.append(goal.to_json())

return jsonify(goals_response), 200

Choose a reason for hiding this comment

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

Try to stay consistent with your return statements here. Most of them are using make_response(jsonify()), so let's repeat that everywhere for predictability and consistency.

Suggested change
return jsonify(goals_response), 200
return make_response(jsonify(goals_response), 200)

@goal_bp.route("/<id>", methods=["GET"])
def get_one_goal(id):
goal = validate_goal(id)
return jsonify({"goal": goal.to_json()}), 200

Choose a reason for hiding this comment

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

Suggested change
return jsonify({"goal": goal.to_json()}), 200
return make_response(jsonify({"goal": goal.to_json()}), 200)

# Update goal


@goal_bp.route("/<id>", methods=["PUT"])

Choose a reason for hiding this comment

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

👍


# Get One Task: One Saved Task
@task_bp.route("/<id>", methods=["GET"])

Choose a reason for hiding this comment

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

👍

# Get Tasks

@task_bp.route("", methods=["POST", "GET"])

Choose a reason for hiding this comment

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

👍

Comment on lines +113 to +123
send_msg_path = "https://slack.com/api/chat.postMessage"
confirm_message = f"You completed the task {task.title}!"
query_params = {
"channel": "task-notifications",
"text": confirm_message
}
headers = {
"Authorization": os.environ.get("slack_token")
}
requests.post(send_msg_path, params=query_params, headers=headers)

Choose a reason for hiding this comment

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

Let's move this to the helpers.py file!



@task_bp.route("/<id>/mark_complete", methods=["PATCH"])

Choose a reason for hiding this comment

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

👍


# Mark Incomplete
@task_bp.route("/<id>/mark_incomplete", methods=["PATCH"])

Choose a reason for hiding this comment

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

👍

goal = validate_goal(id)
request_body = request.get_json()

for id in request_body["id"]:

Choose a reason for hiding this comment

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

Here is where I think the last wave is failing. Double check what the correct key is in the tests! id is for the goal, not the list of tasks

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