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

[MM-18830] Unhelpful error message when adding bot to a channel before adding to team #12844

Merged
merged 6 commits into from
Oct 31, 2019

Conversation

iomodo
Copy link
Contributor

@iomodo iomodo commented Oct 20, 2019

Summary

This PR fixes error message. Message will be:
{{.Username}} is not a member of the team

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-18830

@iomodo iomodo added this to the v5.17.0 milestone Oct 20, 2019
@iomodo iomodo requested review from mkraft and cpanato October 20, 2019 10:31
Copy link
Contributor

@mkraft mkraft left a comment

Choose a reason for hiding this comment

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

LGTM. Small (0/5) suggestion in case it matters to the translators.

i18n/en.json Outdated
@@ -798,6 +798,10 @@
"id": "api.command_invite.user_already_in_channel.app_error",
"translation": "{{.User}} is already in the channel."
},
{
"id": "api.command_invite.user_not_in_team.app_error",
"translation": "{{.User}} is not a member of the team."
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
"translation": "{{.User}} is not a member of the team."
"translation": "{{.Username}} is not a member of the team."

@@ -143,6 +143,10 @@ func (me *InviteProvider) DoCommand(a *App, args *model.CommandArgs, message str
var text string
if err.Id == "api.channel.add_members.user_denied" {
text = args.T("api.command_invite.group_constrained_user_denied")
} else if err.Id == "store.sql_team.get_member.missing.app_error" {
text = args.T("api.command_invite.user_not_in_team.app_error", map[string]interface{}{
"User": userProfile.Username,
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
"User": userProfile.Username,
"Username": userProfile.Username,

@hanzei hanzei added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Oct 21, 2019
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Oct 21, 2019
@DHaussermann DHaussermann added the Setup Cloud Test Server Setup an on-prem test server label Oct 23, 2019
@iomodo
Copy link
Contributor Author

iomodo commented Oct 25, 2019

Gentle reminder @cpanato , @DHaussermann ^_^

@cpanato
Copy link
Contributor

cpanato commented Oct 29, 2019

@iomodo can you update this PR with latest master?

@cpanato cpanato added Do Not Merge Should not be merged until this label is removed and removed 2: Dev Review Requires review by a developer Do Not Merge Should not be merged until this label is removed labels Oct 29, 2019
@mattermod
Copy link
Contributor

Mattermost test server updated with git commit a56ceece101c5baa8de9ef1081c57e3c21d1377e.

Access here: https://mattermost-server-pr-12844.test.mattermost.cloud

Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Tested and works as expected. Left a comment before merging.

i18n/en.json Outdated
@@ -798,6 +798,10 @@
"id": "api.command_invite.user_already_in_channel.app_error",
"translation": "{{.User}} is already in the channel."
},
{
"id": "api.command_invite.user_not_in_team.app_error",
"translation": "{{.Username}} is not a member of the team."
Copy link
Member

Choose a reason for hiding this comment

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

This results to:
Screen Shot 2019-10-31 at 3 20 10 PM

I wonder if it should have @ before the username. Not sure if all but most of system messages have that convention so that it's clickable and can see details on profile popover. @asaadmahmood Any thoughts before merging this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, we should use an @ when the username is being shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @ to the username. There are lots of other uses of username without @ though.

Copy link
Member

Choose a reason for hiding this comment

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

@saturninoabril saturninoabril removed the request for review from DHaussermann October 31, 2019 07:28
@saturninoabril saturninoabril added QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process) and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Oct 31, 2019
@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed Setup Cloud Test Server Setup an on-prem test server labels Oct 31, 2019
@mattermod
Copy link
Contributor

Test server creation failed. See the logs for more information.

@iomodo iomodo merged commit b208bbc into master Oct 31, 2019
@iomodo iomodo deleted the MM-18830-unhelpful-error branch October 31, 2019 08:49
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 31, 2019
lieut-data pushed a commit that referenced this pull request Oct 31, 2019
* Fix error message

* Rename User to Username

* Add '@' to the username
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Oct 31, 2019
jozuenoon pushed a commit to jozuenoon/mattermost-server that referenced this pull request Nov 2, 2019
* master: (70 commits)
  Run unused against codebase (mattermost#12968)
  [MM-12623] Create CLI command "config reset" (mattermost#10296)
  Migrate tests from store/storetest/status_store.go to use test… (mattermost#12873)
  Fix golangci-lint target (mattermost#12985)
  [MM-16437] Plugin crashes the server when calling WriteHeader with an invalid http code (mattermost#11276)
  MM-18060: Include deleted posts in compliance export. (mattermost#12957)
  [MM-18331] When patching a bot send websocket notification (mattermost#12373)
  [MM-19473] Fix data race on user login (mattermost#12870)
  license, openGraph tests: convert to testify (mattermost#12919)
  oauth_test: use testify (mattermost#12949)
  [MM-18830] Unhelpful error message when adding bot to a channel before adding to team (mattermost#12844)
  emoji_test: update to use testify (mattermost#12932)
  MM-19310 - Wrong validation message when Bot name ends in a . (mattermost#12905)
  Migrate tests from store/storetest/oauth_store.go to use testify (mattermost#12875)
  Include extra metadata when clicking an interactive button (mattermost#12697)
  MM-19553: Generate valid passwords on bulk import. (mattermost#12871)
  Convert api4/webhook_test.go t.Fatal calls into require/assert calls (mattermost#12904)
  Fix golangci-lint target for non GOPATH installations (mattermost#12934)
  MM-17888 Check plugin Helpers minimum server version comments (mattermost#12663)
  [MM-18898] Stringify plugin.Log* parameters (mattermost#12700)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants