-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Frontend changes for the user column migration work. #12158
Conversation
…n for the migrated column.
…di-7607-migrate-user-relationship-columns-to-the-new-user-column-4
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## feature/budi-7607-migrate-user-relationship-columns-to-the-new-user-column #12158 +/- ##
===============================================================================================================
+ Coverage 46.82% 75.52% +28.70%
===============================================================================================================
Files 164 338 +174
Lines 5811 14123 +8312
Branches 1164 2961 +1797
===============================================================================================================
+ Hits 2721 10667 +7946
- Misses 2824 3231 +407
+ Partials 266 225 -41
... and 496 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -105,14 +113,17 @@ function getColumnMigrator( | |||
throw new BadRequestError(`Unknown migration type`) | |||
} | |||
|
|||
class SingleUserColumnMigrator implements ColumnMigrator { | |||
abstract class UserColumnMigrator implements ColumnMigrator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vast majority of the 2 classes that now inherit from this was identical, so it felt natural to use an abstract class here.
export interface MigrationResult { | ||
tablesUpdated: Table[] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created and propagated this type back out of the migrator so that we can fire webhook messages back to the client and ensure the client state is correct. This migration endpoint is unusual in that it affects the current table and another table in the same operation, which the UI doesn't automatically account for.
if (res.status !== expectStatus) { | ||
throw new Error( | ||
`Expected status ${expectStatus} but got ${ | ||
res.status | ||
} with body ${JSON.stringify(res.body)}` | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made error messages from tests that call the migration endpoint significantly easier to debug. I would like functionality akin to this for all of our supertest expects, but I saw no easy way to do this generally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - we might be able to overload the supertest object that the config returns, e.g. add it to what this.request
does - I'll take a look at this!
emitTableUpdate(ctx: any, table: Table, options?: EmitOptions) { | ||
// This was added to make sure that sourceId is always present when | ||
// sending this message to clients. Without this, tables without a | ||
// sourceId (e.g. ta_users) won't get correctly updated client-side. | ||
if (isInternalTable(table._id!)) { | ||
table = processInternalTable(table) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two important changes here:
- The addition of
EmitOptions
to indicate when we want a websocket update to also be processed by the event originator (necessary to update the state of the user table in the UI). - The enriching of the
table
withprocessInternalTable
. This addssourceId
to the table, without which the UI won't use it to update its state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to leave this in here, but I think this maybe needs another look. With this enrichment, and a new app, the users table will not have a sourceId when fetched from the tables list endpoint (as it does not actually exist in the doc), but then will have one when a table change event is broadcast. That means that the original frontend issue of not being able to create a relationship to the users table will still exist, and will only be fixed if the client receives a table change event with this enriched property.
So I think the sourceId issue is a totally separate thing.
@mike12345567 could you confirm if removing the sourceId property from users table in new apps is intended? If so then I'm leaning towards removing this enrichment here, and instead updating the frontend to understand that it's optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sourceId
was really only designed for external datasources - but internal tables had one thrown on just for consistency - if the frontend expects it to be there for all tables then we can update the typing to make it so that it is non-optional on all tables - right now it is required for external but is optional for internal tables, however we can make that required I think.
@samwho - let me know if you'd like a hand with that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried creating a brand new app and inspecting the return values from some API endpoints and here's what I found:
GET /api/tables
returns asourceId
(tacked on, thesourceId
is not present in Couch) on each tableGET /api/tables/ta_users
does not return asourceId
GET /api/tables/ta_<id>
does return asourceId
for another internal table (checking Fauxton, this appears to be because the table document has asourceId
)
@mike12345567 I'd appreciate a hand with this, thank you 🙏
packages/frontend-core/src/components/grid/controls/MigrationModal.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredibly impressive for one of your first features @samwho! Fantastic job. Left a few questions but overall this looks great.
packages/frontend-core/src/components/grid/controls/MigrationModal.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @samwho - fantastic implementation :)
…the-new-user-column' into feature/budi-7607-migrate-user-relationship-columns-to-the-new-user-column-4
9edaca0
into
feature/budi-7607-migrate-user-relationship-columns-to-the-new-user-column
This PR hooks up the frontend to the new
POST /api/tables/:table_id/migrate
endpoint. The video demonstrates how the functionality works.CleanShot.2023-10-25.at.15.42.49.mp4