chore(api-service,dashboard): Replace create_at sorting with _id for subscribers #7603
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
desiprisg
force-pushed
the
replace_created_at_with_id_subscribers
branch
from
68ed58d to
4bd7905
Compare
desiprisg
force-pushed
the
replace_created_at_with_id_subscribers
branch
from
4bd7905 to
0f710c0
Compare
@novu/client
@novu/headless
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/shared
commit: |
| @@ -28,7 +28,7 @@ export class SubscriberController { | |||
| after: query.after, | |||
| before: query.before, | |||
| orderDirection: query.orderDirection || DirectionEnum.DESC, | |||
| orderBy: query.orderBy || 'createdAt', | |||
| orderBy: query.orderBy || '_id', | |||
There was a problem hiding this comment.
sorting by id is an internal implementation detail.
we need to map createdAt to id internally.
question: what will happen if the client will send orderBy = createdAt?
There was a problem hiding this comment.
IMO since it has its drawbacks we should keep it as _id. It's not correct to expose it as createdAt.
| subscriberSchema.index({ | ||
| _environmentId: 1, | ||
| _organizationId: 1, | ||
| createdAt: 1, | ||
| _id: 1, | ||
| }); |
There was a problem hiding this comment.
if we sort by _id we will still need index for it.
something like
subscriberSchema.index({
_environmentId: 1,
_organizationId: 1,
_id: 1,
});
id suggest to check if we need
subscriberSchema.index({
_environmentId: 1,
_organizationId: 1,
deleted: 1,
});
if we do not we could remove it and add the suggested above instead.
| @@ -10,7 +10,7 @@ export interface IListSubscribersRequestDto { | |||
|
|
|||
| orderDirection: DirectionEnum; | |||
|
|
|||
| orderBy: 'updatedAt' | 'createdAt'; | |||
| orderBy: 'updatedAt' | '_id'; | |||
There was a problem hiding this comment.
as mentioned above IMO we should encapsulate internal implementation on the API interface, the client should sort createdAt and we will map it to sort by _id
There was a problem hiding this comment.
now that i am looking at the index we have, I don't know how much we benefit by sorting by _id.
why do we need the _id at the index at all? as long as we do not "range" by it. it is not needed.
subscriberSchema.index({
_environmentId: 1,
_organizationId: 1,
createdAt: -1,
});
cc @desiprisg
desiprisg
force-pushed
the
replace_created_at_with_id_subscribers
branch
from
0f710c0 to
ea86808
Compare
desiprisg
force-pushed
the
replace_created_at_with_id_subscribers
branch
from
ea86808 to
b600c92
Compare
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer