Should pushedData in the uncommitted state should move a record to committed?

[bmac]

Currently it always stays in the uncommitted state. Even if the new attributes match the values for the "changed" attributes.

[bmac]

@pangratz I think I remember you fixing this issue. Is that correct?

[pangratz]

Not completely. The stuff I added in #3868 targeted changed properties being updated when new data has been pushed.

I think this issue still needs to be discussed...

[runspired]

I think if all dirty attributes are updated after we do the new diff, we should transition.

[runspired]

We should check but I believe we now move to committed (at least since Identifiers work landed) for the newly created case.

We likely still need to re-diff

[runspired]

We discussed and we agree we should re-diff and enter clean state if no remaining mutations

[runspired]

With the feature flags on for master I think it is likely we now re-diff and enter the clean state, we need a PR of a test though, and it'll be an easy fix if we do not.

There are two tests for this, (1) for newly created records and (2) for existing records.

import { recordIdentifierFor } from '@ember-data/store';

//...

const user = store.createRecord('user', { firstName: 'chris' });
const identifier = recordIdentifierFor(user);
const pushedUser = store.push({
  data : {
    type: 'user',
    id: '1',
    lid: identifier.lid,
    attributes: { firstName: 'chris' }
  }
});
assert.strictEqual(user, pushedUser, 'we got the same user');
// assert various state flags on the record here.
// assert hasDirtyAttributes is false

For the existing record the test is roughly:

const user = store.push({
  data : {
    type: 'user',
    id: '1',
    attributes: { firstName: 'chris' }
  }
});
user.firstName = 'John';
// assert hasDirtyAttributes is true here
store.push({
  data : {
    type: 'user',
    id: '1',
    attributes: { firstName: 'John' }
  }
});
// assert hasDirtyAttributes is false here

[runspired]

@zinyando this could be a good PR to take on for you, see my comment above.

[robbytx]

I believe this is still an outstanding issue for loaded.created.uncommitted. In this state, even if the exact data is returned from the server (because of a successful save in a prior session), the record will transition unconditionally to loaded.updated.uncommitted, so it will have dirtyType = 'updated' but changedAttributes() will be empty.

Interestingly loaded.updated.uncommitted does move to the saved state if there are no changes upon pushedData, so I'm able to use the following logic in my Store to workaround this gap:

  _load(data) {
    const internalModel = super._load(data);
    if (internalModel.isLoaded() && internalModel.dirtyType() === 'updated' // loaded.updated
      && !internalModel.isSaving() && internalModel.isValid() // uncommitted
      && !internalModel.hasChangedAttributes()) { // no actual changes
      // "remind" the model that we just pushed data, thereby invoking loaded.updated.uncommitted.pushedData,
      // which will see there are no changes and transition to loaded.saved state.
      internalModel.pushedData();
      assert(`Internal model should have transitioned to saved state`, !internalModel.dirtyType());
    }
    return internalModel;
  }

I'd be open to feedback if there are any concerns with this interim approach.

[runspired]

@robbytx the state shown by the state machine is different from the state shown on the model. We left the state machine there while we tested if the derived versions more or less matched. We've now entirely removed the state machine on model. I think it's likely the record will now be in a non-new non-dirty state, but we should check with some new tests.

[runspired]

note I suspect this is at the heart of #7780's actual issue

[robbytx]

Following up from my earlier comment which pertained to 3.20.5, I've re-tested this scenario on 4.4.1, and it appears that this is no longer an issue. Upon data being pushed into the store (in my case via included data from another response), I'm seeing that the record will transition from root.loaded.[created/updated].uncommitted to root.loaded.saved if there are no local differences.

I believe this confirms your beliefs:

With the feature flags on for master I think it is likely we now re-diff and enter the clean state, ...

We've now entirely removed the state machine on model. I think it's likely the record will now be in a non-new non-dirty state, but we should check with some new tests.

I suppose you can consider this a manual confirmation, although I understand the value in having this covered by tests.