Feature/cb2 10593 - update parseAdrDetails function #78
Conversation
… adding in-line comments
meys-bjss
requested review from
reisedawson,
simontindallbjss,
Nat-McCabe-BJSS and
DanielFry-bjss
meys-bjss
changed the title
tests/resources/Dockerfile
Outdated
| @@ -1,5 +1,5 @@ | |||
| # Match Aurora MySQL version as closely as possible | |||
| FROM mysql:5.7.12 | |||
| FROM mysql:8.2.0 | |||
There was a problem hiding this comment.
Are we OK to change this? I was just wondering if it's best to leave it as is and you just change the version locally
meys-bjss
dismissed stale reviews from Nat-McCabe-BJSS and shivangidas
via
7ac372f
| weight?: string; | ||
| compatibilityGroupJ?: boolean; | ||
| weight?: number; | ||
| newCertificateRequested?: boolean; |
There was a problem hiding this comment.
This is not required as it is a front end field for UX
There was a problem hiding this comment.
Weight is listed as a number in ADR section, so I would presume that is required, it's how I've done it in Pydantic. CompatibilityGroupJ should still be listed though no? NewCertificateRequested is also in the ADR section documentation that should also be needed
There was a problem hiding this comment.
@simontindallbjss, newCertificateRequested is removed from the final upsert function for now, so it wouldn't end up in NOP. we can always add it whenever needed. Note that all the attributes are defined as 'optional', so fine if they don't exist in the payload.
| documents?: string[]; | ||
| permittedDangerousGoods?: string[]; | ||
| additionalExaminerNotes?: string; | ||
| permittedDangerousGoods?: permittedDangerousGoodsEnum[]; |
There was a problem hiding this comment.
ADR documentation states that this shouldn't be an enum in the backend due to the historic data containing erroneous values
There was a problem hiding this comment.
As previously discussed with @reisedawson-bjss, at this stage, we are going to keep them as Enums. After the first round of remediation (hopefully in pre-prod), if this creates a massive issue, we can always turn them into
general "string" type.
| applicantDetails?: ApplicantDetails; | ||
| memosApply?: string[]; | ||
| dangerousGoods?: boolean; | ||
| memosApply?: memosApplyEnum[]; |
There was a problem hiding this comment.
ADR documentation states that this shouldn't be an enum in the backend due to the historic data containing erroneous values
There was a problem hiding this comment.
Same as permittedDangerousGoodsEnum case
Nat-McCabe-BJSS
dismissed
their stale review
CompatibilityGroupJ query and enum typings need fixing
| export interface VehicleDetails { | ||
| type?: string; | ||
| type?: VehicleDetailsTypeEnum; |
There was a problem hiding this comment.
Should be a string in the backend
There was a problem hiding this comment.
Same as permittedDangerousGoodsEnum case
| @@ -36,8 +114,8 @@ export interface ApplicantDetails { | |||
| } | |||
|
|
|||
| export interface AdditionalNotes { | |||
| number?: string[]; | |||
| guidanceNotes?: string[]; | |||
| number?: additionalNotesNumberEnum[]; | |||
There was a problem hiding this comment.
Should be array of strings in backend
| export interface VehicleDetails { | ||
| type?: string; | ||
| type?: VehicleDetailsTypeEnum; |
There was a problem hiding this comment.
ADR documentation states that this shouldn't be an enum in the backend due to the historic data containing erroneous values
There was a problem hiding this comment.
Same as permittedDangerousGoodsEnum case
| @@ -36,8 +114,8 @@ export interface ApplicantDetails { | |||
| } | |||
|
|
|||
| export interface AdditionalNotes { | |||
| number?: string[]; | |||
| guidanceNotes?: string[]; | |||
| number?: additionalNotesNumberEnum[]; | |||
There was a problem hiding this comment.
ADR documentation states that this shouldn't be an enum in the backend due to the historic data containing erroneous values
There was a problem hiding this comment.
Same as permittedDangerousGoodsEnum case
| applicantDetails?: ApplicantDetails; | ||
| memosApply?: string[]; | ||
| dangerousGoods?: boolean; |
There was a problem hiding this comment.
This is not required as it is a front end field for UX
There was a problem hiding this comment.
Same as newCertificateRequested case
Description
Updating the update-store's parseAdrDetails functionality so that it parses the ADR Details attribute in a given tech record if the adrDetails attribute exists.
Related issue:
CB2-10593
CB2-10592
Useful links:
https://dvsa.atlassian.net/wiki/spaces/HVT/pages/382011842/ADR+Section
https://dvsa.atlassian.net/wiki/spaces/HVT/pages/494174610/SPIKE+Validate+existing+update-store+functionality+for+ADR
Before submitting (or marking as "ready for review")