Repository navigation
Madeline - Pheonix class #35
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
base: main
Are you sure you want to change the base?
Changes from 22 commits
afa229d
a75034a
9b81c64
f20ddf9
3fe589c
37ee6e6
5dae0b9
0aca135
f80a1f2
7c65e6b
92c6133
7e22320
5de99b7
305c76e
34f623b
9be98f8
c327580
d2ad3aa
8a8a0f5
bba559b
5dfb50c
08b6e37
3affb57
01179ea
daf5dd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| from flask import Flask | ||
| from flask import Flask, request, jsonify | ||
| from .db import db, migrate | ||
| from .models import task, goal | ||
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import goals_bp | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would prefer to name the |
||
| import os | ||
|
|
||
| def create_app(config=None): | ||
|
|
@@ -18,5 +20,9 @@ def create_app(config=None): | |
| migrate.init_app(app, db) | ||
|
|
||
| # Register Blueprints here | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) | ||
|
|
||
|
|
||
| return app | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,44 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from datetime import datetime | ||
| from app.routes.utilities_routes import create_model, validate_model, check_for_completion | ||
| from typing import Optional | ||
| from sqlalchemy import ForeignKey | ||
| # from app.models.task import Task | ||
| from ..db import db | ||
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| tasks: Mapped[list["Task"]] = relationship("Task",back_populates="goal") | ||
| # description=Mapped[str] | ||
| # completed_at: Mapped[Optional[datetime]]=mapped_column(nullable = True) | ||
|
|
||
|
|
||
| def to_dict(self): | ||
| goal_as_dict = {} | ||
| goal_as_dict["id"] = self.id | ||
| goal_as_dict["title"] = self.title | ||
| if self.tasks: | ||
| task_ids=[] | ||
| task_dictionaries = [task.to_dict() for task in self.tasks] | ||
| for task in task_dictionaries: | ||
| task_id = task.get("id") | ||
| task_ids.append(task_id) | ||
| goal_as_dict["task_ids"] = task_ids | ||
| else: | ||
| goal_as_dict["task_ids"] = [] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the cases that return task data are more related to the route than to whether the goal has tasks. There's a nested endpoint that the client is supposed to access if they want to retrieve the list of tasks for a goal. So rather than adding this conditional logic to the main The distinction between adding goal details to tasks, but not task details to goal may seem arbitrary, and to a certain degree it is. When designing our own APIs, it's really up to us, and can be influenced largely by how many child records we expect there to typically be. If the main record (here, goal) had many properties, and often had many many tasks, we'd likely want to give the caller a way to retrieve the goal properties without getting all the task details at the same time, which could significantly increase the size of the response. On the other side, since a Task only ever has 1 goal, and even then we're only returning the goal id, conditionally adding that value doesn't add significantly to the overall size of the response. So that's one justification for always including goal id data in the task response (if it has any), but not including task data in the goal response, unless specifically requested through the nested route. |
||
| # task_as_dict["description"] = self.description | ||
| # task_as_dict["is_complete"] = check_for_completion(Goal,self) | ||
|
|
||
| return goal_as_dict | ||
|
|
||
|
|
||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| new_goal = cls( | ||
| title=goal_data["title"], | ||
| # description=goal_data["description"], | ||
| # completed_at=goal_data["completed_at"] | ||
| ) | ||
| return new_goal | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,37 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from ..db import db | ||
| from datetime import datetime | ||
| from app.routes.utilities_routes import create_model, validate_model, check_for_completion | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 These imports aren't needed. |
||
| from typing import Optional | ||
| from sqlalchemy import ForeignKey | ||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]]= mapped_column(nullable = True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marking the |
||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"), nullable=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here as well, since the column is |
||
| goal: Mapped["Goal"] = relationship("Goal", back_populates="tasks") | ||
|
|
||
| def to_dict(self): | ||
| task_as_dict = {} | ||
| task_as_dict["id"] = self.id | ||
| task_as_dict["title"] = self.title | ||
| task_as_dict["description"] = self.description | ||
| task_as_dict["is_complete"] = check_for_completion(Task,self) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to prefer using a literal for the required values. There's less syntax involved, making it a little less redundant. Creating the dictionary, then setting the keys makes us write the |
||
| if self.goal_id: | ||
| task_as_dict["goal_id"] = self.goal_id | ||
|
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice detection of goal membership to detemine whether to include the goal information. |
||
|
|
||
| return task_as_dict | ||
|
|
||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| goal_id = task_data.get("goal_id") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice way to optionally allow a taks to be created as already belonging to a goal. |
||
|
|
||
| new_task = cls( | ||
| title=task_data["title"], | ||
| description=task_data["description"], | ||
| goal_id = goal_id | ||
| ) | ||
| return new_task | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remember to always create a |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,95 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, abort, make_response, request, Response | ||
|
|
||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from ..db import db | ||
| from datetime import datetime | ||
| from app.routes.utilities_routes import create_model, validate_model, get_models_with_filters, check_for_completion, delete_model | ||
|
|
||
| import requests | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unnecessary imports. |
||
|
|
||
| goals_bp = Blueprint("goals_bp",__name__, url_prefix= "/goals") | ||
|
|
||
|
|
||
| #################################################################### | ||
| ######################### Create FUNCTIONS ######################### | ||
| #################################################################### | ||
| @goals_bp.post("") | ||
| def create_goal(): | ||
| request_body = request.get_json() | ||
| return create_model(Goal,request_body) | ||
|
|
||
| @goals_bp.post("/<goal_id>/tasks") | ||
| def post_task_ids_to_goal(goal_id): | ||
| request_body = request.get_json() | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| task_ids = request_body.get("task_ids", []) | ||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| if task: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| goal.tasks.append(task) | ||
|
MBsea21 marked this conversation as resolved.
Outdated
|
||
|
|
||
| db.session.commit() | ||
| goal = goal.to_dict() | ||
| response_body = { | ||
| "id": goal.get("id"), | ||
| "task_ids": goal.get("task_ids") | ||
|
MBsea21 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| return response_body, 200 | ||
|
|
||
|
|
||
| #################################################################### | ||
| ######################### READ FUNCTIONS ######################### | ||
| #################################################################### | ||
| @goals_bp.get("") | ||
| def get_goals(): | ||
| request_arguements = request.args | ||
| return get_models_with_filters(Goal, request_arguements) | ||
|
|
||
| @goals_bp.get("/<goal_id>") | ||
| def get_one_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| response = {"goal": goal.to_dict()} | ||
| return make_response(response, 200) | ||
|
|
||
| @goals_bp.get("/<goal_id>/tasks") | ||
| def get_tasks_for_specific_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| goal_as_dict = goal.to_dict() | ||
| tasks = [task.to_dict() for task in goal.tasks] | ||
|
|
||
| response_body = { | ||
| "id": goal_as_dict.get("id"), | ||
| "title": goal_as_dict.get('title'), | ||
|
MBsea21 marked this conversation as resolved.
Outdated
|
||
| "tasks": tasks | ||
| } | ||
| for key, value in goal_as_dict.items(): | ||
| print("Key is : ", key), | ||
| print("Value is :", value) | ||
|
MBsea21 marked this conversation as resolved.
Outdated
|
||
|
|
||
| return response_body, 200 | ||
|
|
||
| #################################################################### | ||
| ######################### UPDATE FUNCTIONS ######################### | ||
| #################################################################### | ||
| @goals_bp.put("/<goal_id>") | ||
| def update_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
|
|
||
| goal_title = request_body["title"] | ||
|
MBsea21 marked this conversation as resolved.
Outdated
|
||
| db.session.commit() | ||
|
|
||
| response_body = {"message": f"Goal #{goal_id} succesfully updated"} | ||
| return make_response(response_body, 200) | ||
|
|
||
|
|
||
| #################################################################### | ||
| ######################### DELETE FUNCTIONS ######################### | ||
| #################################################################### | ||
| @goals_bp.delete("/<goal_id>") | ||
| def delete_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| return delete_model(Goal, goal) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| from flask import request, jsonify, abort, make_response | ||
| import requests | ||
| import os | ||
|
|
||
|
|
||
| def send_message(message): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice helper to contain the slack message logic. |
||
| slack_channel = os.environ.get("SLACK_CHANNEL") | ||
| slack_url = os.environ.get("SLACK_URL") | ||
|
|
||
| slack_api_token = os.getenv("SLACK_BOT_TOKEN") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than having this read directly from the environment, consider having this function receive these values as input. This prevents the function from needing to "know" where to look for the conifguration values. We could still add a wrapper function whose responsibility is to know where the configuration is, and then passes those values into this |
||
| payload = { | ||
| "channel": slack_channel, | ||
| "token": slack_api_token, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the Slack documentation, we should prefer to send the api key in the header rather than in the body. |
||
| "text": message | ||
| } | ||
|
|
||
| response = requests.post(slack_url, data=payload) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that using the |
||
| message_sent = check_response(response) | ||
| if message_sent is True: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In application code, we rarely check directly against a boolean constant. if message_sent: |
||
| return make_response({"message":"Slack message has been sent"}, 200) | ||
| else: | ||
| abort(make_response({"error":"something else broke, line 22 was met"}, 400)) | ||
|
|
||
| def check_response(response): | ||
|
|
||
| if response.status_code == 200: | ||
| return True | ||
| else: | ||
| error_message = "could not post to slack, was unable to call slack api. Check slack url, token, and request body" | ||
| abort(make_response({"error": error_message},400)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably wouldn't want to abort the entire request solely from the Slack message failing. Perhaps we could log it, or take some other diagnostic action. For now, even ignoring failure entirely would be fine. Also, be sure to check the documentation for how APIs behave. Slack sends a 200 response even for failures. This would really only detect whether there was some underlying network error when trying to reach Slack. But a problem with Slack itself wouldn't be detected. |
||
|
|
||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,97 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, abort, make_response, request, Response | ||
| from app.models.task import Task | ||
| from app.routes.utilities_routes import create_model, validate_model, get_models_with_filters, check_for_completion, delete_model | ||
| from app.routes.slack_functions import send_message | ||
| from datetime import datetime | ||
| from ..db import db | ||
| import requests | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should remove unused imports (this may have been needed prior to some refactoring, but now there's no requests-dependent code here). |
||
| import os | ||
|
|
||
|
|
||
| tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
| invalid_data_response = ({"details" : "Invalid data"}, 400) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: no space between the key and the colon |
||
|
|
||
| #create a new task in database | ||
| @tasks_bp.post("") | ||
| def create_task(): | ||
| request_body = request.get_json() | ||
| return create_model(Task,request_body) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: space after comma, here and throughout |
||
|
|
||
| @tasks_bp.get("") | ||
| def get_tasks(): | ||
| request_arguements= request.args | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: misspelling in variable (should be arguments). Suggest installing Code Spell Checker to help catch cases like this |
||
| return get_models_with_filters(Task,request_arguements), 200 | ||
|
|
||
|
|
||
| #get task by task id: | ||
| @tasks_bp.get("/<task_id>") | ||
| def get_one_task(task_id): | ||
| task = validate_model(Task,task_id) | ||
| task_dict = task.to_dict() | ||
| response = {"task":task_dict} | ||
| expected = { | ||
| "task": { | ||
| "id": 1, | ||
| "title": "A Brand New Task", | ||
| "description": "Test Description", | ||
| "is_complete": False | ||
| } | ||
| } | ||
| print("the task dictionary is:\n", dict) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 Be sure to remove debugging code prior to submission. |
||
| print("the expected dictitonary was:\n",expected) | ||
| return response,200 | ||
|
|
||
| #update task | ||
| @tasks_bp.put("/<task_id>") | ||
| def update_task(task_id): | ||
| task = validate_model(Task,task_id) | ||
|
|
||
| request_body = request.get_json() | ||
| task.title = request_body["title"] | ||
| task.description = request_body["description"] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These keys could be missing from the incoming request. This logic could benefit from checks similar to the KeyError handling for the POST routes. |
||
| try: | ||
| completed_at = request_body["completed_at"] | ||
| except: | ||
| completed_at=task.completed_at | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: add spaces around assignment |
||
|
|
||
| task.completed_at = completed_at | ||
| db.session.commit() | ||
|
|
||
| response = {"task":task.to_dict()} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: there should be a space after the colon |
||
| return response, 200 | ||
|
|
||
|
|
||
| #Delete task | ||
| @tasks_bp.delete("/<task_id>") | ||
| def delete_task(task_id): | ||
| task = validate_model(Task,task_id) | ||
| return delete_model(Task, task) | ||
|
|
||
|
|
||
| #route 2 | ||
| @tasks_bp.patch("/<task_id>/mark_complete") | ||
| def mark_complete(task_id): | ||
| task = validate_model(Task, task_id) | ||
| task.completed_at = datetime.now() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| message = f"Task {task.title} has been marked as complete!" | ||
| slack_response= send_message(message) | ||
|
|
||
| if slack_response.status_code == 200: | ||
| response = {"task":task.to_dict()} | ||
| db.session.commit() | ||
| return (response,200) | ||
| elif slack_response.status_code != 200: | ||
| return {"errror": "failed to send slack notification"}, 500 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lot of redundant error checking for this call. Here, and two versions in the utitliy functions. As I wrote in the utilities, we would likely not want to halt the request solely due to a problem with Slack. |
||
|
|
||
|
|
||
|
|
||
|
|
||
| @tasks_bp.patch("/<task_id>/mark_incomplete") | ||
| def mark_incomplete(task_id): | ||
| task = validate_model(Task, task_id) | ||
| task.completed_at = None | ||
| db.session.commit() | ||
| response = {"task": task.to_dict()} | ||
| return make_response(response,200) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have other import paths to our models (here, through the blueprints, which import the models), we technically don't need the imports here any more. It's fine to leave them for clarity (and a reminder that any other models we add would need to be included until other routes are setup), but if the VS Code warning is bothersome, feel free to remove these.