Skip to content

Dragonflies - Vicky F.#42

Open
vfarinango wants to merge 13 commits intoAda-C23:mainfrom
vfarinango:main
Open

Dragonflies - Vicky F.#42
vfarinango wants to merge 13 commits intoAda-C23:mainfrom
vfarinango:main

Conversation

@vfarinango
Copy link

No description provided.

vfarinango added 12 commits May 1, 2025 16:37
…class methods in task.py. Implements a route_utilities.py module in routes package. Creates a blueprint for task as task_bp and implements a POST request. Adds missing completed_at key for test_create_task() in test_wave_01. Test is passing.
…ng. Passes test_create_task_must_contain_title() and test_create_task_must_contain_description().
…y to match required error messaging. Fixes tets in tests/ package to include missing assertions and modifies various tests to resolve errors.
…ogic into get_models_with_filters() in route_utilities.py. Modifies test_get_tasks_sorted_asc() and test_get_tasks_sorted_desc() in test_wave_02.py to make the correct assertions. All tests passing.
…ts in test_wave_03.py assertions. All tests passing.
… call_slackbot() function. Successfully calling Slack API to post to privte channel when a patch request is made to a given task id.
… bot API integration into task_routes modify_completion_status() route and implements Goal model with id and title attributes and CRUD routes.
…() in goal_routes.py. 4 out of 6 tests passing.
…s more readable. Applies necessary changes across all dependencies. Same 4/6 tests passing as direct previous commit.
…ds_to_goal_already_with_goals() to assert len(db.session.scalar(query).tasks) == 3 instead of 2.
Copy link

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

@vfarinango, I would like you to resubmit your project. There is a refactor for the /goals/<id>/tasks POST route that I would like you to make. You can find more details about it in the comments. You also altered some tests that aren't meant to be altered. We want to make sure not to do that so we can ensure that we are properly testing functionality.

Comment on lines +4 to +5
from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bp

Choose a reason for hiding this comment

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

Nice! You are following Flask convention to by naming your Blueprints bp and then using as to import them under an alias.

from dotenv import load_dotenv
import os

load_dotenv()

Choose a reason for hiding this comment

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

Using load_dotenv to ensure that your environmental variables are loaded before access. 👍🏿

Comment on lines +26 to +27
app.register_blueprint(tasks_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.


class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]

Choose a reason for hiding this comment

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

Great work using the declarative mapping here! Since we are doing any specific declarations like in id we can simply use Mapped in conjunction with a Python datatype to declare what this column will look like.

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")

Choose a reason for hiding this comment

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

Perfect! You are making a relationship attribute on the Goal model. This attribute is going to be a list of Task models. You then use relationship with back_populates to tell SQLAlchemy to sync this attribute with relationship attribute called goal on the Task model.

Comment on lines +59 to +63
assert response.status_code == 404
# assertion 2 goes here
assert response.get_json() == {
"details": "Goal with id 1 was not found."
}

Choose a reason for hiding this comment

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

👍🏿

Comment on lines +89 to +99
response = client.put("/goals/1", json={
"title": "Updated Goal title"
})

# Assert
# ---- Complete Assertions Here ----
# assertion 1 goes here
# assertion 2 goes here
# assertion 3 goes here
# ---- Complete Assertions Here ----
assert response.status_code == 204

query = db.select(Goal).where(Goal.id == 1)
goal = db.session.scalar(query)

assert goal.title == "Updated Goal title"

Choose a reason for hiding this comment

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

Great, you are checking the database to ensure the changes persisted!

Comment on lines +105 to +109
response = client.put("/goals/1", json={
"title": "Updated Goal title"
})

response_body = response.get_json()

Choose a reason for hiding this comment

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

Comment on lines +131 to +132
query = db.select(Goal).where(Goal.id == 1)
assert db.session.scalar(query) == None

Choose a reason for hiding this comment

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

👍🏿

}
query = db.select(Goal).where(Goal.id == 1)
assert len(db.session.scalar(query).tasks) == 2
assert len(db.session.scalar(query).tasks) == 3

Choose a reason for hiding this comment

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

Suggested change
assert len(db.session.scalar(query).tasks) == 3
assert len(db.session.scalar(query).tasks) == 2

… tasks of a goal with those present in the current request. It also now calls a single helper function (get_validated_tasks_from_payload) to handle all task validation and processing. Updates test_wave_06.py test_post_task_ids_to_goal_already_with_goals to ==2 instead of 3. All tests passing.
Copy link

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

Changes look good! Thanks for the updates @vfarinango!

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