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

Sapphire - Linh H. #127

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

Sapphire - Linh H. #127

wants to merge 86 commits into from

Conversation

mlhuynh
Copy link

@mlhuynh mlhuynh commented May 16, 2023

No description provided.

…onse if title, description, or completed_at request is missing.
…utes.py module) to include 'task' in response body.
…s in which that function was called within the task_routes.py module.
…del helper funciton is called within the goal_routes.py module.
Copy link

@goeunpark goeunpark left a comment

Choose a reason for hiding this comment

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

Wonderful work, Linh! 🎉

Your routes are RESTful and your model classes look efficient and clean! You seem to have a great understanding of HTTP codes and wrangling your response into the desired shape of the tests. Your one-to-many goals to tasks relationship is flawless, and I'm obsessed with the detail and frequency of your commit AND flask db migrate messages! Great job all around. ⭐

This project is a well-deserved Green. 🟢 Please let me know if you have any questions on the comments below.

Comment on lines +33 to +37
from .task_routes import tasks_bp
app.register_blueprint(tasks_bp)

from .goal_routes import goals_bp
app.register_blueprint(goals_bp)

Choose a reason for hiding this comment

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

Beautiful imports in your app init! 🎉

@@ -1 +0,0 @@
from flask import Blueprint

Choose a reason for hiding this comment

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

Nice call creating two different routes files for our respective models -- it keeps both goal_routes.py and task_routes.py clean and short!

@@ -0,0 +1,30 @@
"""Added goal_id attribute to connect to goal table

Choose a reason for hiding this comment

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

Nice job remembering to add a message to your flask db migrate -m "message"! ✨

Comment on lines 86 to +106
def test_update_goal(client, one_goal):
raise Exception("Complete test")
#raise Exception("Complete test")

# Act
# ---- Complete Act Here ----
response = client.put("/goals/1", json={"title": "Updated Goal Title"})
response_body = response.get_json()

# Assert
# ---- Complete Assertions Here ----
# assertion 1 goes here
assert response.status_code == 200
# assertion 2 goes here
assert "goal" in response_body
# assertion 3 goes here
assert response_body == {
"goal": {
"id": 1,
"title": "Updated Goal Title"
}
}

Choose a reason for hiding this comment

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

Awesome set of assertions! ❤️‍🔥

Comment on lines +21 to +22
db.session.add(new_goal)
db.session.commit()

Choose a reason for hiding this comment

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

Great job using db.session.<method-name>. Session gives us access to the following:

  • db is our app’s instance of SQLAlchemy.
  • session represents our active database connection.
  • By referencing db.session we can use SQLAlchemy’s methods to perform tasks like committing a change to a model, storing a new record of a model, and deleting a record.

By referencing db.session.add() you are able to use the SQLAlchemy method to store a new record of the Goal model. ⭐

Comment on lines +21 to +31
# _______________
# | |
# | Thank |
# | you for |
# | reviewing |
# | my code! |
# |_____________|
# ||
# (\__/)||
# (•ㅅ•) ||
# /   >||

Choose a reason for hiding this comment

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

Wait, this is so cute 😭 TY for showing up!!

Comment on lines +10 to +11
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True)
goal = db.relationship("Goal", back_populates="tasks")

Choose a reason for hiding this comment

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

Awesome! Beautiful sync up!

Fun fact: the default value for nullable is True so we don't need to explicitly add it 😊

Comment on lines +13 to +36
def to_dict(self):
if self.completed_at:
return dict(
id=self.task_id,
title=self.title,
description=self.description,
is_complete=True
)

if self.goal_id and not self.completed_at:
return dict(
id=self.task_id,
goal_id=self.goal_id,
title=self.title,
description=self.description,
is_complete=False
)
else:
return dict(
id=self.task_id,
title=self.title,
description=self.description,
is_complete=False
)

Choose a reason for hiding this comment

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

This is a great place to DRY our code! Remember that the pipe operator | can merge two dictionaries together, so we could shorten the above to:

def to_dict(self):
    base_goal_dict = {
        "id": self.task_id,
        "title": self.title,
        "description": self.description,
        "is_complete": True
    }

    if self.completed_at:
        return base_goal_dict
    else: 
        base_goal_dict["is_complete"] = False

        if self.goal_id: 
            return base_goal_dict | {"goal_id": self.goal_id, }
        else: 
            return base_goal_dict 

Comment on lines +38 to +44
@classmethod
def from_dict(cls, task_data):
new_task = Task(
title=task_data["title"],
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.

Awesome from_dict instance method!

task.completed_at = datetime.utcnow()
db.session.commit()

slack_bot_message(f"Someone just completed the task {task.title}")

Choose a reason for hiding this comment

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

So clean! So clear! Spectacular! 🌟

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.

3 participants