Spruce - Mary Tian and Ivette Fernandez

[Mary-Tian]

No description provided.

[anselrognlie]

Well done!

Your code is easy to follow, and your tests all show green in VS Code. There are some warnings for a few of the tests when using pytest from the command line. It's always a good idea to double check the terminal output, since it's less likely to obscure such warnings.

Try looking for repetitive parts of the code and moving them to helper methods. Also, try to make your functions follow the single responsibility principle. So for example, think about splitting your route endpoint functions so that each function handles only one verb.

This also extends to the level of abstraction that we deal with in our routes. 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.

Overall, nicely done! 🎉

[anselrognlie]

👍

One thing we can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a routes folder instead of a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so customer.py, video.py, and rental.py. Where each 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)

[anselrognlie]

There's not a strong need to choose a different name for the model table here. The primary reason we might need to do this is if we were using our model to wrap an existing database table that happens not to follow the naming defaults of SqlAlchemy (matching the model name), or if we were an SQL purist and preferred plural table names to represent that tables do store multiple rows. However, in that case we would generally avoid appending _table to the end of the table name (it's repetitive to have table in name of the table).

In general, it's good to know that the __tablename__ value exists, but we don't have much need to use it.

[anselrognlie]

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.

[anselrognlie]

Keep in mind that the default value for nullable is True. When adding column definitions to our models, we should consider whether it makes sense to allow a record to exist without that data, such as a Customer without a name, a Video without a title, or a Rental without a due date. It's true that we can add checks for this in our logic that adds entries, but we should try to leverage as much validation help from the database as possible. If we tell the database that certain columns are required (not nullable), then we'll encounter an error if we accidentally try to create a row without a value for those columns (which is a good thing)!

[anselrognlie]

Notice that your code doesn't use this relationship in the logic anywhere. Using the secondary attribute on a relationship is more appropriate when we have a simple many-to-many relationship, such as with a pure join table. In that case, a relationship like this would allow SQLAlchemy to automatically manage the join table between the two models being linked and we could establish those links by using the relationship like a collection, by appending, removing, etc. In that case, the backref attribute supplies the equivalent name to attach to the model on the other end of the relationship. Since the local name here is videos, a closer match in semantics for the other side might be customers rather than customer_table.

However, in this application, our rental relationship between videos and customers isn't a pure join table. It has its own information that also needs to be tracked, and which SQLAlchemy can't automatically manage (such as the checked in status, and due date). In this case, using reciprocal one-to-many relationships can bring us some of the benefits of moving around the relationship links, while still leaving it to us to explicitly set up the rental model ourselves. Consider a configuration like the following:

    rentals = db.relationship("Rental", backref="customer")

If we had a customer model in the variable customer, then we could access the rentals associated with the customer as customer.rentals. If there were a similar relationship on the video side, then to get the video title of a particular customer rental, we could write code like customer.rentals[0].video.title. SQLAlchemy takes care of using the foreign key information to fill in the relationships.

All this being said, having this secondary relationship is what allows the deletion of a video or customer which has particiapted in a rental (whether current or in the past). When a video or customer is deleted, SQLAlchemy is tracking that and will delete any rental that also refers to that video or customer. Without this relationship, we might need to look into cascading delete behavior for one-to-many relationships (or try to avoid actually deleting data at all, since data = money).

[anselrognlie]

This is another spot that could benefit from moving the dictionary generation into a helper in the video model.

[anselrognlie]

👍

Good job performing similar validations as for creating a video record through POST.

[anselrognlie]

Here's a third repetition. As I said, if we write the same code three times, that's a great time to start thinking about drying it up.

[anselrognlie]

Similar feedback as for the video routes apply here for the customer routes.

[anselrognlie]

Remember to add a message to your migrations with -m when running the migration step.