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

Laurel O scissors #58

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Laurel O scissors #58

wants to merge 10 commits into from

Conversation

lolkinetzky
Copy link

@lolkinetzky lolkinetzky commented May 13, 2021

Not the cleanest code for submission, but here we are! There are some really weird things going on in my local machine. I asked two TA's and two students to help me trouble shoot to no avail, but basically it seemed like my migrations would not work after I added the goal_id columns to my table? Not sure what I did. Katrina K was kind enough to clone my project and test my slack bot from postman on their machine, everything worked fine there.

@lolkinetzky lolkinetzky marked this pull request as draft May 13, 2021 11:41
Copy link

@jmaddox19 jmaddox19 left a comment

Choose a reason for hiding this comment

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

I don't know what you're talking about. I would say this code looks GREAT!

Sorry to hear you had some struggles with the database at the end. I hope you aren't having any similar struggles with the video store project (though I know you're having other issues now...)

There are just a couple little suggestions for improvement but seriously looks great! It looks like you got a lot out of this project!

completed_at = db.Column(db.DateTime, nullable=True,)
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True)

def to_json(self):

Choose a reason for hiding this comment

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

Love the custom method!

goals_bp = Blueprint("goals", __name__, url_prefix="/goals")

#wave4
def hi_slack_api(task):

Choose a reason for hiding this comment

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

Love that this is in its own function! I think it make the code more readable!

task.goal_id = goal.goal_id

db.session.commit()
return make_response({"id": goal.goal_id, "task_ids": request_body["task_ids"]}, 200)

Choose a reason for hiding this comment

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

This change would allow the response to include ALL tasks associated with the goal, not just the new ones.
This isn't super intuitive so I'd definitely be down to talk through this more in our next 1:1

Suggested change
return make_response({"id": goal.goal_id, "task_ids": request_body["task_ids"]}, 200)
task_list = []
for task in goal.tasks:
task_list.append(task.task_id)
return make_response({"id": goal.goal_id, "task_ids": task_list}, 200)

task = Task.query.get(task_id)
task.goal_id = goal.goal_id

db.session.commit()

Choose a reason for hiding this comment

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

Good job remembering to commit here! Many folks forgot to since there isn't actually a test to verify the new tasks get saved in the db


return make_response({"id": int(goal_id), "title": goal.title, "tasks": task_list }, 200)

def helper_fun(task_goal):

Choose a reason for hiding this comment

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

Fun helper function! To improve the code further, this would be a very appropriate method to add to task.

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