Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: change limit to 100 from 1000 #3631

Merged
merged 14 commits into from
Jul 13, 2023
Merged

feat: change limit to 100 from 1000 #3631

merged 14 commits into from
Jul 13, 2023

Conversation

davidsoderberg
Copy link
Contributor

What change does this PR introduce?

Change limit from 100 to 1000 for notification center usage

Why was this change needed?

To reduce load added from the notification center requests

@linear
Copy link

linear bot commented Jun 21, 2023

NV-2161 [v0.16] Update the count query to 100 in the API

Why? (Context)

In the current scope, in order to reduce the amount of the examined and returned documents from MongoDB we will update the API to 100 in our system with API query, but leave the default for other API users to 1000.

What?

This task Score:

  • Update the API so it will use the default of 100 instead of 1000.
  • Remove the 'totalCount' in the feed and messages request to the API and update the 'hasMore' compute logic.

I hope I found all the places that will be required to update in the code, I flagged them with this task number "NV-2161" so you may search for it so it will be easy to find.

@davidsoderberg davidsoderberg requested review from djabarovgeorge, scopsy and p-fernandez and removed request for djabarovgeorge June 21, 2023 06:24
djabarovgeorge
djabarovgeorge reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@djabarovgeorge djabarovgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to sync about it as well, tried the coffee time so we get Dima as well but i am not sure if will make it 😅

apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts Outdated Show resolved Hide resolved
apps/api/src/app/messages/usecases/get-messages/get-messages.usecase.ts Outdated Show resolved Hide resolved
@davidsoderberg davidsoderberg force-pushed the nv-2161-v016-update-the-count-query-to-100-in-the-api branch from fe9b0f3 to cbd9b87 Compare June 21, 2023 11:22
djabarovgeorge
djabarovgeorge reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@djabarovgeorge djabarovgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good left a couple of comments there.

apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts Show resolved Hide resolved
apps/api/src/app/messages/usecases/get-messages/get-messages.usecase.ts Outdated Show resolved Hide resolved
apps/api/src/app/subscribers/subscribers.controller.ts Outdated Show resolved Hide resolved
p-fernandez
p-fernandez approved these changes Jun 21, 2023
View reviewed changes
libs/dal/src/repositories/message/message.repository.ts
@@ -91,7 +91,7 @@ export class MessageRepository extends BaseRepository<MessageDBModel, MessageEnt
subscriberId: string,
channel: ChannelTypeEnum,
query: { feedId?: string[]; seen?: boolean; read?: boolean } = {},
options: { limit: number } = { limit: 1000 } // todo NV-2161 update to 100 in version 0.16
options: { limit: number; skip?: number } = { limit: 100, skip: 0 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to start to centralise these in shared constants so we avoid to miss them when updating. One for the tech debt.

@davidsoderberg davidsoderberg marked this pull request as ready for review June 22, 2023 11:33
djabarovgeorge
djabarovgeorge reviewed Jun 22, 2023
View reviewed changes
Copy link
Contributor

@djabarovgeorge djabarovgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks good, left a couple of comments on there regarding things i was not sure about.

apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts Outdated Show resolved Hide resolved
apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts Outdated Show resolved Hide resolved
@davidsoderberg @djabarovgeorge
Update apps/api/src/app/widgets/usecases/get-notifications-feed/get-n…
c3df760 
…otifications-feed.usecase.ts

Co-authored-by: George Djabarov <39195835+djabarovgeorge@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Pull reviewers stats

Stats of the last 30 days for novu:

User Total reviews Time to review Total comments
p-fernandez 48 1h 38m 147
scopsy 15 24m 3
djabarovgeorge 15 1d 23h 18m 54
LetItRock 14 17h 47m 58
ainouzgali 10 48m 50
unicodeveloper 9 7d 8h 29m 96
davidsoderberg 5 15h 55m 3
BiswaViraj 3 1d 22h 30m 2
jainpawan21 2 7h 59m 2
peoray 1 27m 1
michaldziuba03 1 10h 53m 1
Cliftonz 1 5d 54m 0

p-fernandez
p-fernandez suggested changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the bit of code that looks off. The limit for a tech debt.

apps/api/src/app/widgets/usecases/get-feed-count/get-feed-count.command.ts
@@ -16,8 +16,7 @@ export class GetFeedCountCommand extends EnvironmentWithSubscriber {
@IsOptional()
@Transform(({ value }) => {
if (isNaN(value) || value == null) {
// todo NV-2161 update the limit default to 100 to in version 0.16
return 1000;
return 100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should centralize the selected values and reduce the hardcodes. 🙏🏻

apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts Outdated Show resolved Hide resolved
p-fernandez
p-fernandez approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟

@davidsoderberg davidsoderberg added this pull request to the merge queue Jul 13, 2023
Merged via the queue into next with commit e2ef536 Jul 13, 2023
@davidsoderberg davidsoderberg deleted the nv-2161-v016-update-the-count-query-to-100-in-the-api branch July 13, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djabarovgeorge djabarovgeorge djabarovgeorge left review comments

@p-fernandez p-fernandez p-fernandez approved these changes

@scopsy scopsy Awaiting requested review from scopsy

Assignees
No one assigned
Labels
@novu/api @novu/dal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidsoderberg @p-fernandez @djabarovgeorge @scopsy