Story.saveTiddler preserves the wrong fields when overwriting a tiddler

[SaqImtiaz]

When a tiddler is overwritten, story.saveTiddler incorrectly preserves the fields of the tiddler being discarded rather than those of the tiddler we are saving.

In the following test case, tiddler A has a field called fruit with value banana. However, after renaming A to B, this field is not preserved. Rather the field of the discarded tiddler 'old B' with value orange is preserved:  http://share.lewcid.org/rename%20bug.html

There is a proposed fix in the tiddler called Proposed code, the var extended Fields line is commented out and the replacement line is immediately below it. Reloading the file, running this code via Firebug console and trying the rename again shows that the fields are correctly preserved.

[FND]

Story.prototype.saveTiddler  contains the following line:

var extendedFields = store.tiddlerExists(newTitle) ? store.fetchTiddler(newTitle).fields : /* ... */;

This translates to "use replaced tiddler's extended fields", which is not actually desirable - at least not when renaming (there might be other implications).

[EricShulman]

The patch code that has been submitted is not correct.

When renaming a tiddler so that it replaces another existing tiddler, the user is explicitly asked to confirm if they want to discard the prior tiddler's contents before the processing proceeds any further.

However, once the user has pressed OK, the patched code still wants to fetch the existing fields from the 'newTitle' tiddler, even though the user has indicated that those fields are **supposed to be discarded** along with the rest of that tiddler's previous content and should never be retained when replacing one tiddler with another.

Thus, the only line of code that should have been changed in the patch was the assignment to the extendedFields variable, to completely eliminate any fetch of the fields from 'newTitle', as was shown in the code that Saq posted.

[FND]

Fixed in changeset:9427.

Martin and I spent quite a lot of time figuring this out, making sure our assumptions and conclusions were correct.
Incidentally, this resulted in much simpler code/logic.

Nevertheless, a thorough review of this change would be appreciated to make sure there are no regressions.

[EricShulman]

Replying to FND:

Nevertheless, a thorough review of this change would be appreciated to make sure there are no regressions.

I think you need a 'var extendedFields' in the else clause. Alternatively, this could be written more compactly using:

var extendedFields = store.tiddlerExists(title)?store.fetchTiddler(title).fields:merge({},config.defaultCustomFields);

[FND]

I think you need a 'var extendedFields' in the else clause.

I'm fairly sure JavaScript? needs only one declaration per scope, regardless of whether it is within a conditional or not.
We've only actually tested this in Firebug though, so it is possible that other browsers behave differently - do you have any details on this?

this could be written more compactly using: var extendedFields = store.tiddlerExists(title)?store.fetchTiddler(title).fields:merge({},config.defaultCustomFields);

We intentionally avoided the ternary operator to make the code more readable. (You might have noticed there were nested ternary operators before... )