Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

LG-4327 First pass image metric errors for DPI, Blur, and Glare#14

Merged
amathews-fs merged 5 commits intomainfrom
LG-4327-quick-image-metrics-errors
Apr 7, 2021
Merged

LG-4327 First pass image metric errors for DPI, Blur, and Glare#14
amathews-fs merged 5 commits intomainfrom
LG-4327-quick-image-metrics-errors

Conversation

@amathews-fs
Copy link
Contributor

@amathews-fs amathews-fs commented Apr 5, 2021

Adding the first pass at doc auth errors for problems with the image metrics Acuant returns. There will be a significant refactor for the error handling coming next.

Associated with this PR in Identity-idp

…e image metrics Acuant returns. There will be a significant refactor for the error handling coming next.
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

This is great! A couple comments, but not deal breakers.

attr_reader :config

BARCODE_COULD_NOT_BE_READ_ERROR = 'The 2D barcode could not be read'.freeze
DPI_THRESHOLD = 290
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these fixed values? or might we need to change them at some point. If the latter, might be worth adding a configuration class so the gem can be configured from the idp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah, I thought about it and then didn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some consideration, I will do that during the larger refactor coming next.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have the config object already! Just need to add the properties there, it's referenced in this file a line above already https://github.com/18F/identity-doc-auth/blob/main/lib/identity_doc_auth/acuant/config.rb


context 'when front image HDPI is too low' do
let(:http_response) do
parsed_response_body = JSON.parse(AcuantFixtures.get_results_response_failure)
Copy link
Contributor

Choose a reason for hiding this comment

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

The fixture is parsed in each of these contexts. You could wrap these in a context and pull that out, e.g.,

    context 'image metrics errors' do
      let(:parsed_response_body) { JSON.parse(AcuantFixtures.get_results_response_failure) }

      context 'when front image HDPI is too low' do
        let(:http_response) do
          parsed_response_body['Images'].first['HorizontalResolution'] = 250
          instance_double(
            Faraday::Response,
            body: parsed_response_body.to_json,
          )
        end
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. Don't need to parse each time.

Comment on lines +75 to +97
processed_image_metrics.each do |side, i|
hdpi = i['HorizontalResolution'] || 0
vdpi = i['VerticalResolution'] || 0
if hdpi < DPI_THRESHOLD || vdpi < DPI_THRESHOLD
front_dpi_fail = true if side == :front
back_dpi_fail = true if side == :back
end

sharpness = i['SharpnessMetric']
if sharpness.present? && sharpness < SHARP_THRESHOLD
front_sharp_fail = true if side == :front
back_sharp_fail = true if side == :back
end

glare = i['GlareMetric']
if glare.present? && glare < GLARE_THRESHOLD
front_glare_fail = true if side == :front
back_glare_fail = true if side == :back
end
end

return { results: [Errors::DPI_LOW_BOTH_SIDES] } if front_dpi_fail && back_dpi_fail
return { results: [Errors::DPI_LOW_ONE_SIDE] } if front_dpi_fail || back_dpi_fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason that we have separate "one side failed" vs "both sides failed" errors? I think if we had an array of errors that we added to, or even an ActiveModel::Errors that we add per attribute to, we could get some more useful data?

Suggested change
processed_image_metrics.each do |side, i|
hdpi = i['HorizontalResolution'] || 0
vdpi = i['VerticalResolution'] || 0
if hdpi < DPI_THRESHOLD || vdpi < DPI_THRESHOLD
front_dpi_fail = true if side == :front
back_dpi_fail = true if side == :back
end
sharpness = i['SharpnessMetric']
if sharpness.present? && sharpness < SHARP_THRESHOLD
front_sharp_fail = true if side == :front
back_sharp_fail = true if side == :back
end
glare = i['GlareMetric']
if glare.present? && glare < GLARE_THRESHOLD
front_glare_fail = true if side == :front
back_glare_fail = true if side == :back
end
end
return { results: [Errors::DPI_LOW_BOTH_SIDES] } if front_dpi_fail && back_dpi_fail
return { results: [Errors::DPI_LOW_ONE_SIDE] } if front_dpi_fail || back_dpi_fail
errors = Hash.new { |h, k| h[k] = [] }
processed_image_metrics.each do |side, i|
hdpi = i['HorizontalResolution'] || 0
vdpi = i['VerticalResolution'] || 0
if hdpi < DPI_THRESHOLD || vdpi < DPI_THRESHOLD
errors[side] << :dpi_fail if side == :front
end
sharpness = i['SharpnessMetric']
if sharpness.present? && sharpness < SHARP_THRESHOLD
errors[side] << :sharp_fail
end
glare = i['GlareMetric']
if glare.present? && glare < GLARE_THRESHOLD
errors[side] << :glare_fail
end
end
return errors.to_h # or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a combination of things. The first was that we had slightly different error messages for single errors vs multiple errors, which is in the corresponding IDP PR linked above. The second piece of this was that as part of this I realized that we just needed to refactor the way that we're generating errors for both TrueID and Acuant. My goal for that will be to redesign and simplify the separation between the error generation in the gem and the translation/display in the IDP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A piece I just realized I didn't address: In this case a DPI failure can cause problems with the sharpness and glare metrics. So we aren't delivering multiple errors for that right now. That is also something that will be considered during the refactor.

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.

Let's add the config changes in this PR, and then LGTM

@@ -0,0 +1,62 @@
module IdentityDocAuth
module Errors
# ALerts
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# ALerts
# Alerts

nit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, oops.

@amathews-fs amathews-fs merged commit 3463710 into main Apr 7, 2021
@amathews-fs amathews-fs deleted the LG-4327-quick-image-metrics-errors branch April 7, 2021 15:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants