Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of vendor validator abstraction #1803

Merged
merged 1 commit into from
Nov 30, 2017
Merged

Conversation

jmhooper
Copy link
Member

@jmhooper jmhooper commented Nov 24, 2017

Why: The vendor validator made it difficult to clearly see where and how the logic that communicates with the proofer gem was being used. It will also be less helpful in a world where we have multiple vendors per proofing step. This commit tears the vendor validator down in favor of a unique job class for each step. That job class then invokes the proofer gem code itself.

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.

  • 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.

.reek Outdated
- Idv::VendorResult#initialize
- Idv::ProfileJob#perform
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not check the reek box because I disabled reek here.

The easiest way to fix this in the code is to use a params hash instead of keyword arguments. I'm not a fan of that since it is much harder to know what args the method actually takes.

The other thing I thought of was to create a struct (something like Idv::JobArguments) and put all of the args on that, but that seems like overkill and unnecessarily complicated so I just disabled the reek check.

@jmhooper
Copy link
Member Author

This code is a little bit less DRY than what was there before, but that shouldn't be the case once we have more complicated and specific logic for each step. Also, it removes a layer of abstraction from between where the data is submitted in the form and where it is sent to the proofer gem which I think makes it easier to read and reason about.

@jmhooper
Copy link
Member Author

I paired with Moncef to clean up the reek issues. This should be ready to go.

Copy link

@mugizico mugizico left a comment

Choose a reason for hiding this comment

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

I pair with Jonathan on a video code review/code walkthrough. One action item we came up with is the need to provide architectural documentation on things like idv and how the communication between the various proofing vendors and login.gov works on a high-level. This will especially be helpful to new engineers joining the team when refactoring or add new features to idp. I'll create an issue for it.

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.

Looks great, thanks! I tested locally with the mock vendor, including with the "Fail" first name that raises an exception, and I successfully saw the error message and backtrace in the Sidekiq log and in the Exception Notification email.

**Why**: The vendor validator made it difficult to clearly see where and
how the logic that commuicates with the proofer gem was. It will also be
less helpful in a world where we have multiple vendors per proofing
step. This commit tears the vendor validator down in favor of a unique
job class for each step. That job class then invokes the proofer gem
code itself.
@jmhooper jmhooper force-pushed the jmhooper-idv-job-refactor branch from 48127d2 to 52cb383 Compare November 30, 2017 16:34
@jmhooper jmhooper merged commit 61c94d4 into master Nov 30, 2017
@jmhooper jmhooper deleted the jmhooper-idv-job-refactor branch December 12, 2017 20:13
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