-
Notifications
You must be signed in to change notification settings - Fork 3
feat(domain): auto delete notifications #400
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(domain): auto delete notifications #400
Conversation
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.
Almost there! Just a few things. Also, the notification getById
feature doesn't include the deleted
flag yet.
{ | ||
const notifications = await getNotifications(postId); | ||
|
||
return notifications.map(item => deleteData(item.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.
return notifications.map(item => deleteData(item.id)); | |
const promises = notifications.map(item => deleteData(item.id)); | |
await Promise.allSettled(promises); |
Return type: Promise<void>
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 file contains the actual feature, and therefore should be named remove.ts
.
@@ -0,0 +1,2 @@ | |||
|
|||
export { default } from './deleteData'; |
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.
export { default } from './deleteData'; | |
export { default } from './remove'; |
|
||
export default async function remove(id: string): Promise<void> | ||
{ | ||
return database.updateRecord(RECORD_TYPE, id, { deleted: true }) as Promise<void>; |
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.
return database.updateRecord(RECORD_TYPE, id, { deleted: true }) as Promise<void>; | |
return database.updateRecord(RECORD_TYPE, id, { deleted: true }); |
|
||
import database from '^/integrations/database'; | ||
|
||
import type { RecordQuery } from '^/integrations/database'; |
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 import should be grouped with the other ^/integration/...
import.
|
||
import database from '^/integrations/database'; | ||
|
||
import type { RecordQuery } from '^/integrations/database'; |
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 import should be grouped with the other ^/integration/...
import.
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 file tests the remove
feature of the notification. The name of the file should reflect this.
|
||
expect(result).toHaveLength(0); | ||
}); | ||
|
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 empty line is not required
|
||
import getByPostId from '^/domain/notification/getByPostId'; | ||
import removedPost from '^/domain/notification/notify/removedPost'; | ||
import { DATABASES, VALUES } from './fixtures'; |
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 should be an empty line before this import. Those above are coming from the domain
, and this one from ./
|
Fixes #342
Changes proposed in this pull request:
@MaskingTechnology/comify