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

Add wlrs licence id to the nald licence import #1319

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

jonathangoulding
Copy link
Collaborator

https://eaflood.atlassian.net/browse/WATER-4545

Relevant changes - #1294

As part of the ongoing work to migrate the legacy import jobs we need to establish if a licence already exists in the wrls database.

We check if the licence already exists, if it does then we want to do nothing. If it does not exist we want to generate return logs.

This change is added to highlight where we need to add this additional logic. And the service call will be added in another piece of work.

https://eaflood.atlassian.net/browse/WATER-4545

Relevant changes - #1294

As part of the ongoing work to migrate the legacy import jobs we need to establish if a licence already exists in the wrls database.

We check if the licence already exists, if it does then we want to do nothing. If it does not exist we want to generate return logs.

This change is added to highlight where we need to add this additional logic. And the service call will be added in another piece of work.
@jonathangoulding jonathangoulding self-assigned this Sep 10, 2024
@jonathangoulding jonathangoulding added the enhancement New feature or request label Sep 10, 2024
@jonathangoulding jonathangoulding marked this pull request as ready for review September 10, 2024 10:33
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Unless you know of any issue, we should be able to call the property licenceId because that is what it is on the transformed licence object (we can drop the wrls prefix).

And I think you'll need to drop it in the same way we drop licenceVersions in the persist service. Try updating test/services/import/persist-licence.service.test.js to include the new property: if it blows up you know you do!

On that point, we should also be updating the validator to define that we expect this parameter but null is acceptable.

@jonathangoulding
Copy link
Collaborator Author

jonathangoulding commented Sep 10, 2024

Unless you know of any issue, we should be able to call the property licenceId because that is what it is on the transformed licence object (we can drop the wrls prefix).

And I think you'll need to drop it in the same way we drop licenceVersions in the persist service. Try updating test/services/import/persist-licence.service.test.js to include the new property: if it blows up you know you do!

On that point, we should also be updating the validator to define that we expect this parameter but null is acceptable.

This is not used in the transformedLicence so there is no need to validate and exclude.

This is treated like the naldLicenceId and regionCode

Also calling it licenceId conflcts when descrutuing with code further down.

const licenceId = await PersistLicenceService.go(transformedLicence, transformedCompanies)

@Cruikshanks
Copy link
Member

Unless you know of any issue, we should be able to call the property licenceId because that is what it is on the transformed licence object (we can drop the wrls prefix).
And I think you'll need to drop it in the same way we drop licenceVersions in the persist service. Try updating test/services/import/persist-licence.service.test.js to include the new property: if it blows up you know you do!
On that point, we should also be updating the validator to define that we expect this parameter but null is acceptable.

This is not used in the transformedLicence so there is no need to validate and exclude.

This is treated like the naldLicenceId and regionCode

Also calling it licenceId conflcts when descrutuing with code further down.

const licenceId = await PersistLicenceService.go(transformedLicence, transformedCompanies)

I still think we should be including it in the validator. The validator serves as one place where we define the object we expect to be passed to the persist service. And certainly the test for the validator should be updated to include the property. In fact, I'd be surprised if the test passes when you do because licenceId is not in the schema, and we haven't specified .unknown(true).

On the final point, we only need the ID for the log message. It can simply be renamed as const persistedLicenceId. Alternatively, you could not set it using the destructure, but do it with a let licenceId = transformedLicence.licenceId instead 🤷

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Well, I got completely the wrong end of the stick!

All good, carry on!

@jonathangoulding jonathangoulding merged commit 1fceb2d into main Sep 10, 2024
6 checks passed
@jonathangoulding jonathangoulding deleted the add-wrls-licence-id-to-licence-import branch September 10, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants