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

Rock - Libby #38

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

Rock - Libby #38

wants to merge 4 commits into from

Conversation

libgerber
Copy link

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Heck yeah! This looks very nice, Libby! You were very consistent with your return statements and structuring your routes!

videos_checked_out_count = db.Column(db.Integer, nullable = False, default=0)
rentals_out = db.relationship("Rental", backref="customer") #is lazy=True needed?

def return_data(self):

Choose a reason for hiding this comment

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

👍

Comment on lines +3 to +4
from datetime import datetime
from app.models.rental import Rental

Choose a reason for hiding this comment

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

even though we mention "Rental" as a string argument, we aren't using the class itself or creating instances, so we don't need this

Suggested change
from datetime import datetime
from app.models.rental import Rental
from datetime import datetime

@@ -0,0 +1,26 @@
# from flask import current_app
from app import db
from app.models.rental import Rental

Choose a reason for hiding this comment

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

Suggested change
from app.models.rental import Rental

rentals_out = db.relationship("Rental", backref="video") #is lazy=True needed?


def return_data(self):

Choose a reason for hiding this comment

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

👍

from app.models.video import Video
from app.models.rental import Rental
from app import db
import requests

Choose a reason for hiding this comment

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

Suggested change
import requests


@customers_bp.route("/<customer_id>", methods=["GET"], strict_slashes=False)
def get_one_customer(customer_id):
customer = Customer.query.get_or_404(customer_id)

Choose a reason for hiding this comment

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

👍

Comment on lines +57 to +61
response.append(
{"release_date": video.release_date,
"title": video.title,
"due_date":rental.due_date
})

Choose a reason for hiding this comment

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

maybe we could turn this into a helper method in our model liked the others you made!

def update_customer(customer_id):
customer = Customer.query.get_or_404(customer_id)
request_body = request.get_json()
print(f"request body: {request_body}")

Choose a reason for hiding this comment

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

oop! don't forget to remove print statements used for debugging/troubleshooting

Comment on lines +70 to +78
if "name" not in request_body \
or "postal_code" not in request_body \
or "phone" not in request_body:
return make_response({}, 400)

elif type(request_body["name"]) is not str or request_body["name"] == "" \
or type(request_body["phone"]) is not str or request_body["phone"] == "" \
or type(request_body["postal_code"]) is not int or request_body["postal_code"] == "":
return make_response({}, 400)

Choose a reason for hiding this comment

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

since these have the same return statement, maybe we can combine the two conditionals and they can share the same return statement

Comment on lines +211 to +217
response = {
"customer_id": new_rental.customer_id,
"video_id": new_rental.video_id,
"due_date": new_rental.due_date,
"videos_checked_out_count": customer.videos_checked_out_count,
"available_inventory": video.available_inventory
}

Choose a reason for hiding this comment

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

another good place for a helper method!

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