Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paper - Kaylyn Cardella #63

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Paper - Kaylyn Cardella #63

wants to merge 6 commits into from

Conversation

kecardel
Copy link

No description provided.

kecardel added 6 commits May 18, 2021 22:34
and video.py and defines respective models.
.env created with development and test database URI.
Updates database to reflect attribute changes
to customer.py & video.py
Rental.py populated with model Rental with many-to-many relationship established
…s_by_customer_id. Updates customer.py with relationship to Rental.
Copy link
Contributor

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

There were a few places you could have cleaned up your code but otherwise everything looked great!

postal_code = db.Column(db.Integer)
phone = db.Column(db.String)
registered_at = db.Column(db.DateTime, default=datetime.utcnow())
videos_checked_out_count = db.Column(db.Integer, default=0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this being a column you can use the rentals to count that.

To do that it's helpful to add a relationship:

Suggested change
videos_checked_out_count = db.Column(db.Integer, default=0)
rentals = db.relationship('Rental', back_populates='customer', lazy=True)

"postal_code": self.postal_code,
"phone": self.phone,
"registered_at": self.registered_at,
"videos_checked_out_count": self.videos_checked_out_count
Copy link
Contributor

@kaidamasaki kaidamasaki May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you have the relationship the videos_checked_out_count is just the length of the rentals that haven't been checked in:

Suggested change
"videos_checked_out_count": self.videos_checked_out_count
"videos_checked_out_count": len([rental for rental in self.rentals if not rental.check_in_date])

video_id = db.Column(db.Integer, db.ForeignKey("videos.video_id"))
customers = db.relationship("Customer", backref="rentals")
videos = db.relationship("Video", backref="rentals")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add a check in date to know if the user returned this rental:

Suggested change
check_in_date = db.Column(db.DateTime, nullable=True)

title = db.Column(db.String)
release_date = db.Column(db.DateTime)
total_inventory = db.Column(db.Integer)
available_inventory = db.Column(db.Integer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like with Customer you can compute available_inventory with a relationship:

Suggested change
available_inventory = db.Column(db.Integer)
rentals = db.relationship('Rental', back_populates='video', lazy=True)

"title": self.title,
"release_date": self.release_date,
"total_inventory": self.total_inventory,
"available_inventory": self.available_inventory
Copy link
Contributor

@kaidamasaki kaidamasaki May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like with Customer the available_inventory is just the total inventory minus length of the rentals that haven't been checked in:

Suggested change
"available_inventory": self.available_inventory
"available_inventory": self.total_inventory - len([rental for rental in self.rentals if not rental.check_in_date])


@customers_bp.route("/<customer_id>", methods=["GET"], strict_slashes=False)
def get_customer_by_id(customer_id):
customer = Customer.query.get_or_404(customer_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of get_or_404!

Comment on lines +154 to +155
customer = Customer.query.get(int(rental.customer_id))
video = Video.query.get(int(rental.video_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to just get_or_404 these before trying to make the rental.

Comment on lines +178 to +179
video.available_inventory -= 1
customer.videos_checked_out_count += 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you compute the available_inventory and videos_checked_out_count from the rentals then you don't have to worry about managing these fields and they can't get out of sync.

Comment on lines +246 to +250
rentals = Rental.query.filter_by(customer_id=customer_id).all()

rentals_response = []

for rental in rentals:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rentals = Rental.query.filter_by(customer_id=customer_id).all()
rentals_response = []
for rental in rentals:
rentals_response = []
for rental in customer.rentals:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants