-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add message count limit #3272
Add message count limit #3272
Conversation
…d-message-count-limit
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.
hey, left you a few comments ;) also you forgot to assign the ticket ;)
apps/api/src/app/messages/usecases/get-messages/get-messages.command.ts
Outdated
Show resolved
Hide resolved
apps/api/src/app/messages/usecases/get-messages/get-messages.usecase.ts
Outdated
Show resolved
Hide resolved
apps/widget/cypress/plugins/index.ts
Outdated
async awaitRunningJobs({ templateId, organizationId }: { templateId: string; organizationId: string }) { | ||
await awaitRunningJobs({ apiKey: config.env.API_URL, templateId, organizationId }); | ||
|
||
return true; | ||
}, |
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 don't need to have this wrapper function, instead assign the default value to the awaitRunningJobs:132
, like
async function awaitRunningJobs({
apiKey = config.env.API_URL,
...
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.
Created this one to use it in notifications-list.spec.ts
in order to try to solve the flaky tests in the CI. :(
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.
sorry what I meant is that you don't have to call awaitRunningJobs
in line 116... instead you can do:
const session = new UserSession(serverUrl);
await session.awaitRunningJobs(templateId, undefined, 0, organizationId);
like you don't need to have that additional function awaitRunningJobs
# Conflicts: # apps/api/src/app/subscribers/subscribers.controller.ts # apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts # apps/api/src/app/widgets/widgets.controller.ts
What change does this PR introduce?
In this PR we introduce the count limit param.
Why was this change needed?
We need this additional limit query option, in order to reduce the number of scanned objects/returned on MongoDB.
Other information (Screenshots)