Thank you for reporting this bug. We will make it our priority to review this report.
  1. #1
    Sencha User Dumas's Avatar
    Join Date
    Dec 2008
    Location
    Vienna, Austria
    Posts
    581
    Vote Rating
    9
    Dumas will become famous soon enough

      0  

    Default Ext.data.Store::getNewRecords() behaves strange

    Ext.data.Store::getNewRecords() behaves strange


    REQUIRED INFORMATION


    Ext version tested:
    • Ext 4.0.0
    • Ext 4.0.2a

    Browser versions tested against:
    • Chrome 12

    Description:
    • The documentation of Ext.data.Store::getNewRecords() says: Returns all Model instances that are either currently a phantom (e.g. have no id), or have an ID but have not yet been saved on this Store
    • So I expect that when I add a just created record to the store, what I get an array with one element, instead of none.

    Steps to reproduce the problem:
    • Go to http://dev.sencha.com/deploy/ext-4.0...er/writer.html
    • Run following code in the console:
      var store = Ext.ComponentManager.get('writergrid-1028').getStore(); // maybe id has to be changed
      store.insert(0,Ext.create("Writer.Person"));
      console.info(store.getAt(0).phantom+"--"+store.getNewRecords().length);
    • Now you see in the console "true--0" instead of "true--1"

    The result that was expected:
    • store.getNewRecords() return a newly added element

    The result that occurs instead:
    • returned an empty array

    Test Case:
    see Steps to reproduce the problem:



    Best regards,
    Roland

  2. #2
    Sencha User
    Join Date
    Mar 2011
    Location
    Germany
    Posts
    198
    Vote Rating
    1
    Nickname is on a distinguished road

      0  

    Default


    Hi,

    this is not a bug, well at least with the test case you provided.
    I'll try to explain why.

    Ext.data.Store::getNewRecords() uses the filterBy method of Ext.util.MixedStore to determine the new records.

    Code:
    // in Ext.data.Strore
        getNewRecords: function() {
             return this.data.filterBy(this.filterNew).items;
        },
    
    // in Ext.data.AbstractStore
        filterNew: function(item) {
            return item.phantom === true && item.isValid();
        },
    In the writer example you have validations in the model and thats the point, why the "filterNew" method returns false for the inserted record and the record is not returned as "new". The item.isValid() fails.

    Code:
    Ext.define('Writer.Person', {
        extend: 'Ext.data.Model',
        fields: [{
            name: 'id',
            type: 'int',
            useNull: true
        }, 'email', 'first', 'last'],
    // all the validations are not true if use your testing code
        validations: [{
            type: 'length',
            field: 'email',
            min: 1
        }, {
            type: 'length',
            field: 'first',
            min: 1
        }, {
            type: 'length',
            field: 'last',
            min: 1
        }]
    });
    You can very that with the same steps you wrote to reproduce the problem plus run
    Code:
    store.getAt(0).isValid()
    Hopefully its enough explanation. If you have other similar problems, please provide a test case.
    I was struggling with the write and syncing of grids and had this problem too (well, store.sync() does not have an error reporting, but thats a reported different part)

  3. #3
    Sencha User Dumas's Avatar
    Join Date
    Dec 2008
    Location
    Vienna, Austria
    Posts
    581
    Vote Rating
    9
    Dumas will become famous soon enough

      0  

    Default


    Thanks for the explanation! That makes sense.

    I still think it's a bug, cause it doesn't behave like it claims to... it's "wrong behaviour is just intended, so it's a bug in the docu. The docu should be change to something like:
    Returns all _valid_ Model instances that are either currently a phantom (e.g. have no id), or have an ID but have not yet been saved on this Store...

    So when I want to test for invalid records before store.sync() to give the user a nice error message I have use store.each!?

    thx
    Roland

  4. #4
    Sencha User
    Join Date
    Mar 2011
    Location
    Germany
    Posts
    198
    Vote Rating
    1
    Nickname is on a distinguished road

      0  

    Default


    Yes you can use .each or do the same as the getNewRecords() method does.

    The following snippet returns all new records that are not valid

    Code:
    var store = Ext.ComponentManager.get('writergrid-1028').getStore();
    store.insert(0,Ext.create("Writer.Person"));
    
    var invalid_new_records = store.data.filterBy(function(item) {
        return item.phantom === true && !item.isValid()
    });
    Or you can setup a listener, that checks / shows the valid status of the record.

    In my MVC App I use the following for grids with roweditor

    In the controller I add a listener on click event of the "update" button of the rowEditor editor

    Code:
        /**
         * Fires when the users clicks "update" in the RowEditor to add/edit a group
         * 
         * @param {Object} event
         */
        onSaveGroupAction: function(event) {
            var store = event.store,
                record = event.record,
                errors = record.phantom === true ? record.validate('add') : record.validate();
    
            if(errors.isValid()) {
                record.endEdit();
                store.sync();
            }  else {
                Ext.Msg.show({
                    title:'Error',
                    msg: errors.getRange().join("\n"),
                    buttons: Ext.Msg.OK,
                    icon: Ext.Msg.ERROR
                });
            }
        },
    As you can see, there is a conditional statement, that calls validate() with the parameter 'add'.
    This is a customization in my model definition. I think its proper documentated

    Code:
    Ext.define('Webdesktop.model.administration.Group', {
        extend: 'Ext.data.Model',
        idProperty: 'id',
        fields: [
            {name: 'id', type: 'int'},
            {name: 'name', type: 'string'},
            {name: 'description', type: 'string'},
            {name: 'memberscount', type: 'int'}
        ],
        validations: [
            {type: 'presence', field: 'id'},
            {type: 'presence', field: 'name'},
            {type: 'presence', field: 'description'}
        ],
        /**
         * Override to have special handling for new records
         *
         * If we add new records and want to validate, the validation
         * fails because one model validation rule is: id must be present
         *
         * Adding this custom validate method, we can check if there is
         * an add action and remove all errors for the id field from Ext.data.Error
         * and return the object with real errors for the add process
         *
         * @override
         */
        validate: function(type) {
            var errors = this.callParent();
            if(type === 'add') {
                // onAdd we have no "id" field. remove errors for this field
                errors.removeAll(errors.getByField('id'))
            }
            return errors;
        },
        /**
         * proper call to validate on new records
         * used in groups store filternew() method on syncing the store to backend
         */
        isValidNew: function() {
            return this.validate('add').isValid();
        }
    });
    I want to refactor this in the future to get rid of the string "add" and use a model constant.
    But that does not change the behaviour.

  5. #5
    Sencha User Dumas's Avatar
    Join Date
    Dec 2008
    Location
    Vienna, Austria
    Posts
    581
    Vote Rating
    9
    Dumas will become famous soon enough

      0  

    Default


    thx

  6. #6
    Sencha User
    Join Date
    Mar 2010
    Posts
    9
    Vote Rating
    0
    buford is on a distinguished road

      0  

    Default


    Quote Originally Posted by Nickname View Post
    Hi,
    this is not a bug,
    No, this is clearly a bug. When you call Store.sync() the filters you mention are indeed executed, however, they are not performing the function they are named as. getNewRecords() does not mean "get new records except those that are invalid." It means "get records that are dirty and/or phantom." So this is a bug because it behaves incorrectly both as named and per the documentation.

    getNewRecords() and its pal getModifiedRecords() are also bugged because they ignore errors presented to them from the store's proxy. The proxy is telling these functions that there are errors but instead of doing something logical and meaningful with that information (like, can you throw a guy an exception once in a while??) these functions lose the error information by calling isValid().

    The end result of the way these functions are coded winds up being that Store.sync() is worthless because no error information is ever returned to the developer. I see that other folks have written "you can do this" or (worse) "you can do the same thing as getNewRecords()." That's really a red flag about this portion of the framework if people have to add application layer code to cover up for discarded error information deeper in the framework.

    After a little more reflection, I'd say that Store.sync() is worse than worthless - it's dangerous to your data. Sync() fails silently and leaves the store in an unknown/unexpected state from the application's point of view. It's failure will cause user confusion and possible loss of data that the user *thinks* is saved.
    Last edited by buford; 4 Aug 2011 at 6:55 PM. Reason: Added a little cheese to my whine.

  7. #7
    Sencha User
    Join Date
    Mar 2011
    Location
    Germany
    Posts
    198
    Vote Rating
    1
    Nickname is on a distinguished road

      0  

    Default


    Please if you quote, please quote correctly
    this is not a bug, well at least with the test case you provided.
    In this testcase it is correct that getNewRecords() does not return the new record, because the new record is not valid.
    Yes, valid records, because I think, that only valid records should be in the store. In the testcase we want to push an invalid record directly into the store, but in real examples you have forms/grids etcetera that should visualize the record validation and prevent invalid records.


    It is a framework design problem (not a bug) that errors from Store.Sync() are not returned (or even gathered). Yes, I completely agree here.
    Store.sync() is at the moment something, that you call and hope that everything works fine. It is really not as good as many expect, I'm not satisfied too.
    Thats why I follow this thread and try to get a more specific answer on this subject (contacted abe).

  8. #8
    Sencha User Dumas's Avatar
    Join Date
    Dec 2008
    Location
    Vienna, Austria
    Posts
    581
    Vote Rating
    9
    Dumas will become famous soon enough

      0  

    Default


    Quote Originally Posted by Nickname View Post
    In this testcase it is correct that getNewRecords() does not return the new record, because the new record is not valid.
    Buggy means that a function behaves differently then expected... An expectation is build by it's name and it's documentation.
    Both the name and the docu claim that it returns ALL new records. So I too think it's a bug.
    I think the devs should rename the function to getNewValidRecords and fix the docu or change the function.

    Quote Originally Posted by Nickname View Post
    Yes, valid records, because I think, that only valid records should be in the store. In the testcase we want to push an invalid record directly into the store, but in real examples you have forms/grids etcetera that should visualize the record validation and prevent invalid records.
    Well, invalid records should be possible to be in the store as well, let's say you have a grid with an add button, this creates a new record in the grid and makes it editable.... to first add the empty row you need to add a maybe invalid record.

    Yes, Store.sync() is not a bug, it's bad design.

    Best regards,
    Roland

  9. #9
    Sencha User
    Join Date
    Mar 2010
    Posts
    9
    Vote Rating
    0
    buford is on a distinguished road

      0  

    Default


    Quote Originally Posted by Dumas View Post
    Yes, Store.sync() is not a bug, it's bad design.
    Amen to that.

    This really is the problem, I just didn't want to go there last night.

    Where the design goes wrong is that sync() mixes two different logical activities: collecting new and modified records and the validation status of those records.

    My solution to this problem would be to

    1. add a function to Store called getValidationErrors() that collects errors from the Store's models
    2. remove the isValid() check from getModifiedRecords() and getNewRecords()
    3. call getValidationErrors() in Store.sync() and throw an exception if the error count > 0

    This solution would have multiple benefits I think.

  10. #10
    Sencha User
    Join Date
    Mar 2011
    Location
    Germany
    Posts
    198
    Vote Rating
    1
    Nickname is on a distinguished road

      0  

    Default


    Quote Originally Posted by Dumas View Post
    Buggy means that a function behaves differently then expected... An expectation is build by it's name and it's documentation.
    Both the name and the docu claim that it returns ALL new records. So I too think it's a bug.
    I think the devs should rename the function to getNewValidRecords and fix the docu or change the function.
    Okay, thats true. I didn't read the Docs for this method. I'm using more the source to check out what something does

    Quote Originally Posted by Dumas View Post
    Well, invalid records should be possible to be in the store as well, let's say you have a grid with an add button, this creates a new record in the grid and makes it editable.... to first add the empty row you need to add a maybe invalid record.

    Yes, Store.sync() is not a bug, it's bad design.
    Yes that's obviously true, I use this in my code.
    What I meant, was the current store snapshot, that's gonna be synchronized to the backend with store.sync().
    In my implementation I use a RowEditor, insert the new blank row and start editing. But if the user wants to save this new record (save to store, not syncing), the record must be valid. The user can only stop the editing if the new record is valid or he cancels the edit process. If he cancels the edit process, the new temporary inserted row is removed from the store and I have a store status, that is again ready for synchronization. I have to admit, that I build it this way, because of the current implementation of store.sync().
    But I try to use it like forms with formBind. A form cannot be submitted if it is not valid (eg: a new row cannot be finally inserted if it is not valid). If I can submit the form (form is valid or the model behind it) I validate the data on my backend again and return errors if the save process wasn't successful. And this last step, returning errors and handle them, is missing in store.sync(). For the hundreds of possible implementations users will use grid and the grid plugins a comfortable solution must be presented from sencha.
    If you have autoSync enabled, the frontend framework should do this, because it generates unnecessary request to the backend. I think sencha had the thought: Why should I sync elements of the store, when I defined with Ext.data.Model the isValid status of a record and even if I know the record is invalid, sync the store. The only expecting result can be an error from the backend system.

    I think we have the same basic position here and in the thread I mentioned, Sencha was notified and early solutions are discussed. Sadly the feedback on this issue stopped and I'm trying to keep updated for any possible solutions provided with 4.1. You can push this issue by contacting sencha directly and describe, that this should be handled as a blocker for the 4.1 release and for a well designed solution for everyone the development of fixing this bug should be regularly described with example usage.

Thread Participants: 2