Postal addresses #1032
Postal addresses #1032
Conversation
|
👋 @shebesabrina thanx a ton for getting involved 💚 Before I get into an actual review of the code: it would be super neat if you could just also add the screenshots you posted in #868 here to the PR. Just makes it easier for people reviewing it to get an idea about what's happening on the visual side. |
There was a problem hiding this comment.
Wow! Solid work @shebesabrina 💪
I left some comments here and there. Most important are: we should probably only have one address per user and could therefore use Rails' nested attributes. Plus we should consider the case where users don't need to provide an address.
Ping me if anything is unclear or you need any more information on certain things here ✌️
| @@ -68,8 +68,10 @@ group :test do | |||
| gem 'rspec-html-matchers' | |||
| gem 'rubocop', require: false | |||
| gem 'selenium-webdriver' | |||
| gem 'chromedriver-helper' | |||
There was a problem hiding this comment.
Can you please remove this gem again from the Gemfile? It does not match the Selenium / Chrome setup we use for the feature specs on this project and having a quick look at the CI output for this PR, it looks also like this is the cause for it being 🔴 for feature specs.
I know that this gem is an alternative way to install Chromedriver, but I have only had bad experience with it and in general don't like the idea of "misusing" gems to install system dependencies 😉 - it usually turns out to be a shortcut that's not worth it after all.
If you ran into any problems with setting up the dependencies for the app, just check the README and especially our TROUBLESHOOTING guide, we have a dedicated section for Chromedriver there 📚
| gem 'shoulda-matchers', git: 'https://github.com/thoughtbot/shoulda-matchers.git' | ||
| gem 'simplecov', require: false | ||
| gem 'timecop' | ||
| gem 'pry-rails' |
There was a problem hiding this comment.
Can you also please remove this gem from the Gemfile? I know it's super handy and you're free to use pry when working on the app. We try to not have any "unrelated" changes to the actual purpose of a PR (in your case all things addresses).
You can totally update the setup to suit your personal workflow while developing the app - just don't commit these changes when opening a PR.
| @@ -444,4 +454,4 @@ RUBY VERSION | |||
| ruby 2.5.1p57 | |||
|
|
|||
| BUNDLED WITH | |||
| 1.16.1 | |||
| 1.16.4 | |||
There was a problem hiding this comment.
As explained in my comments above: as it's not really relevant for the actual changes in this PR, it would be best if you could just unstage the changes on the Gemfile and Gemfile.lock. You can still keep these changes on your local fork if they make working with the app easier for you - just exclude them from this PR ✌️
| @@ -0,0 +1,6 @@ | |||
| # frozen_string_literal: true | |||
| class PostalAddressesController < ApplicationController | |||
There was a problem hiding this comment.
Are you planning to make use of this controller later? I may overlook things, but I don't see it being used right now 👀
If you're not using it, best get rid of it. Any unused code is code we have to maintain and results in some work 😉
| @@ -29,6 +29,7 @@ def create | |||
| def update | |||
| respond_to do |format| | |||
| if @user.update_attributes(user_params) | |||
| @user.postal_addresses.create(address_params) | |||
There was a problem hiding this comment.
Hm, doing so means I'd create a new address record every time I update my user profile. What's the benefit of this?
I may not understand the details of this, but I fear we'll just pollute our database with loads of address records then - even though we'd only need one per student 🙈
I'd suggest a different approach: you could just allow the address attributes as nested_attributes on the User model and include them in the already permitted user_params. Doing so, we can stay with only one line like
@user.update_attributes(user_params)And just update (or create) a single address record per user.
| interested_in: [], | ||
| roles_attributes: [:id, :name, :team_id, :_destroy] | ||
| ) | ||
| end | ||
|
|
||
| def address_params |
There was a problem hiding this comment.
See my comment above in the update method: I think using Rails' nested_attributes would be a even better fit here. Just check the Rails guides on this, it's should be pretty well documented there. Ping me if you need any help or guidance 👋
| @@ -65,6 +65,7 @@ def student | |||
| has_many :applications, through: :teams | |||
| has_many :todos, dependent: :destroy | |||
| has_many :comments, dependent: :destroy | |||
| has_many :postal_addresses | |||
There was a problem hiding this comment.
Why has_many? I'd personally have gone for has_one 🤔
I don't think we need more than one address per student.
| = f.simple_fields_for :postal_addresses do |pa| | ||
| = pa.input :street | ||
| = pa.input :state | ||
| = pa.input :zip, required: true, hint: "Please give your postal address, so we can send things we've received from our sponsors for you :)" |
There was a problem hiding this comment.
See my comments above about making use of nested_attributes here.
Aside that: why making only one of the fields required? And what about users who don't want to and don't need to provide an address? We only require the address from students, other users of the app are not required to provide this information. With making the field required like it is now, no user is able to update their profile without adding an address with a zipcode 😉
Also: the hint does not make too much sense for the zip field alone. This looks more like a general hint we'd put above or below the address fields. I'd therefore remove it from the individual field and put it on the address fields in general.
Related issue #
User profile: postal addresses (#868)
Adds multiple fields for a shipping address and extracts the postal address as its own model.