Skip to content
This repository was archived by the owner on Jun 27, 2023. It is now read-only.
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def user_params
:github_handle, :twitter_handle, :irc_handle,
:name, :email, :homepage, :location, :bio,
:tech_expertise_list, :tech_interest_list,
:tshirt_size, :tshirt_cut, :postal_address, :timezone,
:tshirt_size, :tshirt_cut, :timezone,
:country,
:hide_email,
:is_company, :company_name, :company_info,
Expand All @@ -107,7 +107,8 @@ def user_params
:application_learning_history, :application_skills, :application_code_samples,
:application_location, :application_minimum_money, :application_money, :application_goals, :application_code_background,
interested_in: [],
roles_attributes: [:id, :name, :team_id, :_destroy]
roles_attributes: [:id, :name, :team_id, :_destroy],
postal_address_attributes: [:id, :line1, :line2, :city, :state, :zip, :country, :_destroy]
)
end
end
6 changes: 6 additions & 0 deletions app/models/postal_address.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
class PostalAddress < ApplicationRecord
belongs_to :user

validates_presence_of :line1, :city, :zip, :country
end
2 changes: 2 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ def student
has_many :applications, through: :teams
has_many :todos, dependent: :destroy
has_many :comments, dependent: :destroy
has_one :postal_address, dependent: :destroy

validates :github_handle, presence: true, uniqueness: { case_sensitive: false }
validates :homepage, format: { with: URL_PREFIX_PATTERN }, allow_blank: true
Expand All @@ -73,6 +74,7 @@ def student
validates :name, :email, :country, :location, presence: true, unless: :github_import

accepts_nested_attributes_for :roles, allow_destroy: true
accepts_nested_attributes_for :postal_address, allow_destroy: true, reject_if: :all_blank

before_save :normalize_location
after_create :complete_from_github
Expand Down
3 changes: 3 additions & 0 deletions app/views/postal_addresses/_postal_address.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
= @user.postal_address.line1.titlecase + " " + @user.postal_address.line2.titlecase + " "
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this being a bit contra Rails' conventions, I'd say this is making the maintainability of the app a bit harder 😉

The partial being in postal_address/_postal_address, you could render it by just calling

= render @user.postal_address

Internally, this will pass a local assigned variable with the name postal_address to the partial - so there's a variable called postal_address available in the partial (same name as the model).

To fully make sure everything is according to plan, you could even fetch this variable:

# I personally prefer to do a ruby: section in the beginning
ruby:
  postal_address = local_assigns.fetch(:postal_address)

= postal_address.line1

# etc.

Using @user here would be bypassing all this nice assignment magic that Rails does for us. Also - and that's the main point - it makes the partial incompatible for any other use aside from views with an @user set 😢

= @user.postal_address.city.titlecase + ", " + @user.postal_address.state.titlecase + " "
= @user.postal_address.zip + " " +@user.postal_address.country
14 changes: 13 additions & 1 deletion app/views/users/edit.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,19 @@
p.help-block This information will only be visible to yourself and the organizers.
= f.input :tshirt_size, as: :select, collection: User::TSHIRT_SIZES.map { |k| [k, k] }, label: 'T-Shirt size', blank: false, required: false, hint: 'For sponsor T-Shirts, in case they send some.'
= f.input :tshirt_cut, as: :select, collection: User::TSHIRT_CUTS.map { |s| [s, s] }, label: 'T-Shirt cut', include_blank: true
= f.input :postal_address, hint: "Please give your postal address, including your full name, so we can send things we've received from our sponsors for you :)"
h4 Shipping Address
= f.simple_fields_for :postal_address_attributes, @user.postal_address do |pa|
- if @user.postal_address
= pa.input :id, as: :hidden, input_html: { value: @user.postal_address.id }
= pa.check_box '_destroy'
| Remove Address
= pa.input :line1, required: false
= pa.input :line2, required: false
= pa.input :city, required: false
= pa.input :state, required: false
= pa.input :zip, required: false
= pa.input :country, prompt: "Select your country", required: false, include_blank: true


- if admin?
h3.page-header Roles
Expand Down
6 changes: 5 additions & 1 deletion app/views/users/show.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,13 @@ nav.actions
p = @user.company_info

- if can_see_private_info?
- fields = ['tshirt_size', 'tshirt_cut', 'postal_address']
.well.private-info
h3 Private info
- if @user.postal_address
h4 Postal Address
p
Copy link
Member

Choose a reason for hiding this comment

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

Has been some time that I read slim template code 🙈

What's the point of this empty paragraph? Should the postal-address partial rendered in a paragraph? If yes, then I think™ the way to go would be_

p
  = render ...

But looking at the partial not using any markup at all, I'd rather suggest to do the markup in there.

= render @user.postal_address
- fields = ['tshirt_size', 'tshirt_cut']
- fields.each do |field|
- if @user.send(field).present?
p
Expand Down
15 changes: 15 additions & 0 deletions db/migrate/20181107171037_create_postal_addresses.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
class CreatePostalAddresses < ActiveRecord::Migration[5.1]
def change
Copy link
Member

Choose a reason for hiding this comment

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

Not suuuuper necessary, but it would come in quite handy if you could just squash the two migrations.

For instance: roll back the two migrations, edit this one to have the right names (and delete the other migration) and then run it again.

create_table :postal_addresses do |t|
t.string :address_line_1
t.string :address_line_2
t.string :city
t.string :state_or_province
t.string :postal_code
t.string :country

t.references :user, foreign_key: true
t.timestamps
end
end
end
5 changes: 5 additions & 0 deletions db/migrate/20181107172409_remove_postal_address_from_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RemovePostalAddressFromUsers < ActiveRecord::Migration[5.1]
def change
remove_column :users, :postal_address, :text
Copy link
Member

Choose a reason for hiding this comment

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

Should we care about existing users having set up a postal_address already? Would there be a way to migrate their data to the new schema?

end
end
8 changes: 8 additions & 0 deletions db/migrate/20181113210909_fix_postal_address_column_names.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class FixPostalAddressColumnNames < ActiveRecord::Migration[5.1]
def change
rename_column :postal_addresses, :address_line_1, :line1
rename_column :postal_addresses, :address_line_2, :line2
rename_column :postal_addresses, :state_or_province, :state
rename_column :postal_addresses, :postal_code, :zip
end
end
17 changes: 15 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20180714153306) do
ActiveRecord::Schema.define(version: 20181113210909) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
Expand Down Expand Up @@ -173,6 +173,19 @@
t.index ["user_id"], name: "index_notes_on_user_id"
end

create_table "postal_addresses", force: :cascade do |t|
t.string "line1"
t.string "line2"
t.string "city"
t.string "state"
t.string "zip"
t.string "country"
t.bigint "user_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["user_id"], name: "index_postal_addresses_on_user_id"
end

create_table "projects", id: :serial, force: :cascade do |t|
t.string "name", limit: 255
t.datetime "created_at"
Expand Down Expand Up @@ -296,7 +309,6 @@
t.string "twitter_handle", limit: 255
t.string "irc_handle", limit: 255
t.string "tshirt_size", limit: 255
t.text "postal_address"
t.string "timezone", limit: 255
t.string "interested_in", limit: 255, default: [], array: true
t.boolean "hide_email"
Expand Down Expand Up @@ -339,5 +351,6 @@
t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
end

add_foreign_key "postal_addresses", "users"
add_foreign_key "teams", "projects"
end
11 changes: 11 additions & 0 deletions spec/factories/postal_address.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
FactoryBot.define do
factory :postal_address do
user
line1 { FFaker::AddressUS.street_address }
line2 { FFaker::AddressUS.secondary_address }
city { FFaker::AddressUS.city}
state { FFaker::AddressUS.state}
zip { FFaker::AddressUS.zip_code}
country { "United States" }
end
end
69 changes: 69 additions & 0 deletions spec/features/users/postal_address_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
require 'rails_helper'

RSpec.describe 'Add Postal Address', type: :feature do
let(:user) { create(:user) }
let(:address) { create(:postal_address) }
Copy link
Member

Choose a reason for hiding this comment

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

As said in a previous comment, I'd personally prefer to just use actual strings, like "Canada" in the fill_in bits, rather then an address build via a factory... Nevertheless, using create here is not what we want. We don't want to create an address in the database BEFORE the test -> it's more the outcome of the test 😉


context 'signed in' do
before { sign_in user }

context 'in the user edit page' do
before { visit edit_user_path(user) }

it 'allows creation of postal address if all required address fields are entered' do
fill_in 'Line1', with: address.line1
fill_in 'Line2', with: address.line2
fill_in 'City', with: address.city
fill_in 'State', with: address.state
fill_in 'Zip', with: address.zip
select address.country, from: 'Country'
click_on 'Save'

expect(current_path).to eq user_path(user)

expect(page).to have_content address.line1
expect(page).to have_content address.line2
expect(page).to have_content address.city
expect(page).to have_content address.state
expect(page).to have_content address.zip
expect(page).to have_content 'US'
end

it 'does not allow a postal address to be added if required fields are missing' do
fill_in 'Line1', with: address.line1
fill_in 'Zip', with: address.zip
click_on 'Save'

expect(page).to have_content "Postal address city can't be blank"
expect(page).to have_content "Postal address country can't be blank"
end
end

context 'when the user already has a postal address set up' do
let!(:postal_address) { create(:postal_address, user: user) }

it 'autofills saved postal address info on edit form if it exists' do
visit edit_user_path(user)

expect(page).to have_selector("input[value='#{user.postal_address.line1}']")
expect(page).to have_selector("input[value='#{user.postal_address.city}']")
expect(page).to have_selector("input[value='#{user.postal_address.state}']")
expect(page).to have_selector("input[value='#{user.postal_address.zip}']")
end

it 'allows to delete an existing postal address' do
visit edit_user_path(user)

check 'user[postal_address_attributes][_destroy]'
click_on 'Save'

expect(current_path).to eq user_path(user)
expect(page).to_not have_content postal_address.line1
expect(page).to_not have_content postal_address.line2
expect(page).to_not have_content postal_address.city
expect(page).to_not have_content postal_address.state
expect(page).to_not have_content postal_address.zip
end
end
end
end
14 changes: 14 additions & 0 deletions spec/models/postal_address_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
require 'rails_helper'

RSpec.describe PostalAddress, type: :model do
describe 'associations' do
it { is_expected.to belong_to(:user) }
end

describe 'validations' do
it { is_expected.to validate_presence_of(:line1)}
it { is_expected.to validate_presence_of(:city)}
it { is_expected.to validate_presence_of(:zip)}
it { is_expected.to validate_presence_of(:country)}
end
end
1 change: 1 addition & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
it { is_expected.to have_many(:applications).through(:teams) }
it { is_expected.to have_many(:todos).dependent(:destroy) }
it { is_expected.to have_many(:comments).dependent(:destroy) }
it { is_expected.to have_one(:postal_address).dependent(:destroy) }
end

describe 'validations' do
Expand Down