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

Amethyst - Mazzy #119

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

Amethyst - Mazzy #119

wants to merge 29 commits into from

Conversation

mlweir98
Copy link

No description provided.

Comment on lines +9 to +15
def to_dict(self):
goal_as_dict = {}
goal_as_dict["id"] = self.goal_id
goal_as_dict["title"] = self.title


return goal_as_dict

Choose a reason for hiding this comment

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

Nice helper method function! ⭐️

description = task_data["description"]
)

return new_task

Choose a reason for hiding this comment

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

Comment on lines +15 to +20
try:
new_goal = Goal(title=request_body["title"])

except:
abort(make_response({
"details": "Invalid data"

Choose a reason for hiding this comment

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

Like we validated models, could we also make a general function that validates request bodies? Here's an example:

def validate_request_body(request_body, keys):
    for key in keys:
        if not request_body.get(key): 
            abort(make_response({
                'Invalid Data': f'missing key: {key}'
            }, 400))

    return True

We can pass in the request_body and a list of strings that are keys and then check to see if those keys are present.

Choose a reason for hiding this comment

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

Now we can have error handling and request body validation across all our routes more easily.

Comment on lines +38 to +41
for goal in goals:
goals_response.append(
goal.to_dict()
)

Choose a reason for hiding this comment

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

Nice utilization of your .to_dict() helper method! ⭐️

Comment on lines +87 to +90
for task in request_body["task_ids"]:
task = validate_model(Task, task)
tasks_list.append(task)
task.goal_id = goal.goal_id

Choose a reason for hiding this comment

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

⭐️

Comment on lines +108 to +115
for task in goal.tasks:
tasks_list.append({
"id" : task.task_id,
"goal_id" : task.goal_id,
"title" : task.title,
"description" : task.description,
"is_complete" : False
})

Choose a reason for hiding this comment

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

Well done! I have a question for you, do you think that this loop and the logic within could be moved to into the Goal class?

Comment on lines +40 to +45
if sort_query == "asc":
tasks = Task.query.order_by(Task.title.asc()).all()
elif sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
else:
tasks = Task.query.all()

Choose a reason for hiding this comment

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

Awesome! 💫


return jsonify(tasks_response)

def validate_model(cls, model_id):

Choose a reason for hiding this comment

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

Make sure your helper functions are in a place that's easy to find


task = validate_model(Task, task_id)

send_slack_message(task.title)

Choose a reason for hiding this comment

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

So, right now your route sends a message to slack saying that the task has been completed, before the task is marked complete. But what happens if your can't actually commit the change, your code will send out a false positive. You typically will want to send alerts like this after all your other logic has ran.


return make_response({
"details": f'{Task.__name__} {task_id} "{task.title}" successfully deleted'
}), 200

Choose a reason for hiding this comment

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

Well done on this project Maz, I didn't have much to comment on and that is a good thing! Keep up the good work! Really looking forward to what you create in the frontend! Please feel free to reach out if you have any questions about the feedback that I left! ✨💫🤭

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