-
Notifications
You must be signed in to change notification settings - Fork 6
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
Migrate set thread status from database layer #1435
Migrate set thread status from database layer #1435
Conversation
df44ac7
to
b941ce9
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.
Please check the code manually, I tested it in my machine and it didn't work. If iI made a mistake and it works on yours, could you please add a checks in the automatic tests that the status was really changed?
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.
Can be merged and after #1441 it can be refactored => Learning: before migrating mutation we should always update the queries first.
thread: { setThreadStatus: { success: true } }, | ||
}) | ||
given('UuidQuery').for({ ...comment, id: 35163, status: 'done' }) |
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 is needed as long as #1441 is not merged => The reason is that in resolving threads the old uuid resolver is used which does not read from DB
await context.dataSources.model.serlo.getUuid._querySpec.removeCache({ | ||
payloads: ids.map((id) => ({ 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.
This is needed as long as #1441 is not merged.
…ntries instead of removeCacheEntry
Added a test after the comment. The test revealed four bugs, including three from #1351, that are now fixed.
No description provided.