AC2 - gp2 (Nad, Dalia) #18
Conversation
implemented wave 1 for the customer_routes, tests are passing
Nadbranch
Customer routes
Nadbranch
…customer_routes.py
…OST /rentals/check-out to stop customer from checking out the same video twice and to display the correct due_date for the rentals
…er routes to use them
implemented GET /videos/<video_id>/rentals and changed some code in P…
Nadbranch
Nadbranch query parameters and paginations
…/rentals, updated video model to include available_inventory attribute, created new migration for the video model update
…s for the optional wave
Dalia wave 3.
refactored customer routes to use the function to_dict() Tested everything again with no problems
Wave 4 nad videos query parameters
| @@ -5,7 +5,7 @@ | |||
| from dotenv import load_dotenv | |||
|
|
|||
| db = SQLAlchemy() | |||
| migrate = Migrate() | |||
| migrate = Migrate(compare_type=True) | |||
There was a problem hiding this comment.
Just curious, was this addition to manage issues you had updating models?
There was a problem hiding this comment.
we found out that if we add this optional parameter changes made to the data types of attributes for our models will be reflected in the migration. here the documentation for that: https://alembic.sqlalchemy.org/en/latest/autogenerate.html
| postal_code = db.Column(db.String, nullable=False) | ||
| phone = db.Column(db.String, nullable=False) | ||
| registered_at = db.Column(db.DateTime, default=datetime.datetime.now().strftime("%a, %d %b %Y %X %z")) | ||
| videos_checked_out_count = db.Column(db.Integer, nullable=False, default=0) |
There was a problem hiding this comment.
In most situations, if we can derive the data we want from something else we are already storing, we should choose to do that over storing a new piece of data. If we store a piece of derivable data, we need to manually keep our related data in sync which introduces places for potential bugs, and we take up more memory for each record.
In this case, we hold videos which is a list of the videos the user currently has checked out and videos_checked_out_count. We could remove videos_checked_out_count and call len on videos anywhere we need the count.
| postal_code = db.Column(db.String, nullable=False) | ||
| phone = db.Column(db.String, nullable=False) |
There was a problem hiding this comment.
It isn't necessary for this project, but out in the wild we'd likely look at what the max size these strings could possibly be is and give character limits in the definition. The top answers here give some explanation of possible performance impacts: https://stackoverflow.com/questions/1962310/importance-of-varchar-length-in-mysql-table
| id = db.Column(db.Integer, primary_key=True) |
There was a problem hiding this comment.
Another option could be to create a composite primary key from the customer_id and video_id columns. The benefit is we can store one less column for each record, but the limitation is there can only ever be one Rental record with that combination of customer and video ids.
# One possible set up
video_id = db.Column(db.Integer, db.ForeignKey('video.id'), primary_key=True, nullable=False)
customer_id = db.Column(db.Integer, db.ForeignKey('customer.id'), primary_key=True, nullable=False)| id = db.Column(db.Integer, primary_key=True, autoincrement=True, nullable=False) | ||
| title = db.Column(db.String, nullable=False) | ||
| total_inventory = db.Column(db.Integer, nullable=False) | ||
| available_inventory = db.Column(db.Integer, default=available_inventory_default) |
There was a problem hiding this comment.
This is another field that we could derive, this time using total_inventory and the count of current rentals with this video id.
|
|
||
| return model_instance | ||
|
|
||
| def validate_request_body(request_body,required_data): |
There was a problem hiding this comment.
I like the flexibility of this for checking data to create & update models!
| def validate_request_body(request_body,required_data): | ||
| if not request_body: | ||
| msg = "An empty or invalid json object was sent." | ||
| abort(make_response(jsonify({"details":msg}),400)) |
There was a problem hiding this comment.
Tiny style nitpick: for best practices we'd want a space after the comma as a visual separation between the make_response arguments
| new_rental = Rental( | ||
| customer_id=valid_customer.id, | ||
| video_id=valid_video.id, | ||
| due_date=datetime.now() + timedelta(days=MAX_DAYS_RENTALS)) |
There was a problem hiding this comment.
I'm wondering if this is something that could be set as a default on the column like the registered_at attribute of the Customer model.
| valid_video = validate_model(Video, request_body["video_id"]) | ||
|
|
||
| if valid_video.available_inventory < 1: | ||
| abort(make_response(jsonify({"message":"Could not perform checkout"}), 400)) |
There was a problem hiding this comment.
In a real life scenario, we might want to add more specific info like "Could not perform checkout: no copies available"
| try: | ||
| db.session.add(new_video) | ||
| db.session.commit() | ||
|
|
||
| except: |
There was a problem hiding this comment.
Were there errors you were running into saving new models that required the try/except here?
There was a problem hiding this comment.
Not really. This was me being paranoid. One of my most traumatic bugs happened when the db for my app wasn't available due to network problems. It was hard for me to find why sometimes the data was saved and some other times it wasn't. So out of extra precaution, I put those there to have some info in the server logs, in case something couldn't be saved due to database problems.
However I must agree I should have had more consistency as I only did it for creating the video and it should have been part of all the functions where the database was being changed. That would include the update and delete methods for every model and the check-in, check-out endpoints for rentals.
No description provided.