Spruce: Sarah and Kaitlyn #45
Conversation
add delete customer column
…tore Lines starting with '#' will be ignored, and an empty message aborts :wq
There was a problem hiding this comment.
Well done!
The submission tests for delete fail, but the modified versions in your repo are reasonable to handle marking records as deleted. However, while deciding to flag records as deleted rather than actually deleting them is good practice for businesses to keep their data around for logistics, it does complicate our queries. We need to be absolutely sure that the deleted records don't squeak back into our results. It's a useful approach, but comes with a lot of complications. Keep an eye out for comments related to that. It's a little surprising to go with the deleted flag approach for video and customer, when checking in a rental results in deleting that row. I'd suggest trying to take a consistent approach across the entire application. Also, there's an issue in the rentals endpoints for video and customer, so be sure to review that as well.
Nice job splitting your routes into multiple files, and splitting your endpoints by verb. You got a good start on adding some helper methods in your models, so keep at it. We can really clean up our routes by identifying the common operations we keep writing over and over, or even those that a route shouldn't know the details about how it works, and moving those into model methods.
The main job of a route should be to get the data from the request, then call helper methods to do the real work (business logic), and finally take the results and package them to be sent back to the client. Currently, there's business logic (such as how to update the database to reflect checking out and in) in your routes. Think about moving that to methods in your existing model classes, or even creating entirely new classes that represent sequences of actions carried out at the level of business logic. Consider adding a class like RentalManager, that could have check_out and check_in methods to manage those workflows.
But overall, looks good! 🎉
| # check that video is no longer returned (don't assume that delete means remove the row) | ||
| response = client.get("/videos/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 |
There was a problem hiding this comment.
As we discussed, this is the way that I would write this test. The API call shouldn't make any assumptions about how the back end system decides to "delete" a record, only that once it's been deleted, the API shouldn't see the record any more.
| @@ -0,0 +1,56 @@ | |||
| """all models | |||
There was a problem hiding this comment.
Nice job adding a message to your migrations. It can be more useful in longer-lived applications where there might be multiple migrations created over time, but it's good to get in the habit.
| # Register Blueprints Here | ||
| from .video_routes import videos_bp | ||
| app.register_blueprint(videos_bp) | ||
|
|
||
| from .customer_routes import customers_bp | ||
| app.register_blueprint(customers_bp) | ||
|
|
||
| from .rental_routes import rentals_bp | ||
| app.register_blueprint(rentals_bp) |
There was a problem hiding this comment.
👍
Nice job making multiple files containing routes separated by the resource type. For organization, we might put them in a routes folder instead of leaving them in the root. Inside that folder (along with a __init__.py) we could have a file per resource, so customer.py, video.py, and rental.py. If they're all in a routes folder, we wouldn't need to include route in the name. Each resource route file would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.
Then here, we could import and register the blueprints like
from .routes import customer, video, rental
app.register_blueprint(customer.bp)
app.register_blueprint(video.bp)
app.register_blueprint(rental.bp)| customer_id = db.Column(db.Integer, primary_key=True, autoincrement=True) |
There was a problem hiding this comment.
According to the SqlAlchemy docs about autoincrement:
The default value is the string "auto" which indicates that a single-column primary key that is of an INTEGER type with no stated client-side or python-side defaults should receive auto increment semantics automatically; all other varieties of primary key columns will not.
This means that here, since the primary is not a compound key, the type is integer, and there's no specified default value, the desired autoincrement behavior will apply even without listing it. It doesn't hurt anything to list it, but it's not required.
| phone = db.Column(db.String) | ||
| register_at = db.Column( | ||
| db.DateTime(timezone=True), server_default=func.now() |
There was a problem hiding this comment.
Nice use of a server default value. This actually affects the schema, and ensures that the database will always populate a default value when a customer row is created, regardless of whether or not it's through our API. However, the functions we can specify for the server default are a bit of a toss-up. Ideally, we'd just like to store things in UTC and not worry about timezones, but that might not be straightforward with the server default. For a little more flexibility, we could mark the column as non-nullable, and use a client default, which lets us run any Python code, and would still prevent the addition of rows lacking the data. You would need to play around with this a bit more to see which approach might work best.
|
|
||
| video_id = request_body["video_id"] | ||
| customer_id = request_body["customer_id"] | ||
| video = Video.query.get(video_id) |
There was a problem hiding this comment.
Even here we should be doing id format checks, and whether the record was deleted. Leaving the data in the database has far-reaching effects!
| video = Video.query.get(video_id) | ||
| if not video: | ||
| return jsonify(None), 404 | ||
| video_inventory = video.total_inventory - len(video.rentals) |
There was a problem hiding this comment.
Consider adding a helper method to the video model so that we don't have to put the logic for this calculation here in the route.
| ) | ||
|
|
||
| db.session.add(new_rental) | ||
| customer.videos_checked_out = len(customer.rentals) |
There was a problem hiding this comment.
Your rental_dict helper already calculates the videos checked out count dynamically. Think about removing this column entirely.
| if not (video and customer): | ||
| response_body = {"message": f"No outstanding rentals for customer {customer_id} and video {video_id}"} | ||
| return jsonify(response_body), 404 |
There was a problem hiding this comment.
Notice the test for this doesn't check the message. The 404 (not found) message could indicate which of the customer or video wasn't found, not that there are no outstanding rentals.
| if customer.videos_checked_out == 0: | ||
| response_body = {"message": f"No outstanding rentals for customer {customer_id} and video {video_id}"} | ||
| return jsonify(response_body), 400 |
There was a problem hiding this comment.
I would be more comfortable doing the actual Rental lookup you perform a few lines down first, and if no rental is found, then this message would be appropriate. The current check is a proxy for that lookup, but could drift out of agreement if direct modifications are ever made to the database.
No description provided.