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

Setharika Sok task-list-api #118

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

Conversation

SetharikaSok
Copy link

No description provided.

'id':self.task_id,
'title':self.title,
'description':self.description,
'is_complete': True if self.completed_at else False,

Choose a reason for hiding this comment

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

Love the ternary you implemented here!

Comment on lines +27 to +30
abort(make_response({"details":"Invalid data"}, 400))

elif not request_body.get("description") and cls == Task:

Choose a reason for hiding this comment

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

We could write the conditions for this logic in one line like this:

if not (request_body.get('title') or request_body.get('description')):

Choose a reason for hiding this comment

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

We could also generalize your validate_request function to check for any request body like this:

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.

tasks = Task.query.filter_by(title = title_query)

elif description_query:

Choose a reason for hiding this comment

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

Nice job!

Comment on lines +55 to +62
tasks = Task.query.order_by(Task.title.asc())

elif sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc())

elif title_query:
tasks = Task.query.filter_by(title = title_query)

Choose a reason for hiding this comment

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

Nice work on getting the database to do most of the lifting when it comes to sorting the tasks

Comment on lines +99 to +100
task.description = request_body["description"]

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 keys title or description 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 +147 to +152
response = requests.post('https://slack.com/api/chat.postMessage', json=slack_data, headers=headers)
response.raise_for_status()

except requests.exceptions.RequestException as e:
return jsonify({'message': 'Failed to send Slack notification: {e}'})

Choose a reason for hiding this comment

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

Nice work with the try/except clause and on having your Slack post sent after the logic of marking a task complete! We don't want to send out any false positive alerts just in case our logic fails during the update!

Comment on lines +177 to +179
for goal in goals:
goals_response.append({ "title": goal.title, "id": goal.goal_id})

Choose a reason for hiding this comment

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

Do you think that this could be a class method? If so, how would your code look different?

abort(400)

goal.task_ids = task_ids

Choose a reason for hiding this comment

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

So here you are making a new attribute on this goal instance that will hold a list of task ids that were last added. This doesn't actually make an association between the two models. Lines 228 and 229 take care of that.




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