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

Kim 164906617 super user migrations #1940

Merged
merged 5 commits into from
Apr 1, 2019

Conversation

kimallen
Copy link
Contributor

@kimallen kimallen commented Mar 29, 2019

Description

Adds a column is_superuser to the users table.
Adds all current Truss engineers as superusers (except two who have not been added yet as office_users).

Reviewer Notes

The user is created in middleware when the officeuser is created (not in migrations), so running prod migrations does not show user data, so this is difficult to verify.

When I ran the local migration, it seemed to pass, but it is not setting is_superuser to true for me locally, although it did for @mikena-truss when she tried it out. Please uncomment the query in the local migration and see if the is_superuser value gets applied locally.

Setup

To check locally, uncomment the query in the local_migration file.
If you already have your local db set up, run make db_dev_migrate. Then check your db to find 1) a new is_superuser column. 2) SELECT * FROM users WHERE login_gov_email = 'officeuser1@example.com'; and see is_superuser set to true

Code Review Verification Steps

  • Secure migrations have been tested using bin/run-prod-migrations - Note: does not show users table data because it's created in middleware instead of migrations.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #1940 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   60.51%   60.51%   +0.01%     
==========================================
  Files         193      193              
  Lines       12245    12247       +2     
==========================================
+ Hits         7409     7411       +2     
  Misses       3968     3968              
  Partials      868      868

Copy link
Contributor

@lynzt lynzt left a comment

Choose a reason for hiding this comment

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

Looks good to @garrettqmartin8 and me... 🚢

Sidenote: officeuser1 isn't created till we load data via e2e_basic.go which is run after the migration files are run. Therefore, that user isn't in the db at the time the migration runs. Mikena probably ran the migration but didn't drop the existing data so her field was updated as expected...

@kimallen
Copy link
Contributor Author

kimallen commented Apr 1, 2019

Aaah! Thank you @lynzt - that makes sense- I thought it was something like that. . .

@kimallen kimallen merged commit 7d31599 into master Apr 1, 2019
@kimallen kimallen deleted the kim-164906617-super-user-migrations branch April 1, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants