- 
                Notifications
    
You must be signed in to change notification settings  - Fork 44
 
C22 - Sphinx - Tatiana Trofimova #38
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 28 commits
a5d2d10
              8ad364a
              11048e4
              20dabc8
              af10725
              1a41d60
              4d17fc6
              a6094b6
              7d877be
              991f860
              055a22f
              2858943
              3c6bb1f
              ededb67
              4bdfaec
              0589dab
              360a353
              23f70c1
              b4031cc
              d96de8b
              81eb790
              e328024
              d2fbaf4
              db350a8
              b4c37a8
              a8c75bb
              d910bdf
              490d343
              94df535
              cb0af09
              e391da3
              e93fc24
              edc1841
              0039a99
              f9521b9
              3302239
              a6dcf4a
              b49f6a0
              2b73458
              4c54403
              d32dfb3
              1ca3859
              4d92b7d
              3fee895
              c440a7b
              069d7ec
              583722c
              3457fe4
              ed09c67
              6716d2a
              a4943da
              82fc58d
              4eb37c0
              5755c17
              0db806a
              321b6bd
              485dac5
              a92db83
              2e4dc64
              10c2c39
              721fe94
              52da2dc
              2ffdc3f
              a2bf5ed
              1b51387
              09f89e9
              363fb62
              5ceb059
              f22a456
              acca02a
              c2e586d
              5c178dd
              53f779c
              6052392
              b15e2d0
              011e936
              c904e75
              051049f
              6507919
              4697786
              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 | 
|---|---|---|
| 
          
            
          
           | 
    @@ -138,4 +138,7 @@ dmypy.json | |
| .pytype/ | ||
| 
     | 
||
| # Cython debug symbols | ||
| cython_debug/ | ||
| cython_debug/ | ||
| 
     | 
||
| # Slack bot logo | ||
| slack_bot_logo.png | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,19 @@ | ||||||
| from sqlalchemy.orm import Mapped, mapped_column | ||||||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||||||
| from sqlalchemy import String | ||||||
| from ..db import db | ||||||
| 
     | 
||||||
| class Goal(db.Model): | ||||||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||||||
| title: Mapped[str] = mapped_column(String(50)) | ||||||
| 
         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. Consider whether the this column for   | 
||||||
| tasks: Mapped[list["Task"]] = relationship(back_populates="goal") | ||||||
| 
     | 
||||||
| def to_dict(self): | ||||||
| return { | ||||||
| "id": self.id, | ||||||
| "title": self.title | ||||||
| } | ||||||
| 
     | 
||||||
| @classmethod | ||||||
| def from_dict(cls, data): | ||||||
| return Goal(title=data["title"]) | ||||||
| 
         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. While we can explicitly use  
        Suggested change
       
    
  | 
||||||
| 
     | 
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,38 @@ | ||||||||||||||||||||||||||
| from sqlalchemy.orm import Mapped, mapped_column | ||||||||||||||||||||||||||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||||||||||||||||||||||||||
| from sqlalchemy import String, DateTime, ForeignKey | ||||||||||||||||||||||||||
| from datetime import datetime | ||||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||||
| from ..db import db | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| class Task(db.Model): | ||||||||||||||||||||||||||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||||||||||||||||||||||||||
| title: Mapped[str] = mapped_column(String(50)) | ||||||||||||||||||||||||||
| 
         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. As with   | 
||||||||||||||||||||||||||
| description: Mapped[str] = mapped_column(String(255)) | ||||||||||||||||||||||||||
| completed_at: Mapped[Optional[datetime]] = mapped_column(DateTime) | ||||||||||||||||||||||||||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||||||||||||||||||||||||||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| def to_dict(self): | ||||||||||||||||||||||||||
| task_dict = { | ||||||||||||||||||||||||||
| "id": self.id, | ||||||||||||||||||||||||||
| "title": self.title, | ||||||||||||||||||||||||||
| "description": self.description, | ||||||||||||||||||||||||||
| "is_complete": bool(self.completed_at) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| if self.goal_id is not None: | ||||||||||||||||||||||||||
| task_dict["goal_id"] = self.goal_id | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| return task_dict | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||
| def from_dict(cls, data): | ||||||||||||||||||||||||||
| data["completed_at"] = data.get("completed_at", None) | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| task = Task( | ||||||||||||||||||||||||||
| title=data["title"], | ||||||||||||||||||||||||||
| description=data["description"], | ||||||||||||||||||||||||||
| completed_at=data["completed_at"], | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||
| return task | ||||||||||||||||||||||||||
| 
         
      Comment on lines
    
      +32
     to 
      +38
    
   
  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. Since you don't reference the variable  
        Suggested change
       
    
  | 
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1 +1,57 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, abort, make_response | ||
| 
         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. 
  | 
||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from app.db import db | ||
| from app.routes.route_utilities import * | ||
| 
     | 
||
| bp = Blueprint("goal_bp", __name__, url_prefix="/goals") | ||
| 
     | 
||
| 
     | 
||
| @bp.post("/", strict_slashes=False) | ||
| def create_goal(): | ||
| return create_class_instance(Goal, request, ["title"]) | ||
| 
         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. The logic in   | 
||
| 
     | 
||
| @bp.get("/", strict_slashes=False) | ||
| def get_all_goals(): | ||
| return get_all_instances(Goal, request.args) | ||
| 
     | 
||
| @bp.get("/<goal_id>", strict_slashes=False) | ||
| def get_one_goal(goal_id): | ||
| return get_one_instance(Goal, goal_id) | ||
| 
     | 
||
| @bp.put("/<goal_id>", strict_slashes=False) | ||
| def update_goal(goal_id): | ||
| return update_instance(Goal, goal_id, request) | ||
| 
     | 
||
| @bp.delete("/<goal_id>", strict_slashes=False) | ||
| def delete_goal(goal_id): | ||
| return delete_instance(Goal, goal_id) | ||
| 
     | 
||
| @bp.post("/<goal_id>/tasks") | ||
| def assign_task_to_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| task_ids = request.get_json().get("task_ids", []) | ||
| list_of_tasks = [] | ||
| 
     | 
||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| if task: | ||
| list_of_tasks.append(task) | ||
| 
     | 
||
| goal.tasks.extend(list_of_tasks) | ||
| db.session.commit() | ||
| 
     | 
||
| return { | ||
| "id": goal.id, | ||
| "task_ids": [task.id for task in goal.tasks] | ||
| 
         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 like that you get task ids from  Since  You didn't do this, but if you had done   | 
||
| } | ||
| 
     | 
||
| @bp.get("/<goal_id>/tasks") | ||
| def get_task_of_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| 
     | 
||
| return { | ||
| "id": goal.id, | ||
| "title": goal.title, | ||
| "tasks": [task.to_dict() for task in goal.tasks] | ||
| } | ||
| 
                       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. Your helper functions look great. By encapsulating logic in generic helper functions that can be used across multiple routes, you keep your codebase DRY and your routes concise!  | 
            
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| from flask import abort, make_response | ||
| from app.db import db | ||
| 
     | 
||
| def apply_filters(cls, arguments, query): | ||
| for attribute, value in arguments: | ||
| if hasattr(cls, attribute): | ||
| query = query.where(getattr(cls, attribute).ilike(f"%{value}%")) | ||
| 
     | 
||
| def validate_model(cls, cls_id): | ||
| try: | ||
| cls_id = int(cls_id) | ||
| except ValueError: | ||
| message = { "message": f"{cls.__name__} ID {cls_id} is invalid"} | ||
| abort(make_response(message, 400)) | ||
| 
     | 
||
| query = db.select(cls).where(cls.id == cls_id) | ||
| result = db.session.scalar(query) | ||
| 
     | 
||
| if not result: | ||
| message = {"message": f"{cls.__name__} with ID {cls_id} was not found"} | ||
| abort(make_response(message, 404)) | ||
| 
     | 
||
| return result | ||
| 
     | 
||
| def set_new_attributes(instance, req_body): | ||
| for attr, value in req_body.items(): | ||
| if hasattr(instance, attr): | ||
| setattr(instance, attr, value) | ||
| 
     | 
||
| def create_class_instance(cls, request, required_fields): | ||
| 
         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. Notice that your function definition has the param  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. While this function does at some point create an instance of some class (line 33), it ultimately creates a record and saves it to the DB. Maybe a name like   | 
||
| req_body = request.get_json() | ||
| try: | ||
| new_instance = cls.from_dict(req_body) | ||
| except KeyError as error: | ||
| message = {"details": f"Invalid request: missing {error.args[0]}"} | ||
| abort(make_response(message, 400)) | ||
| 
     | 
||
| db.session.add(new_instance) | ||
| db.session.commit() | ||
| 
     | 
||
| return {cls.__name__.lower(): new_instance.to_dict()}, 201 | ||
| 
     | 
||
| def get_all_instances(cls, args): | ||
| sort = args.get("sort") | ||
| query = db.select(cls).order_by(cls.title.desc() if sort=="desc" else cls.title) | ||
| 
     | 
||
| apply_filters(cls, args.items(), query) | ||
| 
     | 
||
| instances = db.session.scalars(query) | ||
| return [instance.to_dict() for instance in instances], 200 | ||
| 
     | 
||
| def get_one_instance(cls, instance_id): | ||
| instance = validate_model(cls, instance_id) | ||
| 
     | 
||
| return { cls.__name__.lower(): instance.to_dict() if instance else instance}, 200 | ||
| 
     | 
||
| def update_instance(cls, instance_id, request): | ||
| instance = validate_model(cls, instance_id) | ||
| req_body = request.get_json() | ||
| if cls.__name__ == "Task": | ||
| req_body["completed_at"] = req_body.get("completed_at", None) | ||
| 
     | 
||
| set_new_attributes(instance, req_body) | ||
| 
     | 
||
| db.session.commit() | ||
| return { cls.__name__.lower(): instance.to_dict() }, 200 | ||
| 
     | 
||
| def delete_instance(cls, instance_id): | ||
| instance = validate_model(cls, instance_id) | ||
| db.session.delete(instance) | ||
| db.session.commit() | ||
| 
     | 
||
| return {"details": f'{cls.__name__} {instance.id} "{instance.title}" successfully deleted'}, 200 | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1 +1,71 @@ | ||||||
| from flask import Blueprint | ||||||
| from flask import Blueprint, request, make_response, abort | ||||||
| 
         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. 
  | 
||||||
| from datetime import datetime | ||||||
| from os import environ | ||||||
| import requests | ||||||
| 
     | 
||||||
| from app.models.task import Task | ||||||
| from app.db import db | ||||||
| from app.routes.route_utilities import * | ||||||
| 
     | 
||||||
| bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||||||
| 
     | 
||||||
| @bp.post("/", strict_slashes=False) | ||||||
| def create_task(): | ||||||
| return create_class_instance(Task, request, ["title", "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. Same comment as in  
        Suggested change
       
    
  | 
||||||
| 
     | 
||||||
| @bp.get("/", strict_slashes=False) | ||||||
| def get_all_tasks(): | ||||||
| return get_all_instances(Task, request.args) | ||||||
| 
     | 
||||||
| @bp.get("/<task_id>", strict_slashes=False) | ||||||
| def get_one_task(task_id): | ||||||
| return get_one_instance(Task, task_id) | ||||||
| 
     | 
||||||
| @bp.put("/<task_id>", strict_slashes=False) | ||||||
| def update_task(task_id): | ||||||
| return update_instance(Task, task_id, request) | ||||||
| 
     | 
||||||
| @bp.delete("/<task_id>", strict_slashes=False) | ||||||
| def delete_task(task_id): | ||||||
| return delete_instance(Task, task_id) | ||||||
| 
     | 
||||||
| @bp.patch("/<task_id>/mark_complete") | ||||||
| def mark_task_completed(task_id): | ||||||
| task = validate_model(Task, task_id) | ||||||
| task.completed_at = datetime.now() | ||||||
| db.session.commit() | ||||||
| 
     | 
||||||
| message_is_sent = send_task_complete_message(task.title) | ||||||
| 
     | 
||||||
| if not message_is_sent: | ||||||
| raise Exception( | ||||||
| "An error occured during notification sending!\ | ||||||
| Please connect to the Task List developer!") | ||||||
| return { "task": task.to_dict() }, 200 | ||||||
| 
     | 
||||||
| @bp.patch("/<task_id>/mark_incomplete") | ||||||
| def mark_task_incompleted(task_id): | ||||||
| task = validate_model(Task, task_id) | ||||||
| task.completed_at = None | ||||||
| db.session.commit() | ||||||
| 
     | 
||||||
| return { "task": task.to_dict() }, 200 | ||||||
| 
     | 
||||||
| def send_task_complete_message(task_title): | ||||||
| request_data = { | ||||||
| "channel": "#api-test-channel", # Slack channel for tests | ||||||
| # "channel": "U07GC9C8Y4X", # My Slack account ID | ||||||
| "username": "Task List app", | ||||||
| 
         
      Comment on lines
    
      +57
     to 
      +59
    
   
  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. Prefer the string literals on lines 56 and 58 be referenced with constant variables like   | 
||||||
| "text": f"Someone just completed the task \"{task_title}\"" | ||||||
| } | ||||||
| message_status = requests.post( | ||||||
| url="https://slack.com/api/chat.postMessage", | ||||||
| 
         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. Prefer the URL to be referenced by a constant variable instead of having a string literal here. URL = "https://slack.com/api/chat.postMessage" 
        url=URL,  | 
||||||
| json=request_data, | ||||||
| headers={ | ||||||
| "Authorization": environ.get('SLACK_API_KEY'), | ||||||
| "Content-Type": "application/json" | ||||||
| 
         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. When you pass your payload as json, like you do on line 63, Flask will auto set the   | 
||||||
| }, | ||||||
| timeout=5 | ||||||
| ) | ||||||
| 
     | 
||||||
| return message_status.json()["ok"] | ||||||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| # A generic, single database configuration. | ||
| 
     | 
||
| [alembic] | ||
| # template used to generate migration files | ||
| # file_template = %%(rev)s_%%(slug)s | ||
| 
     | 
||
| # set to 'true' to run the environment during | ||
| # the 'revision' command, regardless of autogenerate | ||
| # revision_environment = false | ||
| 
     | 
||
| 
     | 
||
| # Logging configuration | ||
| [loggers] | ||
| keys = root,sqlalchemy,alembic,flask_migrate | ||
| 
     | 
||
| [handlers] | ||
| keys = console | ||
| 
     | 
||
| [formatters] | ||
| keys = generic | ||
| 
     | 
||
| [logger_root] | ||
| level = WARN | ||
| handlers = console | ||
| qualname = | ||
| 
     | 
||
| [logger_sqlalchemy] | ||
| level = WARN | ||
| handlers = | ||
| qualname = sqlalchemy.engine | ||
| 
     | 
||
| [logger_alembic] | ||
| level = INFO | ||
| handlers = | ||
| qualname = alembic | ||
| 
     | 
||
| [logger_flask_migrate] | ||
| level = INFO | ||
| handlers = | ||
| qualname = flask_migrate | ||
| 
     | 
||
| [handler_console] | ||
| class = StreamHandler | ||
| args = (sys.stderr,) | ||
| level = NOTSET | ||
| formatter = generic | ||
| 
     | 
||
| [formatter_generic] | ||
| format = %(levelname)-5.5s [%(name)s] %(message)s | ||
| datefmt = %H:%M:%S | 
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.
👍