[Bucks] Add DVLA registration lookup. #5808
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5808 +/- ##
==========================================
- Coverage 82.66% 82.65% -0.01%
==========================================
Files 459 461 +2
Lines 35850 35953 +103
Branches 5854 5855 +1
==========================================
+ Hits 29634 29716 +82
- Misses 4510 4531 +21
Partials 1706 1706 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chrismytton
left a comment
chrismytton
left a comment
There was a problem hiding this comment.
Looks good and works well! Just a few bits of feedback/questions.
| sub lookup : Path : Args(0) { | ||
| my ($self, $c) = @_; | ||
|
|
||
| my $reg = $c->req->body_params->{registration}; |
There was a problem hiding this comment.
Is it worth doing any kind of input validation here, e.g. to check its alphanumeric? Guess the DVLA API does its own validation as well.
| my $response = $ua->post( | ||
| $dvla->{uri} . '/v1/vehicles', | ||
| X_API_Key => $dvla->{key}, | ||
| Content_Type => 'application/json', | ||
| Content => encode_json($request), | ||
| ); | ||
|
|
||
| $c->res->content_type('application/json; charset=utf-8'); | ||
| $c->res->body($response->decoded_content); |
There was a problem hiding this comment.
I think this could do with some kind of error handling. Could either have it acting as a pure proxy, in which case we should be setting the status code on the response so that's passed through to the JS. Or alternatively we could add more complex error handling here on the server to handle 5xx and 4xx errors from the DVLA API and return a consistent "error" response to the JS.
| if (!reg) return; | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| $.post('/report/dvla', {'registration':reg}, function(data) { |
There was a problem hiding this comment.
Similarly here, could do with some error handling for when the DVLA controller returns an error status.
| var type = data.typeApproval || ''; | ||
| var wheelplan = data.wheelplan || ''; | ||
| var vehicle_type = ''; | ||
| if (type.match(/L[1-7]|motorcycle/i) || wheelplan.match(/motorcyle|moped|2 wheel/i)) { |
There was a problem hiding this comment.
Should motorcyle be motorcycle?
| if (!yesno) return; | ||
| yesno = yesno.value; | ||
| if (!yesno) { | ||
| field = document.querySelector('input[name*="' + fields.reg + '"]'); |
There was a problem hiding this comment.
Don't think field is declared anywhere?
| if (data.fuelType) vehicle_desc += ', ' + data.fuelType; | ||
| if (data.yearOfManufacture) vehicle_desc += ', ' + data.yearOfManufacture; | ||
| var reason = 'We cannot accept reports on vehicles that ' + reasons.join(' or '); | ||
| $msg = $('<div class="js-stopper-notice box-warning"><strong>' + vehicle_desc + '</strong><br>' + reason + '. You may be able to <a href="https://contact.dvla.gov.uk/report-untaxed-vehicle">contact the DVLA</a>.</div>'); |
There was a problem hiding this comment.
Don't think $msg is declared anywhere?
| <input class="govuk-radios__input" id="dvla_reg_have_yes" name="dvla_reg_have" type="radio" value="1" data-show="#dvla_reg_field" required> | ||
| <label class="govuk-label govuk-radios__label" for="dvla_reg_have_yes">Yes</label> | ||
| </div> | ||
| <div id="dvla_reg_field" class="hidden-js govuk-radios__conditional govuk-radios__conditional--hidden" id="conditional-contact"> |
There was a problem hiding this comment.
This <div> has two id attributes.
| <div class="floating-button-spacer"><div class="floating-button-wrapper"><div class="floating-button"> | ||
| <div class="pre-button-messaging"></div> | ||
| <button class="btn btn--block btn--primary js-reporting-page--next">Continue</button> | ||
| </div></div></div> |
There was a problem hiding this comment.
Do we want some kind of loading indicator around here somewhere, to show that we're waiting for a response from an external API?
| <div id="dvla_reg_field" class="hidden-js govuk-radios__conditional govuk-radios__conditional--hidden" id="conditional-contact"> | ||
| <div class="govuk-form-group"> | ||
| <label class="govuk-label" for="dvla_reg">Registration number</label> | ||
| <input class="govuk-input required" id="dvla_reg" name="dvla_reg" type="text" spellcheck="false"> |
There was a problem hiding this comment.
When I was testing this I pressed return instead of clicking "Continue" and it submitted the whole form. So think we need to e.preventDefault somewhere along the line.
[skip changelog]