-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix double negative "deactivated" column #2882
Conversation
LastName: "Spaceman", | ||
Email: email, | ||
Role: "SYSTEM_ADMIN", | ||
Deactivated: false, |
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 removed this line since the value we would have been passing in by default was Active: true
, which we would no longer be able to overwrite with the assertions
argument. This is due to the fact that mergo
will only overwrite fields that contain a zero value of their type (the zero value for bool
is false). See more in this issue on the mergo
repository, the dependency under the hood of the mergeModels
function below.
This makes user setup in a few places a bit more verbose, for example, in pkg/models/user_test.go. Open to ideas for making this better!
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've run into this too... blah
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.
Thanks for doing this (and sorry I didn't mention this when I went thru your other pr... it did cross my mind, but didn't say anything)
LGTM - Just a mention about having the active
value default to true
... thoughts?
Add an index?
(also, dunno if we want to add a "test" user for each table that is deactivated
and ensure they have active = false
- that's also pretty verbose, so I'm fine w/o this too...)
@@ -0,0 +1,23 @@ | |||
ALTER TABLE users | |||
ADD COLUMN active BOOLEAN NOT NULL DEFAULT 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.
I'd probably error on the safe side and default to active=false and force the handler to update this field to true
...
Also I assume the active column is used when querying these tables... might want to add an index...
LastName: "Spaceman", | ||
Email: email, | ||
Role: "SYSTEM_ADMIN", | ||
Deactivated: false, |
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've run into this too... blah
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 agree with @lynzt . Make those updates and I'm good. Approval in advance here.
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.
@garrettqmartin8 I'm approving as to not be a blocker (since i'll be out today and mon), but maybe run your changes by @chrisgilmerproj before merging...
Description
Double negatives are confusing, so this PR proposes that we use the word
active
instead ofdeactivated
to describe whether or not a user is able to log into Milmove. Another PR will be coming to drop thedeactivated
column so as to follow the zero downtime deploy guidelines.Reviewer Notes
Dropped a note in context below.
Setup
make db_dev_migrate
make server_run
make client_run
Active
column looks rightCode Review Verification Steps
References