-
Notifications
You must be signed in to change notification settings - Fork 137
Postal address fields #1040
base: master
Are you sure you want to change the base?
Postal address fields #1040
Changes from 12 commits
c954a2a
549d370
08bb3d2
dd200a3
752ab32
0992f1f
f3d0205
020fc85
30eb3b3
e57bc9c
89c8a4c
2eea06b
e73d7fa
b06aa4c
70ecfbc
f92b1c2
978544e
6b07d34
8565c6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # frozen_string_literal: true | ||
| class PostalAddress < ApplicationRecord | ||
| belongs_to :user | ||
| validates_presence_of :address_line_1, :city, :postal_code, :country | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
|
|
||
| def formatted | ||
| "#{capitalize(address_line_1)} #{capitalize(address_line_2)} #{capitalize(city)}, #{state_or_province} #{postal_code} #{country}" | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| end | ||
|
|
||
| private | ||
|
|
||
| def capitalize(string) | ||
| string.split.map(&:capitalize).join(' ') | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,7 +107,17 @@ | |
| 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 | ||
| - if @user.postal_address | ||
| = link_to "Remove Address", user_path(@user, user: { postal_address_attributes: { id: @user.postal_address.id, "_destroy" => true }}), method: :put, data: { confirm: 'Are you sure?' } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I'm not 100% about this pattern, are we using it elsewhere in the form as well? I mean on the one hand it's of course cleverly reusing existing controller actions, but on the other, people may be surprised with the page being refreshed and all their other changes being gone 🤔 Have you considered this as well?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! I've made an update to make this a checkbox rather than a redirect link. I'll push it up shortly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Do you like a checkbox better? I cannot really imagine how this looks like 🤔 Can you add a screenshot please? |
||
| = f.simple_fields_for :postal_address_attributes, @user.postal_address do |pa| | ||
| = pa.input :address_line_1, required: false | ||
| = pa.input :address_line_2, required: false | ||
| = pa.input :city, required: false | ||
| = pa.input :state_or_province, required: false | ||
| = pa.input :postal_code, required: false | ||
| = pa.input :country, prompt: "Select your country", required: false, include_blank: true | ||
|
|
||
|
|
||
| - if admin? | ||
| h3.page-header Roles | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Has been some time that I read 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. |
||
| = @user.postal_address.formatted | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| - fields = ['tshirt_size', 'tshirt_cut'] | ||
| - fields.each do |field| | ||
| - if @user.send(field).present? | ||
| p | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| class CreatePostalAddresses < ActiveRecord::Migration[5.1] | ||
| def change | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we care about existing users having set up a |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| FactoryBot.define do | ||
| factory :postal_address do | ||
| user | ||
| address_line_1 { FFaker::AddressUS.street_address } | ||
| address_line_2 { FFaker::AddressUS.secondary_address } | ||
| city { FFaker::AddressUS.city} | ||
| state_or_province { FFaker::AddressUS.state} | ||
| postal_code { FFaker::AddressUS.zip_code} | ||
| country { FFaker::Address.country } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe 'Add Postal Address', type: :feature do | ||
| let(:user) { create(:user) } | ||
| let(:address) { create(:postal_address) } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| 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 'user[postal_address_attributes][address_line_1]', with: address.address_line_1 | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| fill_in 'user[postal_address_attributes][address_line_2]', with: address.address_line_2 | ||
| fill_in 'user[postal_address_attributes][city]', with: address.city | ||
| fill_in 'user[postal_address_attributes][state_or_province]', with: address.state_or_province | ||
| fill_in 'user[postal_address_attributes][postal_code]', with: address.postal_code | ||
| select address.country, from: 'user[postal_address_attributes][country]' | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| click_on 'Save' | ||
|
|
||
| expect(current_path).to eq user_path(user) | ||
|
|
||
| expect(page).to have_content address.address_line_1 | ||
| expect(page).to have_content address.address_line_2 | ||
| expect(page).to have_content address.city | ||
| expect(page).to have_content address.state_or_province | ||
| expect(page).to have_content address.postal_code | ||
| expect(page).to have_content user.postal_address.country | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| end | ||
|
|
||
| it 'does not allow a postal address to be added if required fields are missing' do | ||
| expect(user.postal_address).to be_nil | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| fill_in 'user[postal_address_attributes][address_line_1]', with: address.address_line_1 | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| fill_in 'user[postal_address_attributes][postal_code]', with: address.postal_code | ||
| click_on 'Save' | ||
|
|
||
| expect(page).to have_content "Postal address city can't be blank" | ||
|
lilwillifo marked this conversation as resolved.
|
||
| expect(page).to have_content "Postal address country can't be blank" | ||
| end | ||
|
|
||
| it 'autofills saved postal address info on edit form if it exists' do | ||
| user.update(postal_address: create(:postal_address)) | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| visit edit_user_path(user) | ||
|
|
||
| expect(page).to have_selector("input[value='#{user.postal_address.address_line_1}']") | ||
| expect(page).to have_selector("input[value='#{user.postal_address.city}']") | ||
| expect(page).to have_selector("input[value='#{user.postal_address.state_or_province}']") | ||
| expect(page).to have_selector("input[value='#{user.postal_address.postal_code}']") | ||
| end | ||
|
|
||
| it 'lets you delete an existing postal address' do | ||
|
lilwillifo marked this conversation as resolved.
Outdated
|
||
| user.update(postal_address: address) | ||
| visit edit_user_path(user) | ||
|
|
||
| accept_alert do | ||
| click_on 'Remove Address' | ||
| end | ||
|
|
||
| expect(current_path).to eq user_path(user) | ||
|
|
||
| expect(page).to_not have_content address.address_line_1 | ||
| expect(page).to_not have_content address.address_line_2 | ||
| expect(page).to_not have_content address.city | ||
| expect(page).to_not have_content address.state_or_province | ||
| expect(page).to_not have_content address.postal_code | ||
| end | ||
| end | ||
| end | ||
| end | ||
| 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(:address_line_1)} | ||
| it { is_expected.to validate_presence_of(:city)} | ||
| it { is_expected.to validate_presence_of(:postal_code)} | ||
| it { is_expected.to validate_presence_of(:country)} | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.