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

Changing User name field #968

Closed
morenoh149 opened this issue Jan 29, 2015 · 9 comments
Closed

Changing User name field #968

morenoh149 opened this issue Jan 29, 2015 · 9 comments
Labels
Milestone

Comments

@morenoh149
Copy link
Contributor

Is there a setting I must enable to change a user's name attribute?
screenshot 2015-01-29 12 51 08

'use strict';
var keystone = require('keystone'),
    Types = keystone.Field.Types;

var User = new keystone.List('User');

User.add({
  name: {
    type: Types.Name,
    required: true,
    index: true
  },
  email: {
    type: Types.Email,
    initial: true,
    required: true,
    index: true
  },
  password: {
    type: Types.Password,
    initial: true,
    required: true
  }
}, 'Permissions', {
  isAdmin: {
    type: Boolean,
    label: 'Can access Keystone',
    index: true
  }
});

// Provide access to Keystone
User.schema.virtual('canAccessKeystone').get(function() {
  return this.isAdmin;
});

/**
 * Relationships
 */
// User.relationship({
//   ref: 'Post',
//   path: 'author'
// });

/**
 * Registration
 */
User.defaultColumns = 'name, email, isAdmin';
User.register();
@snowkeeper
Copy link
Contributor

Confimed for master.
noname

@snowkeeper snowkeeper added the bug label Jan 30, 2015
@morenoh149 morenoh149 added this to the 0.3.x milestone Jan 30, 2015
@snowkeeper
Copy link
Contributor

I did a little digging and found where it seems the error is happening, but I am not sure if it is expected for some reason.

https://github.com/keystonejs/keystone/blob/master/lib/list.js#L83-L85

Object.defineProperty(this, 'nameIsEditable', { get: function() {
        return (this.fields[this.mappings.name] && this.fields[this.mappings.name].type === 'text') ? !this.fields[this.mappings.name].noedit : false;
    }});

This checks if the field is type text. Changing it to test type === 'name' and changing the name field def to include noedit: false shows the field as it should.

I can make the PR if y'all confirm that is the actual culprit.

@snowkeeper
Copy link
Contributor

I tried some other things and found the actual blocker that stops the UI from functioning like 0.2.x.
https://github.com/keystonejs/keystone/blob/master/lib/list.js#L140-L142

if (el.field === this.nameField || el.field.hidden) {
        return;
}

Removing el.field === this.nameField || fixes the UI issue without adjusting
https://github.com/keystonejs/keystone/blob/master/lib/list.js#L83-L85

Object.defineProperty(this, 'nameIsEditable', { get: function() {
        return (this.fields[this.mappings.name] && this.fields[this.mappings.name].type === 'text') ? !this.fields[this.mappings.name].noedit : false;
    }});

Although I am wondering if nameIsEditable is functiong as intended.

@JedWatson when you get settled back in can you give some insight.

@snowkeeper
Copy link
Contributor

Changing nameIsEditable gives a UI like so:
1_007

Changing getOptions() produces a UI like 0.2.x:
1_005

@morenoh149
Copy link
Contributor Author

ah so then it sounds like it was intended to be configurable. I think changing it so nameIsEditable is true by default would be better.

Whether the behaviour changes or not we should add docs here http://keystonejs.com/docs/database/#fieldtypes-name

@snowkeeper
Copy link
Contributor

I figured everything out I think...

Keystone makes special use of a Field defined name to display a header on the form.

Object.defineProperty(this, 'nameIsEditable', { get: function() {
        return (this.fields[this.mappings.name] && this.fields[this.mappings.name].type === 'text') ? !this.fields[this.mappings.name].noedit : false;
}});

nameIsEditable is setting a boolean true if a name field is not explicitly defined as noedit:true, but is only checking for the type text. So Types.Select, Types.Name etc are defined false and are not allowed to be editable in 0.3.0 due to getOptions() not including name fields in list.uiElements.

So the new code preventing editing a name field that is not defined as Types.Text or String is from getOptions(). getOptions provides the fields for the form and skips name fields.
https://github.com/keystonejs/keystone/blob/master/lib/list.js#L140-L142

if (el.field === this.nameField || el.field.hidden) {
        return;
}

Since nameIsEditable is always false for name fields not defined as text, the field is displayed as simple text.
I imagine this is a styling choice to stop using a true header for name all the time as in 0.2.x and let the name field be editable.

If we want to keep the same look as in 0.2.x then getOptions should be fixed.
0 2 x name

If we want to make the name field a header and editable then we can update nameIsEditable to stop checking type.

Object.defineProperty(this, 'nameIsEditable', { get: function() {
        return (this.fields[this.mappings.name]) ? !this.fields[this.mappings.name].noedit : false;
}});

0 3 0 name

@sebmck sebmck closed this as completed in 69b4902 Feb 5, 2015
@snowkeeper
Copy link
Contributor

@sebmck , I am still getting text for Types.Name in the UI without an editable field.
tester user admin user - google chrome_021

sebmck added a commit that referenced this issue Feb 5, 2015
@sebmck
Copy link
Contributor

sebmck commented Feb 5, 2015

@snowkeeper Whoops, forgot to remove a condition. Fixed, let me know if that did the trick.

@snowkeeper
Copy link
Contributor

Thanks @sebmck, looks good!
tester user admin user - google chrome_022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants