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

[153] unique constraints #237

Merged
merged 25 commits into from
Apr 29, 2020
Merged

Conversation

w1stler
Copy link
Member

@w1stler w1stler commented Jan 22, 2020

To test:

  • login in Postman
  • try to create participant without first_name, last_name
  • try to create participant twice (with same email)
  • try to create hacknight without date
  • try to create hacknight with the same date

@w1stler w1stler changed the title [WIP] [153] unique constraints [153] unique constraints Jan 22, 2020
@w1stler w1stler changed the title [153] unique constraints [WIP] [153] unique constraints Jan 22, 2020
@w1stler w1stler changed the title [WIP] [153] unique constraints [153] unique constraints Jan 31, 2020
Copy link
Member

@magul magul left a comment

Choose a reason for hiding this comment

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

One additional thing - would be great to have some meaningful human-readable description of alembic migration (see: https://alembic.sqlalchemy.org/en/latest/autogenerate.html)

email = Column(String(200))
first_name = Column(String(50), nullable=False)
last_name = Column(String(50), nullable=False)
email = Column(String(200), unique=True, nullable=False)
github = Column(String(200), default="")
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be github login also unique?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made some amendments here.

github = fields.Str(validate=[validate.Length(max=200)])
last_name = fields.Str(required=True, validate=[validate.Length(max=50)])
email = fields.Email(required=True, validate=[validate.Length(max=200)])
github = fields.Str(required=True, validate=[validate.Length(max=200)])
hacknights = fields.Nested("HacknightSchema", exclude=("participants",), many=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider validating minimum length of database not-nullable fields (first_name, last_name, github) to avoid adding participants with empty strings ("").

Copy link
Member Author

@w1stler w1stler Apr 4, 2020

Choose a reason for hiding this comment

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

I will create a separate ticket for adding regex validation for all fields (just not to create big pull requests).

if (
Participant.query.filter(Participant.email == json_data["email"]).first()
is not None
):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is not None could be removed.

Copy link
Contributor

@stanislawK stanislawK left a comment

Choose a reason for hiding this comment

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

Is not worth to add some tests for those changes? What do You think?

first_name = fields.Str(required=True, validate=[validate.Length(min=3, max=50)])
last_name = fields.Str(required=True, validate=[validate.Length(min=3, max=50)])
email = fields.Email(required=True, validate=[validate.Length(min=5, max=200)])
github = fields.Str(required=True, validate=[validate.Length(min=5, max=200)])
Copy link
Member

Choose a reason for hiding this comment

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

@w1stler w1stler changed the title [153] unique constraints [wip] [153] unique constraints Feb 20, 2020
@w1stler w1stler changed the title [wip] [153] unique constraints [153] unique constraints Apr 4, 2020
@@ -29,6 +29,21 @@ def post(self):
except ValidationError as err:
return (err.messages), HTTPStatus.BAD_REQUEST

if Participant.query.filter(Participant.email == json_data["email"]).first():
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be:
if Participant.query.filter_by(email=json_data["email"]).first():

if (
Participant.query.filter(Participant.github == json_data["github"]).first()
is not None
):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is not None could be removed.
Also You could use here:
if Participant.query.filter_by(github=json_data["github"]).first():

@w1stler w1stler merged commit a812963 into CodeForPoznan:master Apr 29, 2020
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.

4 participants