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

No longer mention the author of an event in the channel #108 #188

Closed
wants to merge 3 commits into from

Conversation

EugenMayer
Copy link

Right now we mention the user, who is the author of an event like "the one pushed an commit" in the channel and notify her/him about his own change, triggering a notification on the mobile phone, desktop notification and all this.

This is fairly intrusive and without any value to the user, since she/he is the author of the immediate action and already knows about it

Summary

Ticket Link



Right now we mention the user, who is the author of an event like "the one pushed an commit" in the channel and notify her/him about his own change, triggering a notification on the mobile phone, desktop notification and all this.

This is fairly intrusive and without any value to the user, since she/he is the author of the immediate action and already knows about it
@hanzei hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Feb 10, 2020
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

@EugenMayer, thanks for the suggested change! I'm torn between the pain you accurately describe, and the value linking to the mentioned author provides for anyone else consuming the message (e.g. I want to DM that user by clicking on their username).

I wonder if there's a way to get the best of both worlds, by somehow preventing this post from notifying just that user. cc @aaronrothschild to help provide direction here.

@lieut-data lieut-data added 2: Dev Review Requires review by a core committer and removed 2: Dev Review Requires review by a core committer labels Feb 10, 2020
@EugenMayer
Copy link
Author

@lieut-data i understand - sure i'am all for linking the author if possible - but if it is not possible to do so without the mention, i would rather not use the link syntax and live without it.

It is rather rare you click on the author directly - you rather click on the comment/pr/whatever - do what you need and maybe click on the author there ( IMHO )

@lieut-data
Copy link
Member

Understood re: clicking. I personally get a ton of these kinds of messages, and another benefit of linking to the Mattermost username is that I get my local username rendering for free. For example, I choose to see the following:

image

but I could just as easily configure to show first and last names, and more easily identify users instead:
image

Just throwing ideas out here, but I wonder if we could support a @!<username> that suppresses notifications. Or @hmhealey, is there a more elegant way we could accomplish selectively blocking some mentions (but leaving others intact, including possibly for the same user in a different part of the message)?

@lieut-data
Copy link
Member

@EugenMayer, just catching up on your comments on the linked issue -- and I wouldn't strongly oppose making this configurable given the clear pain points in your use case. Deferring to @aaronrothschild for the final call before continuing with developer review.

@hmhealey
Copy link
Member

Just throwing ideas out here, but I wonder if we could support a @! that suppresses notifications. Or @hmhealey, is there a more elegant way we could accomplish selectively blocking some mentions (but leaving others intact, including possibly for the same user in a different part of the message)?

I'd be hesitant to add something like that. Having an at-mention that doesn't trigger a notification seems like something that could cause some confusion since those are generally linked together.

Don't these messages usually appear in a DM channel between you and the bot? Since it's a DM, doesn't that cause the same notification as an at-mention would?

@aaronrothschild
Copy link
Contributor

@EugenMayer, just catching up on your comments on the linked issue -- and I wouldn't strongly oppose making this configurable given the clear pain points in your use case. Deferring to @aaronrothschild for the final call before continuing with developer review.

@EugenMayer Would the approach be something like /github notifications self off or something like that?

This is where a "Settings tab" for a bot would come in handy - for storing user settings for that particular plugin. (This is something we've discussed for future)

@aaronrothschild aaronrothschild removed the 1: PM Review Requires review by a product manager label Feb 12, 2020
@EugenMayer
Copy link
Author

@aaronrothschild i would suggest, if those settings are implemented, turn it into a "opt in".

I kind of have a stronger opinion here, but please take it as an pure IMHO:

Right now i'am not able to make up a scenario where those mentions would ever make sense or have a usage.

The only argument we had here is "the name reads easier in the message" - i fully agree on this point - but it is this only one.

On the contrary if notifications of mattermost are taken serious, a mention is a direct request for the person mentioned to interact with the question/message. It disturbs the user / sends a lot of push/email notifications right away - and it is the intend since it is "important and attention is required".

Considering what notifications we would have here, they do not fall into that category - in the contrary this notification is basically for everybody except the person mentioned - while bursting out notifications just exactly for that person.

This said, i would consider that option you mentioned as "i want a mention, turn it on" for all people and cases i here did not think about - or where "mentioning" is less important the "readability of the commit/pr author" ( seems like this is an odd trade in a chat/comm tool - but clearly an IMHO )

@aaronrothschild
Copy link
Contributor

@aaronrothschild i would suggest, if those settings are implemented, turn it into a "opt in".

I kind of have a stronger opinion here, but please take it as an pure IMHO:

Right now i'am not able to make up a scenario where those mentions would ever make sense or have a usage.

@EugenMayer I'm with you.

The only argument we had here is "the name reads easier in the message" - i fully agree on this point - but it is this only one.

This said, i would consider that option you mentioned as "i want a mention, turn it on" for all people and cases i here did not think about - or where "mentioning" is less important the "readability of the commit/pr author" ( seems like this is an odd trade in a chat/comm tool - but clearly an IMHO )

Agreed - let's make it Opt-in to get self-notifications as an option the user must set explicitly, but by default for users we will suppress notifications triggered by themselves.

@mickmister mickmister changed the title Do longer mention a the author of an event in the channel #108 No longer mention the author of an event in the channel #108 Feb 13, 2020
@EugenMayer
Copy link
Author

Seems it waits for a change by me, but i'am not able to spot what is requested? Did i miss it?

@lieut-data
Copy link
Member

Agreed - let's make it Opt-in to get self-notifications as an option the user must set explicitly, but by default for users we will suppress notifications triggered by themselves.

@aaronrothschild, I don't think that's currently feasible. @EugenMayer's approach here is just to disable at-mentioning the author of /any/ GitHub notification, so as to prevent notifying oneself. This change impacts everyone to prevent notifying the author in question.

I'd be hesitant to add something like that. Having an at-mention that doesn't trigger a notification seems like something that could cause some confusion since those are generally linked together.

@hmhealey: agreed, hence the opt-in to disable the notification part and just reference another user instead. This happens all the time in private channels where the user is not included and thus never gets a mention, but still helps everyone involved know precisely who is being referenced.

Don't these messages usually appear in a DM channel between you and the bot? Since it's a DM, doesn't that cause the same notification as an at-mention would?

I think this is occurring in the context of a channel configured to receive GitHub notifications. For example, I get notified abut my own PRs submitted to the demo plugin from the Toolkit channel.

I suppose it's too heavy handed to find a way to disable /all/ at-mentions for GitHub notification posts, and rely on those using the plugin to subscribe to notifications for themselves as needed?

@EugenMayer
Copy link
Author

EugenMayer commented Feb 15, 2020

I think this is occurring in the context of a channel configured to receive GitHub notifications. For example, I get notified abut my own PRs submitted to the demo plugin from the Toolkit channel.

Exactly what we have here too, we stream commits/opening PR's into our dev channel

I'd be hesitant to add something like that. Having an at-mention that doesn't trigger a notification seems like something that could cause some confusion since those are generally linked together.

I'am not sure what you mean, but i guess when i write i comment and in there i mention like @lieut-data. This would, if notified in mattermost, trigger 2 direct high-noice notifications: one from github and then and mattermost. Even though it is argueable this make sense for quiet some people, i understand, i did not plan to remove any @Mention syntax from message. If we actually find an @user mention in the body and we know which mattermost user @user is, and we can replace that with @mattermostuser to create a mattermost action - this would be just fine. It is an directy intention by the comment author to mention the other user.

But i'am neither aware what this has to do with this PR nor with the feature we are currently discussion about.

a) The above, there is user A mentioning user B - needs his attention (the case you are concerned about)
b) In this PR we talk about user A creating a commit/PR and this triggers a mention of himself to himfself (the case i'am concerned about)

In an environment where mattermost is the primary chat tool and the github is the primary SCM at the same time, b) makes the entire integration a blocker as described above.

While i would fully support a), i would say that allowing b) does a lot of harm.

Still i'am not sure how a) and b) are related

@lieut-data
Copy link
Member

@EugenMayer, my thinking is that an @mention serves two purposes: notifying someone (which only happens in a channel in which they are a member), and correlating the GitHub username with the Mattermost user for everyone else.

I think it's really powerful to transparently resolve GitHub usernames back to Mattermost usernames in context, even in this case. I'd love to fully explore options that solve the pain point /and/ do the username translation, hence the side conversation re: @!<username>.

@aaronrothschild, I'm hoping this gives you all the necessary context to drive this forward. It may very well be that we solve this pain point now, and look at the username translation asynchronously.

@EugenMayer
Copy link
Author

@lieut-data i have nothing against mapping a github name - but the sacrifice of "mentioning" the user who has done the action would be a non-acceptable trade for that. Being able to use @<someone> without a mention would of course be great though.

I would opt in for "disabling this for now" and implement "user name enrichment and mapping of github" in an extra PR.

Generally "displaying" and "mentioning" should not be interlinked this way

@mattermod
Copy link
Contributor

This issue 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!

/cc @jasonblais @hanzei

@hanzei hanzei added the Triage label Mar 2, 2020
@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Mar 7, 2020
@EugenMayer
Copy link
Author

Well i guess this one is dead in the water then :)

@lieut-data
Copy link
Member

Apologies, @EugenMayer -- there's been a lot of competing priorities the past few weeks, and it's definitely not intentional! I missed re-requesting a review from @aaronrothschild after our most recent discussion to ensure alignment. Hopefully that helps :)

@EugenMayer
Copy link
Author

No worries, great if it could be picked up any time soon, thanks!

@aaronrothschild
Copy link
Contributor

I would opt in for "disabling this for now" and implement "user name enrichment and mapping of github" in an extra PR.

@EugenMayer Apologies for the delay on this issue. I agree - it seems like a distinct feature. Mapping the GH username to the MM username has to be thought out a bit IMO.

@aaronrothschild, I don't think that's currently feasible. @EugenMayer's approach here is just to disable at-mentioning the author of /any/ GitHub notification, so as to prevent notifying oneself. This change impacts everyone to prevent notifying the author in question.

Just so I'm clear - the current PR functionality simply removes the inclusion of "@{owner}" in the notification message itself. Thus, a DM notification is not triggered.

What is the easiest way for us to make this an optional setting? We can place it in the plugin settings area, or we could setup a new slash command to store plugin settings (this would be a plugin-wide setting).

@mickmister mickmister removed their request for review April 8, 2020 01:58
@hanzei hanzei removed the Triage label Apr 21, 2020
@larkox larkox linked an issue Jun 2, 2020 that may be closed by this pull request
@EugenMayer
Copy link
Author

I think there are arguments for both sides, with mention and without. So i think having plugin settings with a switch is the best middleground, the default should be what we have right now, so with DM. If the community decides, over time, that this default is not the usual case, it can be still changed.

@EugenMayer
Copy link
Author

Time to say goodby to this one, clinicly dead :)

@EugenMayer EugenMayer closed this Sep 1, 2020
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.

Do not trigger mentions from plugin messages
7 participants