create fix for undefined substring call #324

csduarte · 2017-11-20T20:57:48Z

Summary

Adds a fix for an undefined method call for the team button in the team sidebar.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)
Has server changes (please link)
Has redux changes (please link)
Has UI changes
Includes text changes and localization file (.../i18n/en.json) updates
Touches critical sections of the codebase (auth, posting, etc.)

@dmeza

saturninoabril · 2017-11-21T09:23:00Z

components/team_sidebar/components/team_button.jsx

@@ -49,10 +49,11 @@ export default class TeamButton extends React.Component {

 let btn;
 let content = this.props.content;
+ const shortName = this.props.displayName ? this.props.displayName.substring(0, 2) : '??';


Looks good to me except for the use of ??.
@jasonblais Could you please validate ?? if we could use this convention whenever displayName is undefined?

Yes this feels weird, if the displayName is not set I would use the name of the team, I don't think ?? will do the job at all

@saturninoabril @enahum How do I validate this? Do I need to test the team icons in the sidebar?

yes the text displayed in the team icon should never be ?? as that does not tell me what team is it, and apart from that what I'm seeing is that if somehow the displayName is not set it will not only set the name as ?? but the tooltip will also be empty

@dmeza Which issue is this PR resolving?

I was trying to test the fix on the test server, and it seems to work fine, but I'm not sure if I have the correct repro steps since I'm unable to create a team without a display name.

Honestly, I have not seen that happened to team display name (being null). Maybe, the first thing is to request from @dmeza about the repro steps on how he observed null displayName being passed to TeamButton.

I know this seems odd, we thought so at the time, but it is possible. By looking at code we could not figure out why, debugging was hard. It completely killed production at one point during launch. So it's best we just guard against this knowing it's not a likely case, but is possible.

Again, we didn't do the ?? for style, we did it because the web app would not load without fixing this.

Thanks! The context is useful, have queued for review by our design team.

@dmeza Waiting on design team's feedback -- just curious if it's easy to set a non-empty tooltip for this case like Name undefined? Just want to gauge the level of effort,

@dmeza The ?? should be okay in the team icon. I'm not sure what else would be better instead. It would be nice if there was a tooltip in that case with text Name undefined, but depends on level of effort for adding it.

dmeza · 2017-12-07T15:26:53Z

@jasonblais @saturninoabril rebased to the latest from master and added a Tooltip Name Undefined when Team displayName is null.

jasonblais

PM approved

saturninoabril · 2017-12-08T00:11:47Z

components/team_sidebar/components/team_button.jsx

@@ -70,14 +70,15 @@ export default class TeamButton extends React.Component {
 </div>
 );
 } else {
+ const toolTip = this.props.displayName ? this.props.tip : 'Name undefined';


@jasonblais Should Name undefined be localized?

@dmeza All looks good to me except for a minor request to have this localized

localizeMessage('team.button.name_undefined', 'Name undefined')

saturninoabril · 2017-12-08T10:11:32Z

components/team_sidebar/components/team_button.jsx

@@ -70,14 +70,15 @@ export default class TeamButton extends React.Component {
 </div>
 );
 } else {
+ const toolTip = this.props.displayName ? this.props.tip : 'Name undefined';


@dmeza All looks good to me except for a minor request to have this localized

localizeMessage('team.button.name_undefined', 'Name undefined')

dmeza · 2017-12-08T14:18:27Z

@saturninoabril rebased to latest from master and added localized message for Name undefined.

saturninoabril

Cool, thanks!

jwilander

LGTM, could you just give this one final rebase and we can merge?

dmeza · 2017-12-13T17:10:13Z

@jwilander rebased to latest from master.

dmeza · 2017-12-18T21:59:55Z

@jwilander rebased to latest from master.

* create fix for undefined substring call * Add Tooltip `Name Undefined` when Team displayName is null. * Add localized message for `Name undefined`

jasonblais added the 2: Dev Review Requires review by a core commiter label Nov 20, 2017

coreyhulen requested review from saturninoabril and enahum November 21, 2017 07:14

saturninoabril reviewed Nov 21, 2017

View reviewed changes

saturninoabril added the 1: PM Review Requires review by a product manager label Nov 21, 2017

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Nov 21, 2017

jasonblais self-assigned this Nov 21, 2017

dmeza force-pushed the bash-2_undefined-call branch from a946edb to b8a9420 Compare November 21, 2017 16:30

jasonblais added the Awaiting Submitter Action Blocked on the author label Nov 29, 2017

dmeza force-pushed the bash-2_undefined-call branch from b8a9420 to f4a0f58 Compare December 7, 2017 15:17

dmeza force-pushed the bash-2_undefined-call branch from f4a0f58 to 300c56d Compare December 7, 2017 15:45

jasonblais approved these changes Dec 7, 2017

View reviewed changes

jasonblais removed 1: PM Review Requires review by a product manager Awaiting Submitter Action Blocked on the author Setup Old Test Server Triggers the creation of a test server labels Dec 7, 2017

mattermost deleted a comment from mattermod Dec 7, 2017

jasonblais removed their assignment Dec 7, 2017

saturninoabril reviewed Dec 8, 2017

View reviewed changes

saturninoabril suggested changes Dec 8, 2017

View reviewed changes

dmeza force-pushed the bash-2_undefined-call branch from 300c56d to 2c61262 Compare December 8, 2017 14:17

saturninoabril approved these changes Dec 8, 2017

View reviewed changes

jwilander approved these changes Dec 13, 2017

View reviewed changes

jwilander removed the request for review from enahum December 13, 2017 16:59

dmeza force-pushed the bash-2_undefined-call branch from 2c61262 to 399e06e Compare December 13, 2017 17:07

create fix for undefined substring call

eb1386e

David Meza added 2 commits December 18, 2017 16:53

Add Tooltip Name Undefined when Team displayName is null.

b11ef39

Add localized message for Name undefined

62bdf75

dmeza force-pushed the bash-2_undefined-call branch from 399e06e to 62bdf75 Compare December 18, 2017 21:54

grundleborg added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Dec 20, 2017

jwilander merged commit e0457ee into mattermost:master Dec 21, 2017

lindalumitchell added the Tests/Not Needed Does not require new release tests label Dec 29, 2017

jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 4, 2018

dmeza deleted the bash-2_undefined-call branch December 7, 2018 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create fix for undefined substring call #324

create fix for undefined substring call #324

csduarte commented Nov 20, 2017 •

edited

Loading

saturninoabril Nov 21, 2017

enahum Nov 21, 2017

jasonblais Nov 21, 2017

enahum Nov 21, 2017

jasonblais Nov 21, 2017

saturninoabril Nov 22, 2017

csduarte Nov 22, 2017

jasonblais Nov 23, 2017

jasonblais Nov 23, 2017

jasonblais Nov 29, 2017

dmeza commented Dec 7, 2017 •

edited

Loading

jasonblais left a comment

saturninoabril Dec 8, 2017

saturninoabril Dec 8, 2017

saturninoabril Dec 8, 2017

dmeza commented Dec 8, 2017

saturninoabril left a comment

jwilander left a comment

dmeza commented Dec 13, 2017

dmeza commented Dec 18, 2017

create fix for undefined substring call #324

create fix for undefined substring call #324

Conversation

csduarte commented Nov 20, 2017 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeza commented Dec 7, 2017 • edited Loading

jasonblais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeza commented Dec 8, 2017

saturninoabril left a comment

Choose a reason for hiding this comment

jwilander left a comment

Choose a reason for hiding this comment

dmeza commented Dec 13, 2017

dmeza commented Dec 18, 2017

csduarte commented Nov 20, 2017 •

edited

Loading

dmeza commented Dec 7, 2017 •

edited

Loading