Keystone-classic: [v0.3] Boolean fields with noedit:false and hidden:true do not retain value

[creynders]

Expected behavior

If a hidden yet editable boolean field has a true value in the database, it should retain it when it's saved through the admin UI.

Actual behavior

The field will always be set to false, even if it were true before

Steps to reproduce the behavior

1/ Add a Boolean field to a list

    completed: {
      type: Boolean,
      default: false,
      noedit: false,
      hidden: true
    }

2/ add a pre('save') hook that will set it to true if it's not:

keystone.list('MyList').schema.pre('save', function(done){
  if(!this.completed){
    console.log('SETTING IT TO TRUE');
    this.completed = true;
  }
  done();
});

Re-saving the document in the admin UI will output 'SETTING IT TO TRUE' every time.

Motivation

This is useful for allowing documents that are constructed over time. I have several lists that allow people to come back and modify the document until a certain condition is met in which case further modification should no longer be allowed.

[creynders]

I.e. a temporary fix is to set noedit:true

[ddaddy]

Bumping this Bug as it can have some serious side effects.
I have isAdmin set to as hidden, and after editing my User record I was immediately kicked out of the Admin Panel as isAdmin automatically got set to false.

[jstockwin]

Is this simply the if (!value || value === 'false') { here?
The Boolean value is not provided in the incoming form, since the field is hidden, and so !value is true and hence the field is set to false?

[ddaddy]

Yes that looks to be the culprit. As its hidden, it must always be false.

[r3wt]

@ddaddy why must it always be false? In some cases you may want to expose things on the frontend of you app to your userd and hide them from administrators. A prime example would be privacy settings on a user record.

[ddaddy]

I agree. I was saying that as its hidden, the code must be seeing it as false.

[jstockwin]

This can now be demonstrated in current master by running the following command:
node test/e2e/server.js --env default --config ./test/e2e/adminUI/nightwatch.json --test test/e2e/adminUI/tests/group999FixMe/2946.js

When this issue is fixed, please move 2946.js to .../group007Misc/somethingSensible.js, and also change 'demonstrate issue 2946': function(browser) in this file to something more sensible. If you can see a way to make these tests more generic, or add additional testing, then please go ahead.

[jstockwin]

(Which I guess also means this issue is present in v0.4 as well as 0.3)

[JedWatson]

This is a real trick. Not checking a checkbox removes it from the form, as far as FormData is concerned. Which means we have to treat a non present boolean value as a false one. This is an exception in terms of how we handle fields because of that.

I think the right solution is to render a <input type="hidden" /> for hidden boolean fields, so their values are persisted when the form data is processed...

[jstockwin]

Yeah, I think that's what I was trying to say earlier.
I agree that this is probably the best solution, but it does come with some caveats.

Currently, if I have a hidden field, it is completely hidden to my users. I can potentially save sensitive information to a hidden field, and they'd never be able to view it (not that I can actually think of a reason for wanting to...). Adding it into the form as a hidden input type means they can work out the value by viewing the page source. Given it's a Boolean, I guess this is rarely going to be an issue, but it's something to be aware of.

[JedWatson]

The hidden option wasn't really intended for security, as such... but you're right, it doesn't mean others may not use it as one :)

Given our focus on 0.4 and that whatever the fix is won't come forward anyway, I'm just going to hack a fix for this (for boolean fields specifically) into 0.3 now

[jstockwin]

Makes sense. This is currently an issue in 0.4 too though? Or is there a re-write coming that's going to change it all anyway?

[JedWatson]

We're going to have to deal with it in 0.4 too but the code is completely rewritten. I'm tempted to change the spec for boolean fields so that they don't get updated by default if not provided (which is more API friendly, but breaks processing FormData). Will have to think about how not to cause people upgrade headaches though.

[jstockwin]

The whole problem is that when a boolean is set to false it's not provided in the form though. Unticking it in the form causes it to not be provided. We can't just not update it in that case. But yeah, agree with the general idea :)

[JedWatson]

Since the Admin UI is all react talking to an API we can submit a value now - it's not a problem (for us) anymore. 0.3 actually submits an HTML <form>.

Legacy use of the UpdateHandler would break with the change I'm proposing but we can work out a way around that. I'll open another issue for discussion when I'm ready.

[JedWatson]

I fixed this, not sure why it didn't auto-close the issue from my commit message. May be because it was in a branch, not master.