Scissors: Jittania Smith #49
Conversation
…ecklist that summarizes README and test requirements.
…isn't right. Will be easier to address once I've done Wave 2 I think
…an tests - the README says to include a available_inventory entry in response bodies for GET and PUT endpoints, but doing so will cause the Postman tests to fail
…_code type was changed - in my version of the README it's shown as a string
… now with updated Postman tests
…sts for both waves.
…lly not happy with this code
…eleting customers/videos don't actually work because they don't delete the Rental records as well; fixed (Postman tests still pass)
| if rentals_list != None: | ||
| response_body["rentals"] = rentals_list |
There was a problem hiding this comment.
you should be able to use the current_rentals property to create the rentals list instead of requiring it to be passed in here.
There was a problem hiding this comment.
It looks like you didn't end up using that parameter anywhere anyway...but little tidbit for future reference.
| return response_body | ||
|
|
||
|
|
||
| # ❗️ Does it matter that I didn't give this its own file? |
There was a problem hiding this comment.
Python will accept it but people tend to pretty universally prefer each class to be in its own file in most programming languages for readability.
| customer = db.relationship('Customer', backref='rental') | ||
| video = db.relationship('Video', backref='rental') |
There was a problem hiding this comment.
Because you already have the fk_video_id and fk_customer_id fields, it isn't necessary or typical to add a relationship attribute for those fields here as well. I'm not sure whether there is a way to make use of this in any way because I haven't seen it before.
| fk_customer_id = db.Column(db.Integer, db.ForeignKey('customer.cust_id')) | ||
| due_date = db.Column(db.DateTime) | ||
|
|
||
| # ❗️ backref here is declaring a 'rental' property for both the Customer and Video classes, but I'm not sure when or if that's being used since I couldn't get either video.current_renters OR customer.current_rentals to work |
There was a problem hiding this comment.
Those functions look totally appropriate as they are to me, fwiw!
| rentals_bp = Blueprint("rentals", __name__, url_prefix="/rentals") | ||
|
|
||
|
|
||
| # ❗️ SO MUCH REPEAT CODE I'M SO SORRRYYYYYYY - so many opportunities here to DRY up code, just ran out of time |
There was a problem hiding this comment.
There'll be more opportunities to practice DRYing code
|
|
||
| request_body = request.get_json() | ||
|
|
||
| # ❗️ just awful - I hacked my way through this piecemeal and with more time could have just modified the existing helper function I call here |
There was a problem hiding this comment.
I think the code looks pretty good tbh!
| if not record_due_date: | ||
| return make_response({"details": f"Video with video id {video_id} was already checked in"}, 400) |
| current_rentals = [] | ||
| current_rentals_response = [] | ||
|
|
||
| # ❗️ concerned again about the fact that I'm not really updating the rental table at any point to indicate whether its records are current or not.... |
There was a problem hiding this comment.
removing the due date does accomplish this, right? Maybe I'm missing something...?
| # ❗️ concerned again about the fact that I'm not really updating the rental table at any point to indicate whether its records are current or not.... | ||
| rental_records = Rental.query.filter_by(fk_customer_id=customer_id) | ||
|
|
||
| # ❗️ I know this can't be the best way to do this (might even be...the worst way), but trying to use `saved_customer.current_rentals` to get some kind of list of current rentals wasn't working in the way I expected, and I just ran out of time |
There was a problem hiding this comment.
Huh, bummer you couldn't get that attribute to work! but great that you got this working regardless!
There was a problem hiding this comment.
Again, I really don't think this is bad, even if it is doing a little unnecessary work!
| # format the response body | ||
| for each_renter in current_renters: | ||
| customer_info = Customer.query.get_or_404(each_renter["customer_id"]) | ||
| rental_info = Rental.query.get_or_404(each_renter["rental_id"]) |
There was a problem hiding this comment.
the rental info was just retrieved above so there's no need to fetch it again here
There was a problem hiding this comment.
same goes for the similar code in the previous function
Desperately needs to be refactored but out of time