Skip to content

Conversation

@shoom3301
Copy link
Contributor

/api/notification-list endpoint was not considering different cases for account parameter.
Now it select notifications for both original and lowercased.

@shoom3301 shoom3301 requested a review from a team June 19, 2024 11:13
@shoom3301 shoom3301 self-assigned this Jun 19, 2024
@shoom3301 shoom3301 merged commit dff026d into main Jun 19, 2024
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

👍

notification_template: notificationsTemplateFilter(push)
$or: [
{ account, notification_template: templateFilter },
{ account: account.toLowerCase(), notification_template: templateFilter },
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why not just to lowecase it when we save it in the db so we don't need to implement the OR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be added from admin panel UI and we can't control the value there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

okok

@alfetopito alfetopito deleted the fix/notifications-case-sens branch June 19, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants