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.

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

NC - dropdown menu with remove message and read unread action #2766

Merged
merged 8 commits into from
Feb 20, 2023

Conversation

ainouzgali
Copy link
Contributor

@ainouzgali ainouzgali commented Feb 14, 2023

What change does this PR introduce?

Why was this change needed?

https://www.loom.com/share/f1c63987ec5844e181b3683e6e1a32e3

Screen Shot 2023-02-14 at 20 16 19

Other information (Screenshots)

WIP:
still needs yo add tests
add to headless
tidy up code
improve design

@linear
Copy link

linear bot commented Feb 14, 2023

NV-1612 In App - remove a notificatino - ellipses

On right click / click on elipses:
A drop down is opened and a remove option is shown.

Let's try to do it with the existing design system without a designer.

More details:
https://www.notion.so/novuhq/InApp-Notification-Center-9ef54ad03bcc475bbc1e72026fcb8a4d#11d76a9d41ee41638de00ff21bea1eb9


Created via Raycast

@ainouzgali ainouzgali marked this pull request as ready for review February 19, 2023 09:20
Copy link
Contributor

@scopsy scopsy left a comment

Choose a reason for hiding this comment

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

Amazing work, added a few notes regarding the API 🎉

Comment on lines +66 to +87
private async updateServices(command: RemoveMessageCommand, subscriber, message, marked: string) {
const admin = await this.memberRepository.getOrganizationAdminAccount(command.organizationId);
const count = await this.messageRepository.getCount(command.environmentId, subscriber._id, ChannelTypeEnum.IN_APP, {
[marked]: false,
});

this.updateSocketCount(subscriber, count, marked);

if (admin) {
this.analyticsService.track(`Removed Message - [Notification Center]`, admin._userId, {
_subscriber: message._subscriberId,
_organization: command.organizationId,
_template: message._templateId,
});
}
}

private updateSocketCount(subscriber: SubscriberEntity, count: number, mark: string) {
const eventMessage = `un${mark}_count_changed`;
const countKey = `un${mark}Count`;

this.queueService.wsSocketQueue.add({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have those things duplicated elsewhere right? Maybe we can have a reusable function to do the repeated behaviour?

apps/api/src/app/widgets/widgets.controller.ts Outdated Show resolved Hide resolved
messageId: messageId,
});

return await this.removeMessageUsecase.execute(command);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any e2e test for this endpoint (API) do we have one?

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

hey, super excellent job 🎉 I love the approach you took and you didn't forget about the docs :) thanks!
left some small suggestions ;)

_organizationId: command.organizationId,
_id: command.messageId,
});
const item = await this.messageRepository.findDeleted({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const item = await this.messageRepository.findDeleted({
const [deletedMessage] = await this.messageRepository.findDeleted({

return deletedMessage;
}

private async updateServices(command: RemoveMessageCommand, subscriber, message, marked: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix the types here... I do see that this code is duplicated, should we expose it to a separate use-case?

if (admin) {
this.analyticsService.track(`Removed Message - [Notification Center]`, admin._userId, {
_subscriber: message._subscriberId,
_organization: command.organizationId,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should add here the env id too?

async removeMessage(
@SubscriberSession() subscriberSession: SubscriberEntity,
@Param('messageId') messageId: string
): Promise<MessageEntity> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should be consistent with our API, currently, we have many different approaches when it comes to DELETE REST calls, sometimes we return true, sometimes deleted object, and in other scenarios 204 no content...
maybe we should talk about it on the tech meeting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I was facing the same confusion about what to return

packages/headless/src/lib/headless.service.ts Show resolved Hide resolved
@ainouzgali
Copy link
Contributor Author

I will extract the duplicated code + add more tests in a separate PR later tonight/tomorrow

@ainouzgali ainouzgali added this pull request to the merge queue Feb 20, 2023
Merged via the queue into next with commit cd0b383 Feb 20, 2023
@ainouzgali ainouzgali deleted the nv-1612-in-app-remove-a-notificatino-ellipses branch February 20, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants