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

Refactor the import legacy persist logic #1387

Merged
merged 25 commits into from
Oct 11, 2024

Conversation

jonathangoulding
Copy link
Collaborator

We have recently introduced functionality to import data from NALD using for a licence and companies for that particular licence.

This change will break out the persist functionality into logical groups of functionality.

We have recently introduced functionality to import data from NALD using for a licence and companies for that particular licence.

This change will break out the persist functionality into logical groups of functionality.
@jonathangoulding jonathangoulding added the housekeeping Refactoring, tidying up or other work which supports the project label Oct 7, 2024
@jonathangoulding jonathangoulding self-assigned this Oct 7, 2024
@jonathangoulding jonathangoulding marked this pull request as ready for review October 7, 2024 10:19
@jonathangoulding
Copy link
Collaborator Author

I think it's worth having the big test to test the complete transaction.

Do we want to add duplicate tests per persist service ?

Worth a discussion.

@Cruikshanks
Copy link
Member

I think it's worth having the big test to test the complete transaction.

Do we want to add duplicate tests per persist service ?

Worth a discussion.

Looks like we need a discussion! For me, the main driver for this was just how big and complex the tests were becoming, which is a 'smell' that the service under test is doing too much.

So, I would expect to see tests specifically for the things we have broken out, and persist-import.service.test.js to feature a lot of stubbing of those child services.

The child services can then focus on ensuring they behave as expected when inserting or updating. In PersistImportService we could, for example, start looking at how it behaves when one of those child services errors, i.e. confirm that nothing gets persisted. 🤷

app/services/import/persist-import.service.js Outdated Show resolved Hide resolved
app/services/import/legacy/process-licence.service.js Outdated Show resolved Hide resolved
app/services/import/persist/persist-company.service.js Outdated Show resolved Hide resolved
app/services/import/persist/persist-company.service.js Outdated Show resolved Hide resolved
app/services/import/persist/persist-company.service.js Outdated Show resolved Hide resolved
app/services/import/persist/persist-company.service.js Outdated Show resolved Hide resolved
app/services/import/persist/persist-licence.service.js Outdated Show resolved Hide resolved
test/services/import/persist-licence.service.test.js Outdated Show resolved Hide resolved
test/services/import/persist-licence.service.test.js Outdated Show resolved Hide resolved
@jonathangoulding jonathangoulding force-pushed the refactor-import-legacy-persist-logic branch from d82efea to cd512c5 Compare October 7, 2024 15:00
@jonathangoulding jonathangoulding marked this pull request as draft October 7, 2024 15:00
@jonathangoulding jonathangoulding marked this pull request as ready for review October 8, 2024 09:30
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.

Just gonna ping these back and then will check in again later, as they'll also help make the changes a bit less confusing for me!

app/services/import/legacy/process-licence.service.js Outdated Show resolved Hide resolved
app/services/import/persist-import.service.js Outdated Show resolved Hide resolved
@jonathangoulding jonathangoulding merged commit 57f4e3f into main Oct 11, 2024
5 of 6 checks passed
@jonathangoulding jonathangoulding deleted the refactor-import-legacy-persist-logic branch October 11, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants