Postal address fields

[lilwillifo]

Related issue #868

@klappradla 👋
This extracts Postal Addresses to their own DB backed model.

A user can add, edit, or remove a postal address. If left blank, the profile will save, but if the address is partially filled out, it alerts them of any missing required fields there.

I didn't get into address validation. Not sure if you want to pull in a GoogleMaps type gem to confirm that valid addresses are coming in, or if you're okay with relying on user input here.

I haven't included phone number here as part of this, but that can be easily added if that's what's required. It felt a little strange to include a phone number in a postal address model for shipping, so I left it off for now.

One note: there's one failing test. It's failing on master for me, so not sure if this is just a local issue for me or something your team is aware of.

1. Season.transition? on New Year's should not be transition
   Failure/Error: it { is_expected.not_to be_transition }
   expected Season(id: integer, starts_at: date, ends_at: date, name: string, created_at: datetime, updated_at: d...otification_at: datetime, project_proposals_open_at: datetime, project_proposals_close_at: datetime).transition? to return false, got true
   ./spec/models/season_spec.rb:208

Profile Page:

Edit page:

[klappradla]

Awesome job @lilwillifo ❤️💛💚💙💜 Thanx a ton for getting involved 🙏

I left some mostly cosmetic comments here and there - just tell me if something is weird ✌️

For the failing spec: I don't know from the top of my head what this could be. But a lot of the specs are time-critical 😉 So maybe it's something that is failing because the day the specs where ran is one that is not correctly covert by the seasons. Would be ace if you could find out what's going on there 💯

If not, don't worry, I can also look into it once I find time.

[emcoding]

The failing test has been solved by @klappradla himself :-) in #1039 👏 🙇
@lilwillifo That means that the tests should pass if you rebase your branch. Are you familiar with rebasing, if not, just ask and we can help you out.

[lilwillifo]

Hey @klappradla I made the requested changes. I rebased off of master, but am still getting a failing test around seasons on Travis CI. Not sure what's going on there. Thanks for your review on this!

[klappradla]

For the failing spec, just in case you didn't see it - the output is rather verbose at the moment until #1048 is merged 😹 - Travis also prints, the seed (45351 for your last build), so you can run

bundle exec rspec --seed 45351

To get the same errors locally.

Your particular failure comes down to this idea of Season.current, which is shooting us in the foot ever since 😹

You can fix the particular example by deliberately setting the other season to a time outside of the scope of the sequence in the default season factory:

RSpec.describe 'Editing Teams', type: :feature do
  let(:user)            { create(:organizer) }
  let(:season)          { Season.current }
  let(:other_season)    { create(:season, :past) }
  let(:team)            { create(:team, name: 'Team Yay', season: season) }
  let(:selection_phase) { season.applications_close_at + 1.day }
  let!(:exercism)       { create(:project, :accepted, name: 'Exercism', season: season) }

 # etc.

[klappradla]

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 😢

[klappradla]

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.

[klappradla]

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.

[klappradla]

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?

[klappradla]

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 😉