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

[15921] Disable email notifications for the bot user #20938

Closed
wants to merge 3 commits into from

Conversation

ashikjm
Copy link

@ashikjm ashikjm commented Sep 2, 2022

Summary

This commit disables the email notifications for the bot accounts. Currently, they are set to true which caused bounce back messages when someone DM's the bot.

Additionally, when a user is converted to bot, the email notifications are disabled and when a bot is converted to user, email notifications are enabled.

Ticket Link

Fixes #15921

Release Notes


* Disable email notifications for bot users by default. Also, disable email notifications if a user is converted to bot and enable if vice-versa.

@mm-cloud-bot
Copy link

@ashikjm: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mattermod
Copy link
Contributor

Hello @ashikjm,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mm-cloud-bot mm-cloud-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed labels Sep 2, 2022
@isacikgoz isacikgoz added the 2: Dev Review Requires review by a developer label Sep 6, 2022
@isacikgoz
Copy link
Member

Hey @ashikjm thanks for the contribution! Let us know if you need any help regarding to failing tests.

@ashikjm
Copy link
Author

ashikjm commented Sep 12, 2022

@isacikgoz yes please. All the tests are failing with the same error.

app/users/users.go Outdated Show resolved Hide resolved
@ashikjm ashikjm requested review from mkraft and removed request for isacikgoz September 13, 2022 04:32
@ashikjm
Copy link
Author

ashikjm commented Sep 13, 2022

@isacikgoz Looks like re-requesting review from @mkraft (as there were some changes suggested) have removed you from the reviewers list. I dont have the permissions to add you back to the reviewers list. :(

Regarding the tests, I see they are failing stating that there are no test files for wsapi. Could you guide me here?

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

Sorry, wrong PR :)

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

Commented about the error in the tests. Apart from that, can you please also add a couple of app level test for this case? Let's validate that emails are not sent for the our cases. Also validate it's not affecting user creation path.

t.Run("fetch user 1", func(t *testing.T) {
actual, err := ss.User().GetNotifyProps(u1.Id)
require.NoError(t, err)
require.Equal(t, u1, actual)
Copy link
Member

Choose a reason for hiding this comment

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

You are trying to compare a *model.User to a map[string]string instance here. Hence it's failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

gentle ping about this comment.

@isacikgoz
Copy link
Member

Hi @ashikjm, big thanks for your contribution! There hasn't been recent activity on this PR so I've marked it as inactive. Following our inactive contribution process, the PR is now closed due to 30 days or more of inactivity.

However, if you're still interested working on the changes, let us know! Note that we merged multiple repositories into one monorepo, which may require you to resubmit the changes.

If you have any questions, don't hesitate to let us know, we're happy to help.

@isacikgoz isacikgoz closed this Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a developer Contributor Lifecycle/1:stale release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bot Accounts get created with email notification of Immediately
5 participants