Lg 7688 dont use Proofing::Base by gsa-manish · Pull Request #7349 · 18F/identity-idp

gsa-manish · 2022-11-16T13:46:12Z

🎫 Ticket

🛠 Summary of changes

Refactored removing Proofing:::Base

kbighorse

So great to get rid of that base class, well-done!
[issue] Can we revert uses of Proofing::LexisNexis::Ddp::Result and Proofing::LexisNexis::Ddp::Proofer::Config unless in the DDP context?

kbighorse · 2022-11-16T19:39:58Z

spec/jobs/resolution_proofing_job_spec.rb

Wasn't this better before?

I second this question. What was wrong with the ddp-mock-transaction-id-123 value?

After the refactoring it returns 1234 instead of Proofing::Mock::DdpMockClient::TRANSACTION_ID. Do we know if it should return Proofing::Mock::DdpMockClient::TRANSACTION_ID or some internal request ID that gets created?

I feel like ideally we would change the source of this data instead of changing this test to match. (I don't think it is worth holding up the PR however.)

kbighorse · 2022-11-16T19:41:03Z

spec/jobs/resolution_proofing_job_spec.rb

[suggestion] Can we DRY up these stub_request calls?

kbighorse · 2022-11-16T19:44:15Z

spec/services/proofing/lexis_nexis/ddp/result_spec.rb

[issue] Can we restore Proofing::ResultSpec?

kbighorse · 2022-11-16T19:51:10Z

app/services/proofing/mock/ddp_mock_client.rb

[suggestion] you might consider the more readable %i notation to generate an array of symbols here

kbighorse · 2022-11-16T19:51:31Z

app/services/proofing/mock/ddp_mock_client.rb

[suggestion] consider %i

kbighorse · 2022-11-16T19:52:02Z

app/services/proofing/mock/ddp_mock_client.rb

[suggestion] add an empty line before a return value

kbighorse · 2022-11-16T19:52:31Z

app/services/proofing/mock/resolution_mock_client.rb

[praise] so great these didn't need any more modification!

kbighorse · 2022-11-16T19:59:44Z

[issue] I'm noticing that lexisnexis_ddp_proofer is used (4x) in ResolutionProofingJobSpec but never defined 🤔. Should it be ddp_proofer instead?

kbighorse · 2022-11-17T16:59:49Z

Approved to unblock merging

changelog: Improvements, Refactoring, Remove proofing::base, Proofing::Result

jskinne3

I'm so glad to see app/services/proofing/base.rb removed!

jskinne3 · 2022-11-21T19:27:04Z

app/services/proofing/mock/ddp_mock_client.rb

-    class DdpMockClient < Proofing::Base
-      vendor_name 'DdpMock'
+    class DdpMockClient
+      class << self


Why did vendor_name, required_attributes, optional_attributes, and stage become class methods?

Since other places rely on DdpMockClient and they currently require them to be class methods, I did not want to mess up the way it accesses them as it is.

After looking over the old Proofing::Base I mostly understand this now. 👍

jskinne3 · 2022-11-21T19:29:15Z

spec/jobs/resolution_proofing_job_spec.rb

I second this question. What was wrong with the ddp-mock-transaction-id-123 value?

zachmargolis · 2022-11-21T20:44:18Z

app/services/proofing/result.rb

+    end
+      end


this indentation seems wrong. I think it was correct before

Suggested change

end

end

end

end

zachmargolis · 2022-11-21T20:45:43Z

spec/jobs/resolution_proofing_job_spec.rb

+              headers: {
+                'Accept' => '*/*',
+                'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
+                'Authorization' => 'Basic Og==',
+                'Content-Type' => 'application/json',
+                'User-Agent' => 'Faraday v2.6.0',
+              },


I think we can do a hash_including here too, user-agent and accept-encoding change often so I'd remove them

Suggested change

headers: {

'Accept' => '*/*',

'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',

'Authorization' => 'Basic Og==',

'Content-Type' => 'application/json',

'User-Agent' => 'Faraday v2.6.0',

},

headers: hash_including(

'Authorization' => 'Basic Og==',

'Content-Type' => 'application/json',

),

jskinne3

Approving because none of my questions are worth holding up this badly-needed PR

This reverts commit c0dc267.

gsa-manish force-pushed the LG-7688-dont-use-proofing-base branch from 0714fd2 to 775b781 Compare November 16, 2022 14:30

gsa-manish changed the title ~~WIP: Lg 7688 dont use proofing base~~ Lg 7688 dont use proofing base Nov 16, 2022

gsa-manish requested review from jmhooper, jskinne3 and kbighorse November 16, 2022 15:14

gsa-manish changed the title ~~Lg 7688 dont use proofing base~~ Lg 7688 dont use Proofing::Base/Proofing::Result Nov 16, 2022

kbighorse suggested changes Nov 16, 2022

View reviewed changes

kbighorse reviewed Nov 16, 2022

View reviewed changes

app/services/proofing/mock/ddp_mock_client.rb Outdated

Copy link

Contributor

kbighorse Nov 16, 2022

Choose a reason for hiding this comment

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

[suggestion] consider %i

kbighorse reviewed Nov 16, 2022

View reviewed changes

app/services/proofing/mock/ddp_mock_client.rb Outdated

Copy link

Contributor

kbighorse Nov 16, 2022

Choose a reason for hiding this comment

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

[suggestion] add an empty line before a return value

kbighorse reviewed Nov 16, 2022

View reviewed changes

app/services/proofing/mock/resolution_mock_client.rb Outdated

Copy link

Contributor

kbighorse Nov 16, 2022

Choose a reason for hiding this comment

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

[praise] so great these didn't need any more modification!

kbighorse approved these changes Nov 17, 2022

View reviewed changes

kbighorse added 2 commits November 21, 2022 11:52

Test ddp proofer in resolution proofing job spec

440914e

Stop inheriting from LexisNexis::Proofer

0d4440b

gsa-manish force-pushed the LG-7688-dont-use-proofing-base branch 2 times, most recently from 209a25c to 36d2dbc Compare November 21, 2022 17:45

LG-7688-dont-use-proofing-base

cf5b4f2

changelog: Improvements, Refactoring, Remove proofing::base, Proofing::Result

gsa-manish force-pushed the LG-7688-dont-use-proofing-base branch from 36d2dbc to cf5b4f2 Compare November 21, 2022 18:19

gsa-manish changed the title ~~Lg 7688 dont use Proofing::Base/Proofing::Result~~ Lg 7688 dont use Proofing::Base Nov 21, 2022

jskinne3 suggested changes Nov 21, 2022

View reviewed changes

zachmargolis reviewed Nov 21, 2022

View reviewed changes

jskinne3 self-requested a review November 21, 2022 22:33

jskinne3 approved these changes Nov 21, 2022

View reviewed changes

gsa-manish merged commit c0dc267 into main Nov 22, 2022

gsa-manish deleted the LG-7688-dont-use-proofing-base branch November 22, 2022 15:40

zachmargolis mentioned this pull request Nov 22, 2022

Fix alignment in Proofing::Result #7383

Merged

mdiarra3 mentioned this pull request Nov 23, 2022

RC 230 #7388

Merged

mdiarra3 added a commit that referenced this pull request Nov 23, 2022

Revert "Lg 7688 dont use Proofing::Base (#7349)"

8a87bbe

This reverts commit c0dc267.

mdiarra3 mentioned this pull request Nov 28, 2022

Deploy RC 231 to Stages/prod #7395

Merged

This was referenced Nov 29, 2022

Deploy RC 232 to Prod #7404

Closed

Deploy RC 232 to production #7414

Merged

-    end
-      end
+  end
+end

Conversation

gsa-manish commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

kbighorse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbighorse commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbighorse commented Nov 17, 2022

Uh oh!

jskinne3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskinne3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gsa-manish commented Nov 16, 2022 •

edited

Loading

kbighorse commented Nov 16, 2022 •

edited

Loading