rename participant field (fixes #136) #142
Conversation
backend/tests/test_db.py
Outdated
| @@ -29,18 +29,18 @@ def test_add_new_participant_to_db(_db, new_participant): | |||
| assert len(db.session.query(Participant).all()) == 0 | |||
|
|
|||
| new_participant = Participant( | |||
| name=new_participant['name'], | |||
| lastname=new_participant['lastname'], | |||
| first_name=new_participant['first_name'], | |||
There was a problem hiding this comment.
We could do that as new_participant = Participant(**new_participant)
backend/tests/test_models.py
Outdated
| @@ -19,14 +19,14 @@ def test_create_new_user(new_user): | |||
| def test_create_new_participant(new_participant): | |||
| """Test participant model.""" | |||
| new_participant = Participant( | |||
| name=new_participant['name'], | |||
| lastname=new_participant['lastname'], | |||
| first_name=new_participant['first_name'], | |||
There was a problem hiding this comment.
Ditto: new_participant = Participant(**new_participant)
| @@ -26,7 +26,7 @@ def upgrade(): | |||
| op.create_table('participant', | |||
| sa.Column('id', sa.Integer(), autoincrement=True, nullable=False), | |||
| sa.Column('name', sa.String(length=20), nullable=True), | |||
| sa.Column('lastname', sa.String(length=20), nullable=True), | |||
| sa.Column('last_name', sa.String(length=20), nullable=True), | |||
backend/factories.py
Outdated
| email = factory.LazyAttribute( | ||
| lambda obj: "{}@cfp.com".format(obj.lastname) | ||
| lambda obj: "{}@cfp.com".format(obj.last_name) |
There was a problem hiding this comment.
We don't own cfp.com domain, it's not connected with us anyhow. We should change domain here to codeforpoznan.pl od stick to reserved, testing domains (like example.com, or codeforpoznan.test).
There was a problem hiding this comment.
changed to "codeforpoznan.test"
|
|
||
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.add_column('participant', sa.Column('first_name', sa.String(length=50), nullable=True)) |
There was a problem hiding this comment.
We should use alter_column here, not drop_column and add_column as altering will preserve data.
There was a problem hiding this comment.
It has been done automatically by alembic
There was a problem hiding this comment.
Not sure about this one with alembic. 😕
There was a problem hiding this comment.
@magul, we are waiting for your re-review
There was a problem hiding this comment.
I understand, that Alembic could create such migration, but that's just a tool and in that case, it does it wrong. We're not on production right now, but as a learning exercise, we should prepare that migration in such manner, that it could be safely run on a production environment.
Right now that migration will drop existing columns an create new ones. Data previously stored in removed columns are permanently lost. I belive we should use here https://alembic.sqlalchemy.org/en/latest/ops.html?highlight=unique#alembic.operations.Operations.alter_column
There was a problem hiding this comment.
@magul
I made all the requested changes in migration files. I checked the command flask db upgrade and flask db migrate - everything worked. When I opened the database in dbeaver it had column names: first_name and last_name. I hope that now everything works ok, but it would be nice if you could check :)
|
|
||
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.add_column('participant', sa.Column('first_name', sa.String(length=50), nullable=True)) |
There was a problem hiding this comment.
I understand, that Alembic could create such migration, but that's just a tool and in that case, it does it wrong. We're not on production right now, but as a learning exercise, we should prepare that migration in such manner, that it could be safely run on a production environment.
Right now that migration will drop existing columns an create new ones. Data previously stored in removed columns are permanently lost. I belive we should use here https://alembic.sqlalchemy.org/en/latest/ops.html?highlight=unique#alembic.operations.Operations.alter_column
| existing_type=sa.VARCHAR(length=20), | ||
| type_=sa.Date(), | ||
| existing_nullable=True) | ||
| op.execute("""ALTER TABLE hacknight ALTER COLUMN date TYPE DATE USING date::date""") |
There was a problem hiding this comment.
@magul there was some problem with the upgrade of database with date type. It required: "USING date::date" and in SQLAlchemy it was impossible to add this "USING" part. With @stanislawK we were looking for different solutions and we found that we should use pure SQL to solve this problem.
I hope that it is correct. I don't know how to test it.