-
Notifications
You must be signed in to change notification settings - Fork 108
Add military_status to user (fixes #373) #375
Conversation
Also fixes #374 |
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.
@hollomancer - You'll also need to add this new attribute to:
- the list of strong params in the
users_controller
: https://github.com/hollomancer/operationcode_backend/blob/master/app/controllers/api/v1/users_controller.rb#L53-L84 - admin dashboard, strong params: https://github.com/hollomancer/operationcode_backend/blob/master/app/admin/user.rb#L2-L9
- admin dashboard, index view: https://github.com/hollomancer/operationcode_backend/blob/master/app/admin/user.rb#L50-L82
- admin dashboard, create/update form: https://github.com/hollomancer/operationcode_backend/blob/master/app/admin/user.rb#L92-L119
@@ -0,0 +1,5 @@ | |||
class AddMilitarystatusToUser < ActiveRecord::Migration[5.0] | |||
def change | |||
add_column :users, :military_status, :string |
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.
As this is a string
, is the plan to accept any string
, or a specific list of strings for consistency? Meaning a specified set of four statuses, for example. I highly recommend the latter.
This way, we can enforce data integrity, hang logic on the responses, do data analysis, filtering and searching, add rules for certain statuses, etc., etc.
If the answer is yes, then we'll need to do a few extra additions in this PR before shipping it.
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.
Yeah, I agree. Let's do it the right way.
From the frontend, current, veteran, spouse
are the values provided.
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 just need to add the check in /app/models/user.rb
, is that correct?
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.
@hollomancer - To establish the specific set of validated statuses, you'll want to:
- Create a constant for each status here.
- Create a constant to represent an array of all of the statuses. You'll need to include one for
nil
, to account for our current users, and future users, that will not have a status. For example:
CURRENT = 'current'
VETERAN = 'veteran'
SPOUSE = 'spouse'
MILITARY_STATUSES = [CURRENT, VETERAN, SPOUSE, nil]
- In
models/user.rb
, Add aninclusion
validation for the new:military_status
attribute, for theMILITARY_STATUSES
. Here is how to do that. - In
admin/user.rb
, add the set of selections to ActiveAdmin dashboard form portion. For example:
f.input :military_status, as: :select, collection: User::MILITARY_STATUSES.map { |status| [status, status].compact }.reject(&:empty?), include_blank: true
- Add test coverage to
user_test.rb
testing the new inclusion validation - In apiary.apib, in the
military_status
definition, change that to something like:
Users military status, either: current, veteran, spouse, or blank
@hpjaj is this only waiting a review, or are there more changes to be made? |
@dmarchante, this one's on me to finish validation appropriately. |
test/models/user_test.rb
Outdated
@@ -1,6 +1,6 @@ | |||
require 'test_helper' | |||
|
|||
class UserTest < ActiveSupport::TestCase | |||
class UserTest < ActiveSupport::TestCase # rubocop:disable Metrics/ClassLength |
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.
@hpjaj, should we break this up into smaller segments?
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.
No, let's keep all of the user model related test code in this file.
As far as disabling RuboCop, you can:
- do it like this: http://www.mindonrails.com/notes/334-disabling-and-re-enabling-a-rubocop-rule-for-a-specific-set-of-code
- add an exclusion for the test file to
.rubocop.yml
The latter is slightly preferable. If you run into any issues doing it that way, due to the Metrics/ClassLength
setting in .rubocop_todo.yml
, then just go with the former. Not a big deal in this case.
app/admin/user.rb
Outdated
f.input :pay_grade | ||
f.input :military_occupational_specialty | ||
f.input :github | ||
f.input :twitter | ||
f.input :linkedin | ||
f.input :employment_status | ||
f.input :education | ||
f.input :military_status |
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.
You'll want to delete this input (we'll favor the new collection input on line 109).
test/models/user_test.rb
Outdated
@@ -260,4 +260,11 @@ def user_opts | |||
assert_equal data[:zip], userInfo[:zip] | |||
assert_equal '/profile', redirect | |||
end | |||
|
|||
test 'military status accepts valid states' do | |||
assert create(:user, military_status: 'spouse') |
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.
Can you pls change these three to pass the three new constants, to support our validation? i.e.
assert create(:user, military_status: User::SPOUSE).valid?
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 work @hollomancer !
Description of changes
Adds a column for
military_status
, which is currently not present onusers
. This will allow the information we request regarding a military spouse / active duty in signup flow to save to the DB.Issue Resolved
Part of fixing #373 (some frontend work needed)
It's been a while for me. Please review.