Skip to content

Tear down code for phone profile verification#2369

Merged
jmhooper merged 2 commits intomasterfrom
jmhooper-tear-down-phone-verification
Aug 13, 2018
Merged

Tear down code for phone profile verification#2369
jmhooper merged 2 commits intomasterfrom
jmhooper-tear-down-phone-verification

Conversation

@jmhooper
Copy link
Contributor

Why: The phone verification process for IdV used to mirror USPS
verification. We changed that functionality, but never properly cleaned
up some of the dead code. This commit does that, and also prepares to
drop the unnecessary phone_confirmed column from the profile model.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

  • For DB changes, check for missing indexes, check to see if the changes
    affect other apps (such as the dashboard), make sure the DB columns in the
    various environments are properly populated, coordinate with devops, plan
    migrations in separate steps.

  • For route changes, make sure GET requests don't change state or result in
    destructive behavior. GET requests should only result in information being
    read, not written.

  • For encryption changes, make sure it is compatible with data that was
    encrypted with the old code.

  • For secrets changes, make sure to update the S3 secrets bucket with the
    new configs in all environments.

  • Do not disable Rubocop or Reek offenses unless you are absolutely sure
    they are false positives. If you're not sure how to fix the offense, please
    ask a teammate.

  • When reading data, write tests for nil values, empty strings,
    and invalid formats.

  • When calling redirect_to in a controller, use _url, not _path.

  • When adding user data to the session, use the user_session helper
    instead of the session helper so the data does not persist beyond the user's
    session.

  • When adding a new controller that requires the user to be fully
    authenticated, make sure to add before_action :confirm_two_factor_authenticated.

@jmhooper
Copy link
Contributor Author

This commit tears out the unused code. I'd like to go back and move the profile verification logic into the IdV namespace and pull out some of the branching. That seemed like it would add too much bloat to this PR so I'm going to hold off for now.

@jmhooper jmhooper force-pushed the jmhooper-tear-down-phone-verification branch from 63ca174 to 31120e1 Compare July 23, 2018 21:21
**Why:** The phone verification process for IdV used to mirror USPS
verification. We changed that functionality, but never properly cleaned
up some of the dead code. This commit does that, and also prepares to
drop the unnecessary `phone_confirmed` column from the profile model.
@jmhooper
Copy link
Contributor Author

I'm confused about why code climate is saying coverage has decreased? It's telling me it's gone down in the VerifyProfilePhoneController, which is a file I deleted.

@jmhooper
Copy link
Contributor Author

The code coverage check here looks like it is failing since I deleted a whole bunch of covered code, reducing the total code coverage percentage. I don't think I introduced any uncovered paths.

@jgsmith-usds
Copy link
Contributor

It's expected for the ratio to decrease if we subtract the same number from the numerator and denominator (e.g., all lines of code removed were covered).

Copy link
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM. I love code removals.

@jmhooper jmhooper merged commit f400abe into master Aug 13, 2018
@jmhooper jmhooper deleted the jmhooper-tear-down-phone-verification branch February 15, 2019 19:14
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.

3 participants