Skip to content

Conversation

yxzhang88
Copy link

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work Miranda, you hit the learning goals here. Well done. I left some minor comments in the code. Let me know if you have questions via slack.

Comment on lines +16 to +17
if "title" not in response_body:
return 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.

It's good that you're doing data validation, but you should probably indicate what data is invalid.

@goal_bp.route("", methods=["GET"])
def read_goal():
goals = Goal.query.all()
response = [goal.return_body() for goal in goals]

Choose a reason for hiding this comment

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

Nice use of a list comprehension.

Comment on lines +17 to +22
@classmethod
def from_dict(cls, data_dict):
if "title" in data_dict:
new_obj = cls(title=data_dict["title"])
return new_obj

Choose a reason for hiding this comment

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

Good use of a class-method as a helper.

Comment on lines +90 to +101
PATH = "https://slack.com/api/chat.postMessage"

SLACKBOT_TOKEN = os.environ.get("SLACKBOT_TOKEN")

# the query parameters come from the
query_params = {
"token": SLACKBOT_TOKEN,
"channel": "task-notifications",
"text": f"Someone just completed the task {task.title}"
}

requests.post(url=PATH, data=query_params, headers={"Authorization": SLACKBOT_TOKEN})

Choose a reason for hiding this comment

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

The slack portion of this method might do well as a helper function.

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