Skip to content

Conversation

@renefs
Copy link
Contributor

@renefs renefs commented May 22, 2025

  • Handles empty names on avatars for users and participants.
  • Adds a name_not_empty validation in the participant schema.
  • Adds a migration to set to "?" the existing empty participant names

Fixes #502

@ThiefMaster
Copy link
Member

DB constraint is maybe a bit too much for it, how about just rejecting it in the schema?

def not_empty(value):
    if not value:
        raise ValidationError('This field cannot be empty.')

@renefs renefs force-pushed the 502_fix_empty_name branch from 5023f8f to 42b90d2 Compare May 23, 2025 07:21
@renefs renefs force-pushed the 502_fix_empty_name branch from 42b90d2 to c861b55 Compare July 4, 2025 07:21
@github-actions
Copy link

github-actions bot commented Jul 4, 2025

This PR contains database changes. Before merging it, make sure to apply the migration in production:

BEGIN;

-- Running upgrade 30b5fe7b5949 -> cab5d47c1152

UPDATE participants
        SET name = '?'
        WHERE name= '';;

UPDATE alembic_version SET version_num='cab5d47c1152' WHERE alembic_version.version_num = '30b5fe7b5949';

COMMIT;

When reviewing the PR, make sure that the changes will not break the previously deployed
version, i.e. any new column needs to have a server_default or be nullable.

@renefs renefs force-pushed the 502_fix_empty_name branch from c861b55 to b46b8e1 Compare July 4, 2025 09:29
@renefs
Copy link
Contributor Author

renefs commented Jul 4, 2025

I wonder if it's worth to add anything to the downgrade code like:

def downgrade():
    op.execute(
        """
        UPDATE participants
        SET name = NULL
        WHERE name = '?';
        """
    )

Since this is fixing a "bug"... probably not?

@ThiefMaster
Copy link
Member

I wonder if it's worth to add anything to the downgrade code like:

No, it's a fix, so it makes sense to leave the downgrade step empty.

@ThiefMaster
Copy link
Member

ThiefMaster commented Jul 4, 2025

For the broken comment about the alembic revision here in this PR, can you try removing these 3 lines?

https://github.com/indico/newdle/blob/master/ci/print_revision_sql.sh#L14-L16

Not sure why they were needed...

Edit: Ah, probably because it gets passed into github output later... https://github.com/indico/newdle/blob/master/.github/workflows/migration-sql-comment.yml#L48
But it looks like the peter-evans/create-or-update-comment actions now supports body-path to point to a file instead of hardcoding a string, which should remove the need to use output altogether.

@renefs renefs force-pushed the 502_fix_empty_name branch 2 times, most recently from c5e4c85 to d445709 Compare July 4, 2025 09:42
renefs and others added 2 commits July 4, 2025 12:29
Set default name for participants with no name in migration
@ThiefMaster ThiefMaster force-pushed the 502_fix_empty_name branch from a193524 to 28b80bb Compare July 4, 2025 10:29
@ThiefMaster ThiefMaster enabled auto-merge (squash) July 4, 2025 10:30
@ThiefMaster ThiefMaster merged commit 663795f into indico:master Jul 4, 2025
2 checks passed
@tomasr8
Copy link
Member

tomasr8 commented Jul 4, 2025

Thanks @renefs!!

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.

Do not allow participants w/ empty name

3 participants