-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
chore:system user and events created by userid migrations #5612
chore:system user and events created by userid migrations #5612
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
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.
Nice. :)
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.
Once you've confirmed that this doesn't break creation of initial admin user I'm all for this.
Co-authored-by: Christopher Kolstad <[email protected]>
Co-authored-by: Christopher Kolstad <[email protected]>
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.
Nice. I assume the where(is_system: false)
will solve the creating admin user.
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.
Adding a test like:
favor-permission-name-over-id.e2e.test.ts
dedupe-permissions.e2e.test.ts
Could potentially be useful just to validate the migration behavior.
Test added, I've updated the username of the system user @nunogois |
INSERT INTO users | ||
(id, name, username, email, created_by_user_id, is_system) | ||
VALUES | ||
(-1337, 'Unleash System', 'unleash_system_user', '[email protected]', -1337, true); |
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.
Even though I like -1337
, I'm wondering if we should just use -1
instead.
expect(userResults[1].is_system).toEqual(false); | ||
expect(userResults[1].id).toEqual(1); | ||
expect(userResults[1].username).toEqual('testperson'); | ||
}); |
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.
Nice 🏅
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.
LGTM!
About the changes
Migrations for:
includes
is_system: false
in the activeUsers where filterTested by running:
select * into users_pre_check from users where id > -1; delete from users where id > -1;
before starting unleash, then inspecting users table after unleash has started and verifying that an 'admin' user has been created.