-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: rollback publish changes #431
Conversation
3bbe665
to
4bf2792
Compare
src/Collection/Collection.service.ts
Outdated
SlotUsageCheque.delete({ created_at: now }), | ||
ItemCuration.delete({ created_at: now }), |
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.
There are three issues that I see here:
- Although it could be rare, there's no guarantee that there will be only one publication in the same millisecond, as there are operations that could be done in nanoseconds, hitting the same millisecond, possibly deleting things that weren't supposed to be deleted.
- Item curations can be created at any time. Following the same idea as the other point, we could be deleting an item curation that was created in the same millisecond period.
- Having a new instance of the builder will increase the possibility of hitting the same millisecond.
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.
I thought of all of those (except the last one regretfully) and was expecting some feedback lol. The thing is that deleting itemCurations is a bit of a pain, but I'll try saving the ids and using them here. It'll make the code more complex but I don't see any way around it. Let me know if you do!
4bf2792
to
f47b8e6
Compare
Pull Request Test Coverage Report for Build 1988953138
💛 - Coveralls |
) | ||
jest.spyOn(ethers.utils, 'verifyMessage').mockReturnValue('0x') | ||
|
||
jest.spyOn(SlotUsageCheque, 'create').mockResolvedValueOnce({}) |
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 adding here an id so we can later assert that the delete procedure was called with this id?
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 same goes for the delete
method of the SlotUsageCheque
.
let itemCurationDeleteSpy: jest.SpyInstance< | ||
ReturnType<typeof ItemCuration.delete> | ||
> |
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.
With the change, instead of ItemCuration.delete
, ItemCuration.deleteByIds
is being used.
3c0868e
to
0d93240
Compare
Closes #422