PLT-4332 Position field for Users #4632

grundleborg · 2016-11-22T14:31:30Z

Summary

Adds a Position field to users, editable in the Account Settings general tab, and displayed below the name in the profile popover.

Ticket Link

#4188

https://mattermost.atlassian.net/browse/PLT-4332

Issues

~~Need to decide on if/how to integrate this change with other authentication systems.~~
When to update the API docs (given this new field isn't in a released version of MM yet)?

Checklist

Added or updated unit tests (required for all new features)
Added API documentation (required for all new APIs)
All new/modified APIs include changes to the drivers
Has enterprise changes: https://github.com/mattermost/enterprise/pull/71
Has UI changes
Includes text changes and localization file updates
Touches critical sections of the codebase (auth, upgrade, etc.)

enahum · 2016-11-22T17:23:28Z

i18n/en.json

@@ -3760,6 +3760,10 @@
    "translation": "Invalid nickname"
  },
  {
+    "id": "model.user.is_valid.position.app_error",
+    "translation": "Invalid position"


Maybe we should be more specific on why this field is invalid?

How does "Invalid Position: maximum length is 128 characters." sound?

enahum · 2016-11-22T17:27:13Z

webapp/components/user_settings/user_settings_general.jsx

+        if (this.props.activeSection === 'position') {
+            let extraInfo;
+            let submit = null;
+            if ((this.props.user.auth_service === 'ldap' || this.props.user.auth_service === Constants.SAML_SERVICE) && global.window.mm_config.PositionAttributeSet === 'true') {


If this is the case, we should add this to SAML and LDAP, if not we should remove this and add it later in case it applies

I'm going to raise this question on pre-release as I'm not sure of the answer myself.

Sorry, what is the question here? @grundleborg

@yangchen1 sorry, there isn't actually a question there! It was "Should the position field be populated automatically from external auth providers, like other fields are?". I think we've come to the conclusion that the answer is yes.

Ah OK, great! :) That sounds right.

enahum · 2016-11-22T17:29:17Z

webapp/components/user_settings/user_settings_general.jsx

+                describe = (
+                    <FormattedMessage
+                        id='user.settings.general.emptyPosition'
+                        defaultMessage="Click 'Edit' to add your position"


I don't think this string says much, is it related to your job position? maybe discuss this with a PM?

Deferring to a PM on this one.

@grundleborg If it isn't clear, let's say Click 'Edit' to add your job title / position.

enahum · 2016-11-22T17:32:48Z

Changing this to be reviewed by a PM first cause this qualifies as a new feature

yangchen1 · 2016-11-23T16:20:17Z

Overall looks good! Thanks @grundleborg I forgot to include the character limit in the spec, which should be no more than 35 characters including spaces. Otherwise it goes too out of the box here:

Also, this is probably not related to this PR, but clicking the profile picture seems to not display the position field, but clicking people's username does - it should probably show for both.

grundleborg · 2016-11-23T20:25:11Z

@yangchen1 fixed the limit to be 35 characters instead. You can also now see the Position attribute setting in LDAP and SAML settings pages of Admin Console.

yangchen1 · 2016-11-25T21:32:45Z

@grundleborg to add automatic truncation if LDAP position is longer than 35 characters

grundleborg · 2016-11-28T10:50:32Z

@yangchen1 fixed.

yangchen1 · 2016-12-05T14:08:19Z

Looks good! :) 👍

crspeller · 2016-12-05T14:42:44Z

config/config.json

@@ -188,7 +188,8 @@
        "SkipCertificateVerification": false,
        "QueryTimeout": 60,
        "MaxPageSize": 0,
-        "LoginFieldName": ""
+        "LoginFieldName": "",


Can we move this so it is with the other attributes?

crspeller · 2016-12-05T15:13:10Z

Discussion on pre-release: https://pre-release.mattermost.com/core/pl/iabmurbjx7y1z8u8off1mkcsjy

Let's make the back-end implementation be a custom field so we can expand on this later.

coreyhulen · 2016-12-10T00:01:35Z

store/sql_user_store.go

@@ -50,6 +50,7 @@ func NewSqlUserStore(sqlStore *SqlStore) UserStore {
 		table.ColMap("NotifyProps").SetMaxSize(2000)
 		table.ColMap("Locale").SetMaxSize(5)
 		table.ColMap("MfaSecret").SetMaxSize(128)
+		table.ColMap("Position").SetMaxSize(35)


I would set it to something more std like 64 chars then limit in the UI or via checks. We do this all over the place because we change our minds so often on size. That way the DB doesn't need to change, just a random constant somewhere.

OK, will change this to 64 char hard limit in DB, with validation/UI limit of 35 char.

Done. See: https://github.com/mattermost/enterprise/pull/71

grundleborg · 2016-12-13T13:52:24Z

@crspeller Sorry, UI fail. Was suposed to say that to @enahum and to add to your one that we decided against custom fields in dev meeting.

Done. See: https://github.com/mattermost/enterprise/pull/71

enahum

other than @crspeller and @coreyhulen comments

enahum · 2016-12-13T14:22:54Z

also I think it needs a rebase

grundleborg · 2016-12-13T16:18:32Z

@crspeller @coreyhulen rebased and fixed both your review comments.

coreyhulen · 2016-12-13T16:36:56Z

Feel free to merge once build is complete.

grundleborg · 2016-12-14T10:46:55Z

Please could someone with the necessary permissions merge this?

coreyhulen added the 2: Dev Review Requires review by a developer label Nov 22, 2016

enahum previously requested changes Nov 22, 2016

View reviewed changes

jwilander added 1: PM Review Requires review by a product manager and removed 2: Dev Review Requires review by a developer labels Nov 22, 2016

grundleborg added the Setup Old Test Server Triggers the creation of a test server label Nov 23, 2016

grundleborg force-pushed the PLT-4332 branch from 3e08b12 to a124919 Compare November 23, 2016 20:23

grundleborg added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Nov 23, 2016

enahum added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Nov 23, 2016

jasonblais added 2: Dev Review Requires review by a developer and removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Dec 5, 2016

crspeller previously requested changes Dec 5, 2016

View reviewed changes

jasonblais mentioned this pull request Dec 7, 2016

[Help Wanted] Add a "Position" field to Account Settings and profile popover #4188

Closed

coreyhulen suggested changes Dec 10, 2016

View reviewed changes

enahum approved these changes Dec 13, 2016

View reviewed changes

grundleborg added 2 commits December 13, 2016 15:25

Add User.Position field to store & model.

0dcab47

GOFMT

c7a4241

grundleborg added 14 commits December 13, 2016 15:25

Add Position to user settings.

23b70f3

Unit tests.

c3b08ee

Add position to profile popup.

fda0d4e

i18n

06cd26c

Fix log message for invalid position.

41da0fc

Add Position field attribute to LDAP config.

f503dad

Add Position field attribute to SAML config.

c6ba526

Reword empty position message.

fe13d58

Change Position Max Length to 35.

84f0691

Better invalid position error message.

af875f9

Add new fields to config.json.

640c812

Ensure position is never longer than max when displayed.

3ce5664

Hard limit of 64 chars with soft limit still 35

9969a1a

Put field with other attributes.

32bc67e

grundleborg force-pushed the PLT-4332 branch from b6d55df to 32bc67e Compare December 13, 2016 16:17

coreyhulen approved these changes Dec 13, 2016

View reviewed changes

coreyhulen added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Dec 13, 2016

enahum merged commit 8406e85 into mattermost:master Dec 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PLT-4332 Position field for Users #4632

PLT-4332 Position field for Users #4632

grundleborg commented Nov 22, 2016 •

edited

enahum Nov 22, 2016

grundleborg Nov 23, 2016

grundleborg Nov 23, 2016

enahum Nov 22, 2016

grundleborg Nov 23, 2016

yangchen1 Nov 23, 2016

grundleborg Nov 23, 2016

yangchen1 Nov 23, 2016

grundleborg Nov 23, 2016

enahum Nov 22, 2016

grundleborg Nov 23, 2016

yangchen1 Nov 23, 2016

grundleborg Nov 23, 2016

enahum commented Nov 22, 2016

yangchen1 commented Nov 23, 2016

grundleborg commented Nov 23, 2016

yangchen1 commented Nov 25, 2016

grundleborg commented Nov 28, 2016

yangchen1 commented Dec 5, 2016

crspeller Dec 5, 2016

crspeller commented Dec 5, 2016

coreyhulen Dec 10, 2016

grundleborg Dec 13, 2016

grundleborg commented Dec 13, 2016

enahum left a comment

enahum commented Dec 13, 2016

grundleborg commented Dec 13, 2016

coreyhulen commented Dec 13, 2016

grundleborg commented Dec 14, 2016

PLT-4332 Position field for Users #4632

PLT-4332 Position field for Users #4632

Conversation

grundleborg commented Nov 22, 2016 • edited

Summary

Ticket Link

Issues

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum commented Nov 22, 2016

yangchen1 commented Nov 23, 2016

grundleborg commented Nov 23, 2016

yangchen1 commented Nov 25, 2016

grundleborg commented Nov 28, 2016

yangchen1 commented Dec 5, 2016

Choose a reason for hiding this comment

crspeller commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grundleborg commented Dec 13, 2016

enahum left a comment

Choose a reason for hiding this comment

enahum commented Dec 13, 2016

grundleborg commented Dec 13, 2016

coreyhulen commented Dec 13, 2016

grundleborg commented Dec 14, 2016

grundleborg commented Nov 22, 2016 •

edited