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

Trim whitespace of existing assignment and grouping titles #6477

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented Jul 29, 2024

Since #6472 we do the same in the python code, we trim new assignment and course titles. We also normalize empty (``) titles to null.

This PR does the same for existing titles in the DB with a DB migration.

Testing

tox -e dev --run-command 'alembic upgrade head'
dev run-test-pre: PYTHONHASHSEED='378749627'
dev run-test-pre: commands[0] | pip-sync-faster requirements/dev.txt --pip-args --disable-pip-version-check
dev run-test: commands[0] | alembic upgrade head
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade f8ff7e49cbbf -> e5a9845d55d7, Clean grouping and assignment titles.
Assignment titles updated: 5
Empty assignment titles: 1
Grouping titles updated: 2

def upgrade() -> None:
conn = op.get_bind()

result = conn.execute(
Copy link
Member Author

Choose a reason for hiding this comment

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

)
print(f"Assignment titles updated: {result.rowcount}")

result = conn.execute(
Copy link
Member Author

Choose a reason for hiding this comment

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

)
print(f"Empty assignment titles: {result.rowcount}")

result = conn.execute(
Copy link
Member Author

Choose a reason for hiding this comment

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

@marcospri marcospri marked this pull request as ready for review July 29, 2024 08:35
@marcospri marcospri requested a review from acelaya July 29, 2024 08:45
@marcospri
Copy link
Member Author

For #6456

@acelaya
Copy link
Contributor

acelaya commented Jul 29, 2024

Could you add a description to the PR?

@marcospri
Copy link
Member Author

Could you add a description to the PR?

My bad, had to many PRs and marked the wrong one ready for review. Added a bit more context now.

sa.text(
"""
UPDATE assignment SET title = TRIM(title)
WHERE TRIM(title) != title;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really make a difference? Couldn't we just get away with UPDATE assignment SET title = TRIM(title)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, while the where makes the query slower it should lock less rows which should be safer

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point.

@marcospri marcospri merged commit 9ca2539 into main Jul 31, 2024
9 checks passed
@marcospri marcospri deleted the clean-titles branch July 31, 2024 08:16
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.

2 participants