Skip to content

Possum - Jesica G#26

Open
jguall421 wants to merge 10 commits intoAda-C24:mainfrom
jguall421:main
Open

Possum - Jesica G#26
jguall421 wants to merge 10 commits intoAda-C24:mainfrom
jguall421:main

Conversation

@jguall421
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@yangashley yangashley 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 on task-list-api!

Comment thread app/models/goal.py
class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
tasks: Mapped[list["Task"]] = relationship(back_populates="goal")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 A goal has many tasks!

Comment thread app/models/goal.py
Comment on lines +18 to +19
if self.tasks:
goal_dict["tasks"] = [task.to_dict() for task in self.tasks]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Nicely done!

Comment thread app/models/goal.py
Comment on lines +24 to +25
new_goal = cls(title=goal_data["title"])
return new_goal
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could directly return the object since you create the variable new_goal and then immediately return it without accessing it.

Suggested change
new_goal = cls(title=goal_data["title"])
return new_goal
return cls(title=goal_data["title"])

Comment thread app/models/task.py
Comment on lines +12 to +16
title: Mapped[str]
description: Mapped[str]
completed_at: Mapped[Optional[datetime]]
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👌

Comment thread app/models/task.py
"description": self.description,
"is_complete": self.completed_at is not None
}
if self.goal_id is not None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be more Pythonic to do a truthy check, like:

Suggested change
if self.goal_id is not None:
if self.goal_id:

Comment thread app/routes/task_routes.py
@bp.patch("/<task_id>/mark_complete")
def mark_complete(task_id):
task = validate_model(Task, task_id)
# response = task.to_dict()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# response = task.to_dict()

❗️take care to remove commented out code. It clutters the codebase, can be confusing to other devs, and if it was accidentally uncommented and the code executed unintentionally then we'd have a bug.

Comment thread app/routes/task_routes.py
Comment on lines +62 to +69
json_payload = {
'channel': 'task-notifications',
'text':f"Someone just completed the task, {task.title}",
'Content-Type':'application/json; charset=utf-8'
}
requests.post('https://slack.com/api/chat.postMessage',
json=json_payload,
headers={'Authorization': 'Bearer ' + os.environ.get('SLACK_TOKEN')})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Prefer this logic to be in a helper function like make_request_to_slack to keep this route concise and single-responsibility.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The channel name and slack URL that appear here as magic strings should be avoided by using constant variables instead.

Suggested change
json_payload = {
'channel': 'task-notifications',
'text':f"Someone just completed the task, {task.title}",
'Content-Type':'application/json; charset=utf-8'
}
requests.post('https://slack.com/api/chat.postMessage',
json=json_payload,
headers={'Authorization': 'Bearer ' + os.environ.get('SLACK_TOKEN')})
SLACK_CHANNEL = "task-notifications"
SLACK_ENDPOINT = "https://slack.com/api/chat.postMessage"
json_payload = {
'channel': SLACK_CHANNEL,
'text':f"Someone just completed the task, {task.title}",
'Content-Type':'application/json; charset=utf-8'
}
requests.post(SLACK_ENDPOINT,
json=json_payload,
headers={'Authorization': 'Bearer ' + os.environ.get('SLACK_TOKEN')})

Comment thread app/routes/task_routes.py
def mark_incomplete(task_id):
task = validate_model(Task, task_id)

if task.completed_at is not None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be more Pythonic to check that the value is truthy:

Suggested change
if task.completed_at is not None:
if task.completed_at:

Comment thread app/__init__.py
from flask import Flask
from .db import db, migrate
from .models import task, goal
#from .models import task, goal
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
#from .models import task, goal

Delete unused imports

Comment thread tests/test_wave_01.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Your added assertions for this test file look good to me.

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