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

Add opt-out option for email notifications in user settings (#163) #176

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arafat4693
Copy link

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feature
  • Why was this change needed? (You can also link to an open issue here)
    Fixes 🚀 Feature: Disable email notifications from settings #163
    This PR introduces an opt-out option for email notifications in system settings. Users can now choose whether they want to receive transactional emails.

Copy link

vercel bot commented May 25, 2024

@arafat4693 is attempting to deploy a commit to the Listinai Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@nevo-david nevo-david left a comment

Choose a reason for hiding this comment

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

Hi @arafat4693, thank you so much!
Left you some comments :)

) {
return this._organizationService.deleteTeamMember(org, id);
}

@Get('/email-notifications/:userId')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit dangerous because the userId can be faked. You should use the same technic in other routes

@GetUserFromRequest() user: User

return this._usersService.getEmailNotifications(userId);
}

@Post('/email-notifications/:userId')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here

const newSetting = !isChecked;
setIsChecked(newSetting);

await fetch(`/settings/email-notifications/${user.id}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to pass the user.id, we have the authenticated user

import { useVariables } from '@gitroom/react/helpers/variable.context';

const general = isGeneral();

Check warning

Code scanning / ESLint

Disallow unused variables Warning

'general' is assigned a value but never used.
@jamesread
Copy link
Collaborator

@arafat4693 , are you still interested in this PR?

@arafat4693
Copy link
Author

This PR is done. Waiting for It to be merged

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.

🚀 Feature: Disable email notifications from settings
3 participants