Skip to content

Add controller/view/xlations/specs for recovery flow reboot#1470

Merged
el-mapache merged 1 commit intomasterfrom
ab-update-account-reactivation
Jun 2, 2017
Merged

Add controller/view/xlations/specs for recovery flow reboot#1470
el-mapache merged 1 commit intomasterfrom
ab-update-account-reactivation

Conversation

@el-mapache
Copy link
Contributor

@el-mapache el-mapache commented Jun 2, 2017

Why: We are changing around our accout recovery flow to better match
the existing experience

Linked issue is pretty large, this is a subset of the changes being made to the account recovery flow. Currently, this only adds the new reactivate_account_controller, routes, specs, and view. The user will still be redirected to the existing reactivate_account screen at /account/reactivate

Screenshot:

screen shot 2017-06-02 at 2 11 09 pm

Designs are still in progress and thus don't match the comps exactly. Updates will be made in a future PR as soon as these are finalized.

@el-mapache el-mapache self-assigned this Jun 2, 2017
@el-mapache el-mapache changed the title Add controller/view/xlations/specs for recovery Add controller/view/xlations/specs for recovery flow reboot Jun 2, 2017
config/routes.rb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. should this get a leading slash for consistency?
  2. to keep these alphabetized by path, should we move these new ones down 2 lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I thought about moving them down, but it felt weird since logically startis going to happen first. Our convention is more about alphabetizing, I'll change it

Copy link
Contributor

Choose a reason for hiding this comment

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

seems weird to me to split across two lines like this, should we use the > syntax? ditto for the key below

explanation: >
  When you created your account ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add the > signs, but both of the strings still need a line break to fit within the 100 character limit, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, definitely, but the > tells it to ignore newlines inside the strings 😁

Copy link
Contributor Author

@el-mapache el-mapache Jun 2, 2017

Choose a reason for hiding this comment

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

ok! Do I still need the quotes to escape the : at the end of the string? Or is that going to reintroduce a newline character. Is there a better way to escape?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it's indented consistently the : should be fine

explanation: >
  blah blah blah: blah
  more text goes here indented the same level
  blah: this is fine too

Copy link
Contributor

Choose a reason for hiding this comment

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

GH's highlighting got that wrong, but I confirmed with the Ruby YAML parser:

irb(main):002:0> YAML.load <<-STR
irb(main):003:0" explanation: >
irb(main):004:0"   blah blah blah: blah
irb(main):005:0"   more text goes here indented the same level
irb(main):006:0"   blah: should be fine
irb(main):007:0" STR
=> {"explanation"=>"blah blah blah: blah more text goes here indented the same level blah: should be fine\n"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, my syntax highlighter was freaking out, so I didn't try it out 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

code climate is highlighting this as untested, can we add a scenario for visiting without the correct profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think the cleanest way to test this is? Stub out a user and the password_reset_profile method?

Copy link
Contributor

Choose a reason for hiding this comment

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

have a spec that signs in to the controller with a user that has no profiles?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so we should definitely try to use as much "real" code as possible and minimize stubbing of specific filters and methods like this.

I would remove these 3 stubs and do:

let(:user) { build(:user, profiles: profiles) }
let(:profiles) { [] }
before { stub_sign_in(user) }

And ditto for the spec below.

Then we can override the let(:profiles) with an array of profile objects to match the various scenarios we want to test. (it might have to be a create(:user ... to make sure they get written to the DB, but same idea

@el-mapache
Copy link
Contributor Author

el-mapache commented Jun 2, 2017

@zachmargolis updated, with a 0.1% increase in code coverage 😸

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome, and thanks for adding the spec! 🚀

@el-mapache
Copy link
Contributor Author

Of course, thanks for the spec writing tips!

@el-mapache el-mapache force-pushed the ab-update-account-reactivation branch from e35415e to 232eda5 Compare June 2, 2017 19:54
**Why**: We are changing around our accout recovery flow to better match
the existing experience
@el-mapache el-mapache merged commit 704ef95 into master Jun 2, 2017
@el-mapache el-mapache deleted the ab-update-account-reactivation branch June 2, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants