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

Raina Campbell Completed Task List #125

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

Conversation

Rainacam12
Copy link

No description provided.

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 +18 to +21
try:
new_goal = Goal.from_dict(request_body)
except:
return {"details": "Invalid data"}, 400

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.


request_body = request.get_json()

for i in request_body["task_ids"]:

Choose a reason for hiding this comment

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

Love this!

Comment on lines +56 to +57
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.

How would your code look different if you were to implement this into a class function?


request_body = request.get_json()

goal.title = request_body["title"]

Choose a reason for hiding this comment

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

Right now, if a user sends a request without the key title your server would crash. There's a couple of ways to handle this, you could call the validate_request function before you access the keys in request_body or you could implement a try/except block.

Comment on lines +56 to +61
if sorting_query == "asc":
tasks = Task.query.order_by(Task.title.asc()).all()
elif sorting_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.

Great work on making the database do the heavy lifting when it comes to sorting the tasks!

"is_complete": (task.completed_at != None)
}
}
else:

Choose a reason for hiding this comment

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

Another way you could implement this by changing your to_dict method for Task to include a goal_id if one is present.

"channel": "C057EA9H4G7",
"text": slack_message
}
response = requests.post("https://slack.com/api/chat.postMessage", headers=header, json=json_body)

Choose a reason for hiding this comment

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

Our code could possibly fail while trying to commit the change to our database and therefore sending the message to Slack could be a false positive. We should send alerts like these at the very end of our logic just in case something goes wrong.

db.session.commit()

return abort(make_response({"details":f"Task {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, 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