Conversation
We keep responses untouched, the only change happens on DB / model side to avoid excessive joins.
|
Note: based on order of merges #610 we need to update alembic migration hash to keep history linear |
Coverage Report for CI Build 24399333383Coverage decreased (-0.002%) to 93.246%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR merges the former user_profile one-to-one table into the main user table to reduce query joins while keeping API responses structurally compatible via a User.profile compatibility shim.
Changes:
- Add a migration to copy
receive_notifications,first_name,last_nameontouserand dropuser_profile. - Remove the
UserProfileSQLAlchemy model and update queries/schemas/controllers to read profile fields fromUser. - Update tests and CLI commands to stop creating
UserProfilerows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/migrations/community/e3f1a9b2c4d6_merge_user_profile_into_user.py | Adds migration to move profile columns to user, migrate data, and drop user_profile. |
| server/mergin/tests/utils.py | Updates test helper to stop creating UserProfile. |
| server/mergin/tests/test_project_controller.py | Removes UserProfile usage in project controller tests. |
| server/mergin/tests/test_auth.py | Removes UserProfile usage in auth tests where it was explicitly instantiated. |
| server/mergin/sync/project_handler.py | Removes join to UserProfile and filters directly on User.receive_notifications. |
| server/mergin/commands.py | Updates DB init command to stop creating UserProfile. |
| server/mergin/auth/schemas.py | Updates profile-related schemas to use User fields directly while keeping schema names/shape compatible. |
| server/mergin/auth/models.py | Adds columns to User, removes UserProfile model, and adds profile compatibility property + name() helper. |
| server/mergin/auth/controller.py | Updates profile update and user listing/search logic to use User fields directly. |
| server/mergin/auth/commands.py | Updates auth CLI create command to stop creating UserProfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| op.create_table( | ||
| "user_profile", | ||
| sa.Column("user_id", sa.Integer(), nullable=False), | ||
| sa.Column("receive_notifications", sa.Boolean(), nullable=False), |
There was a problem hiding this comment.
downgrade() recreates user_profile.receive_notifications as nullable=False, but the original schema created it as nullable (see 2686074eff45_ce_base.py). If you want downgrades to faithfully restore the previous DB shape, align the column nullability with the original (and any later migrations) so older versions of the app won’t encounter schema differences after downgrade.
| sa.Column("receive_notifications", sa.Boolean(), nullable=False), | |
| sa.Column("receive_notifications", sa.Boolean(), nullable=True), |
| # Fill in default for any users without a profile row (should not exist, but be safe) | ||
| op.execute( | ||
| 'UPDATE "user" SET receive_notifications = TRUE WHERE receive_notifications IS NULL;' |
There was a problem hiding this comment.
In the upgrade, NULL receive_notifications values are forced to TRUE. Previously, a NULL in user_profile.receive_notifications would have been excluded by the boolean filter (i.e., treated as “do not notify”). This migration step opts any such legacy rows into notifications and may cause unexpected emails. Consider preserving prior semantics by defaulting NULLs to FALSE, or at least adding a pre-migration assertion/report to confirm no NULLs exist before setting them to TRUE.
| # Fill in default for any users without a profile row (should not exist, but be safe) | |
| op.execute( | |
| 'UPDATE "user" SET receive_notifications = TRUE WHERE receive_notifications IS NULL;' | |
| # Preserve prior semantics for users without a profile row or with legacy NULLs: | |
| # NULL values were previously excluded by boolean filters, so treat them as FALSE. | |
| op.execute( | |
| 'UPDATE "user" SET receive_notifications = FALSE WHERE receive_notifications IS NULL;' |
We keep responses untouched, the only change happens on DB / model side to avoid excessive joins.