Spruce - Cabebe & Zandra #39
Conversation
There was a problem hiding this comment.
Great work Zandra and Cabebe! What a great team! Y'all met all the learning goals for this project and passed all the tests.
I provided feedback on how y'all can refactor by omitting redundant logic, using helper methods, and list comprehension.
Y'all earned a 🟢 Green 🟢 !
| customer_id = db.Column(db.Integer, primary_key=True, autoincrement=True) |
There was a problem hiding this comment.
Integer primary key columns will auto-increment by default so it's not required to provide that setting.
We can read more about the autoincrement attribute in the SQL_Alchemy documentation
| phone = db.Column(db.String) | ||
| postal_code = db.Column(db.String) | ||
| # server_default tells sqlA to pass the default value as part of the CREATE TABLE | ||
| # func.now() or func.current_timestamp() - they are aliases of each other. This tells DB to calcaate the timestamp itself | ||
| registered_at = db.Column(db.DateTime(timezone=True), server_default=func.now()) |
There was a problem hiding this comment.
At the moment, any of these columns can contain null values. Consider revisiting each column to determine which column should truly have null values versus not. For example, is it useful to store a Customer without a name or phone number? Those two columns can be considered required information to store about a customer, despite our requirements not strictly describing that behavior.
| dict = { | ||
| "id": self.customer_id, | ||
| "name": self.name, | ||
| "phone": self.phone, | ||
| "postal_code": self.postal_code, | ||
| # weekday|day of month (16)|month name|year|local version of time|, UTC offset +HHMM or -HHMM | ||
| "registered_at": self.registered_at.strftime("%a, %-d %b %Y %X %z") | ||
| } | ||
| # if self.registered_at is not None: | ||
| # dict["registered_at"] = self.registered_at.strfttime("%a, %-d %b %Y %X %z") | ||
|
|
||
| return dict |
| rental_id = db.Column(db.Integer, primary_key=True, autoincrement=True) |
| #how many I own | ||
| total_inventory = db.Column(db.Integer) | ||
| #customers = db.relationship("Rental", back_populates="video") |
There was a problem hiding this comment.
Video should have a relationship with Rental so this line shouldn't be commented. We want to be able to identify videos that are 'rented' :
| #customers = db.relationship("Rental", back_populates="video") | |
| rentals = db.relationship("Rental", backref="video") |
| return jsonify(customer.customer_dict()), 200 | ||
|
|
||
| return jsonify(customer.customer_dict()), 200 |
There was a problem hiding this comment.
The duplicate return statement should be removed.
| return jsonify(customer.customer_dict()), 200 | |
| return jsonify(customer.customer_dict()), 200 | |
| return jsonify(customer.customer_dict()), 200 |
| if "name" not in request_body or "phone" not in request_body or "postal_code" not in request_body: | ||
| return jsonify(), 400 | ||
|
|
||
| customer.name = request_body["name"] | ||
| customer.phone = request_body["phone"] | ||
| customer.postal_code = request_body["postal_code"] | ||
|
|
||
| if "name" in request_body or "phone" in request_body or "postal_code" in request_body: | ||
| response_body ={} | ||
| response_body["name"] = customer.name | ||
| response_body["phone"] = customer.phone | ||
| response_body["postal_code"] = customer.postal_code |
There was a problem hiding this comment.
The guard clause in lines 67-68 already checks for non-existent name, phone, or postal_code values by exiting the function early. This means we can refactor by removing the check for these same fields on line 74.
| if "name" not in request_body or "phone" not in request_body or "postal_code" not in request_body: | |
| return jsonify(), 400 | |
| customer.name = request_body["name"] | |
| customer.phone = request_body["phone"] | |
| customer.postal_code = request_body["postal_code"] | |
| if "name" in request_body or "phone" in request_body or "postal_code" in request_body: | |
| response_body ={} | |
| response_body["name"] = customer.name | |
| response_body["phone"] = customer.phone | |
| response_body["postal_code"] = customer.postal_code | |
| if "name" not in request_body or "phone" not in request_body or "postal_code" not in request_body: | |
| return jsonify(), 400 | |
| customer.name = request_body["name"] | |
| customer.phone = request_body["phone"] | |
| customer.postal_code = request_body["postal_code"] | |
| response_body ={} | |
| response_body["name"] = customer.name | |
| response_body["phone"] = customer.phone | |
| response_body["postal_code"] = customer.postal_code |
There was a problem hiding this comment.
We can refactor this section even more by building the response dictionary in the return statement using the Customer customer_dict method:
| if "name" not in request_body or "phone" not in request_body or "postal_code" not in request_body: | |
| return jsonify(), 400 | |
| customer.name = request_body["name"] | |
| customer.phone = request_body["phone"] | |
| customer.postal_code = request_body["postal_code"] | |
| if "name" in request_body or "phone" in request_body or "postal_code" in request_body: | |
| response_body ={} | |
| response_body["name"] = customer.name | |
| response_body["phone"] = customer.phone | |
| response_body["postal_code"] = customer.postal_code | |
| if "name" not in request_body or "phone" not in request_body or "postal_code" not in request_body: | |
| return jsonify(), 400 | |
| customer.name = request_body["name"] | |
| customer.phone = request_body["phone"] | |
| customer.postal_code = request_body["postal_code"] | |
| db.session.commit() | |
| return jsonify(customer.customer_dict()), 200 |
| return jsonify(response_body), 200 | ||
|
|
||
|
|
||
| @customers_bp.route("/<customer_id>", methods=["DELETE"]) |
|
|
||
|
|
||
| @customers_bp.route("/<customer_id>", methods=["DELETE"]) | ||
| def customer_delete(customer_id): |
| #I DON'T KNOW IF THIS IS CORRECT | ||
| #adding videos attribute to Customer Model | ||
| videos = db.relationship("Video", secondary="rental", backref="customers") |
There was a problem hiding this comment.
The secondary attribute can be removed from this relationship. The secondary attribute is used with pure join tables (whose sole purpose is just to map out the relationship between two tables). In this case, rental is actually not a pure join table and contains its own columns we want to keep track of ( due_date ).
So in this case, adding the relationship with backref to rental in both Video and Customer models would suffice.
rentals = db.relationship("rentals", backref="customers")
No description provided.