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

Kunzite: Group 5 — Alycia, Barbara, Danqing, Doris #17

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

Conversation

dorl9039
Copy link

No description provided.

Barbara-Bennett and others added 28 commits June 30, 2023 09:05
adding tests for patch route
create a route to view all cards and add a nnew card
adding two card routes to board routes, removing them from card_routes
Add create route to view single board
testing everything, reorganizing for readability
get rid of incorrect routes for getting and creating cards
Adding delete board route & tests
Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

A very light bit of feedback.

"owner": self.owner
}
if self.cards:
board_data["card_count"] = len(self.cards)

Choose a reason for hiding this comment

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

If all we need is the count of records, rather than using len(), which will retrieve the whole records, only to count them and throw them away, look into ways that SqlAlchemy allows us to specifically only get the count of records.

Comment on lines +19 to +20
if self.board:
card_data["board_id"] = self.board_id

Choose a reason for hiding this comment

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

We conditionally included the goal id for task list since it was optional whether a task was part of a goal. Here, we expect all cards to belong to a board (this can even be enforced at a data level by requiring the board id foreign key to be non-null) so we could skip the conditional here.

new_card = Card(
message=card_data["message"],
likes_count=card_data["likes_count"],
board=validate_model(Board, card_data.get("board_id"))

Choose a reason for hiding this comment

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

validate_model belongs to the route layer of functionality. It lives in the routes_helper and uses flask functions like abort and make_response. Try to avoid having lower layer types (like the models) depending on higher layer functionality.

db.session.commit()


return make_response(jsonify({"card_like_count": card.likes_count}), 200)

Choose a reason for hiding this comment

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

Prefer to return the entire updated card record. Doing so would allow the front end to replace the entire card record in state without needing to worry about merging the updated data.

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.

5 participants