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 - Gabby Y, Amber S, Areeg J, Angelica Y #13

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

Conversation

gnyoung
Copy link

@gnyoung gnyoung commented Jul 21, 2023

No description provided.

gnyoung and others added 30 commits June 22, 2023 17:09
@gnyoung gnyoung changed the title Kunzite - Gabby, Amber, Areeg, Angelica Kunzite - Gabby Y, Amber S, Areeg J, Angelica Y Jul 21, 2023
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.

Very light feedback. Looks great over all! Just a few recommendations with how your routes are structured.


return make_response(jsonify("All cards successfully deleted!"), 200)

@cards_bp.route("/<card_id>", methods=["PATCH"])

Choose a reason for hiding this comment

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

Since this isn't a general purpose PATCH method, consider adding an additional qualifier to the end (like the mark (in)complete endpoints in task list)

@cards_bp.route("/<card_id>/like", methods=["PATCH"])

Comment on lines +8 to +9
@cards_bp.route("", methods=["POST"])
def create_card():

Choose a reason for hiding this comment

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

👀 We shouldn't be able to create a Card that doesn't belong to a Board. Consider using the nested POST under Board to create a new Card which belongs to that Board, rather than associating an existing Card to an existing Board. That approach was slightly peculiar to Task List, which allowed Tasks to exist that weren't part of an overarching Goal.

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