-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: Update SW video field #696
Conversation
Pull Request Test Coverage Report for Build 5942496375
💛 - Coveralls |
src/Curation/Curation.router.ts
Outdated
*/ | ||
private updateCollectionItemsContent = async (collectionId: string) => { | ||
const itemService = new ItemService() | ||
itemService.updateDCLItemsContent(collectionId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the await
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right! added the await
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look great 👍
I've left some comments on the tests that I think we could improve.
src/Curation/Curation.router.spec.ts
Outdated
updateSpy = jest | ||
.spyOn(service, 'updateById') | ||
.mockResolvedValueOnce(expectedCuration) | ||
const updateItemSpy = jest | ||
.spyOn(Item, 'upsert') | ||
.mockResolvedValue({ | ||
...mockItem, | ||
video: newVideoHash, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind setting these two in the beforeEach
?
The same goes for the other test.
src/Item/Item.service.spec.ts
Outdated
}, | ||
video: 'videoHash', | ||
} | ||
;(Item.findOne as jest.Mock).mockRestore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you need to use restores because there was an issue with the mocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required the restores, removing it
src/Item/Item.service.spec.ts
Outdated
;(collectionAPI.fetchCollection as jest.Mock).mockImplementation(() => | ||
Promise.resolve({ | ||
...collectionFragmentMock, | ||
creator: oldCreatorAddress, | ||
}) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mocks that return values can be simplified using the resolve
or return
mocks. Would you mind changing them?
;(collectionAPI.fetchCollection as jest.Mock).mockImplementation(() => | |
Promise.resolve({ | |
...collectionFragmentMock, | |
creator: oldCreatorAddress, | |
}) | |
) | |
;(collectionAPI.fetchCollection as jest.Mock).mockResolvedValueOnce({ | |
...collectionFragmentMock, | |
creator: oldCreatorAddress, | |
}) |
src/Item/Item.service.spec.ts
Outdated
;(Item.findOne as jest.Mock).mockRestore() | ||
;(Item.findOne as jest.Mock).mockResolvedValueOnce(dbItem) | ||
;(Item.hasPublishedItems as jest.Mock).mockResolvedValue(true) | ||
;(Item.upsert as jest.Mock).mockImplementation((value) => value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using the once
version of the mocks? This will prevent the mocks from leaking into other tests.
60e5b04
to
9b0baa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
This PR updates the video field for smart wearables when approving a curation.