PR#1595: frontend: copr homepage/contact to be valid or None - copr/copr

copr / copr

#1595 frontend: copr homepage/contact to be valid or None

Merged 4 years ago by praiskup. Opened 4 years ago by praiskup.

Unknown source nullify-homepage-contact into master

frontend: copr homepage/contact to be valid or None

Pavel Raiskup • 4 years ago

b840016

frontend/coprs_frontend/alembic/versions/d6cbf6cd74da_prefer_none_over_empty_string_in_email_.py

file added

+25

		`@@ -0,0 +1,25 @@`
		`+ """`
		`+ prefer None over empty string in email and homepage`
		`+`
		`+ Revision ID: d6cbf6cd74da`
		`+ Revises: b14b27e4a795`
		`+ Create Date: 2020-11-24 09:53:05.334633`
		`+ """`
		`+`
		`+ import sqlalchemy as sa`
		`+ from alembic import op`
		`+`
		`+`
		`+ revision = 'd6cbf6cd74da'`
		`+ down_revision = 'b14b27e4a795'`
		`+`
		`+ def upgrade():`
		`+ session = sa.orm.sessionmaker(bind=op.get_bind())()`
		`+ session.execute("UPDATE copr SET homepage = null WHERE homepage = ''")`
		`+ session.execute("UPDATE copr SET contact = null WHERE contact = ''")`
		`+`
		`+ def downgrade():`
		`+ """`
		`+ No way back is needed, even before this migration we had NULL values in`
		`+ many rows and it did not break anything`
		`+ """`

frontend/coprs_frontend/coprs/forms.py

file modified

+15 -2

		`@@ -365,6 +365,17 @@`
		`return False`
		`return None`

		`+`
		`+ class EmptyStringToNone:`
		`+ """ Transform empty text field to None value """`
		`+ def __call__(self, value):`
		`+ if value is None:`
		`+ return None`
		`+ if value.strip() == "":`
		`+ return None`
		`+ return value`
		`+`
		`+`
		`class CoprFormFactory(object):`

		`@staticmethod`
		`@@ -389,13 +400,15 @@`
		`"Homepage",`
		`validators=[`
		`wtforms.validators.Optional(),`
		`- wtforms.validators.URL()])`
		`+ wtforms.validators.URL()],`
		`+ filters=[EmptyStringToNone()])`

		`contact = wtforms.StringField(`
		`"Contact",`
		`validators=[`
		`wtforms.validators.Optional(),`
		`- EmailOrURL()])`
		`+ EmailOrURL()],`
		`+ filters=[EmptyStringToNone()])`

		`description = wtforms.TextAreaField("Description")`

praiskup commented 4 years ago

Fix forms.py so the empty string in homepage/contact fields are replaced
with None value. That way we fill the database with correct values
(empty string is invalid) and later we can use python-marshmallow to
serialize the Copr info to be sent via APIv2.

I previously tried to fix similar problem in 00f9473. That fix
worked because the invalid empty string values triggered ValidationError
in all previous versions of marshmallow on dump/serialize (sending data
to user). Newer versions of marshmallow though don't validate()
automatically when serializing, only when deserializing! That's why
don't get ValidationError nowadays, and so even invalid values are sent
to the user.

Well, to be precise - previous marshmallow did validate only the
Email and URL versions before, nothing else [1].

So the effect of this patch is that we'll never got invalid values in
database, and all the existing empty values are migrated to None by
Alembic migration.

[1] https://github.com/marshmallow-code/marshmallow/issues/1132

Fixes: #1588

frostyx commented on line 23 of frontend/coprs_frontend/alembic/versions/d6cbf6cd74da_prefer_none_over_empty_string_in_email_.py 4 years ago

Either I understand the sentence incorrectly or s/non-destructive/destructive/ . But that's just a minor thing, the meaning is obivous.

frostyx commented on line 9 of frontend/coprs_frontend/coprs/forms.py 4 years ago

Not sure what will happen if the value is going to be " ". Will it reproduce the issue? I am wondering whether we should .strip() or not.

praiskup commented on line 9 of frontend/coprs_frontend/coprs/forms.py 4 years ago

That should be rejected by URL/EmailOrURL validator, the empty string though passed by because of the Optional validator.

praiskup commented on line 23 of frontend/coprs_frontend/alembic/versions/d6cbf6cd74da_prefer_none_over_empty_string_in_email_.py 4 years ago

I did not write it correctly, by "non-destructive" I meant that there's no way to get back as it can not break anything. There already are fields that are set to None.

rebased onto 9d7997d13886a6cfc47752316af9769e71baf31c

4 years ago

praiskup commented on line 9 of frontend/coprs_frontend/coprs/forms.py 4 years ago

Hmm, you are right .. Optional() takes strip_whitespace=False (default True). So multiple spaces are passed to the database, and not filtered. That's not what I consider a sane default :-(

rebased onto 8bc96a218157f15c543af8e062ee100ebac0dc94

4 years ago

praiskup commented 4 years ago

Both remarks fixed.

frostyx commented 4 years ago

LGTM

rebased onto b840016

4 years ago

praiskup commented 4 years ago

Just rebased before merge, to update the CI badge.

Pull-Request has been merged by praiskup

4 years ago