A record in the invalid state becoming valid keeps the same transaction

[cyril-sf]

This is fixing the issue "unable to reuse a transaction on an invalid record"

[Fixes #457]

[cyril-sf]

@tomdale @wycats

Hey guys, any chance someone can take a look at this PR and let me know if that's right or what you would expect otherwise?

Thanks

cc @dgeb

[wagenet]

@cyril-sf it looks like this has some merge conflicts. Can you rebase on master?

[cyril-sf]

Sure.

[cyril-sf]

I didn't get any conflicts, weird. Did I miss something?

[dgeb]

@wagenet I didn't get any conflicts either ... could you please verify?

[wagenet]

That's odd, it says it can be merged now.

[cyril-sf]

It's not odd, it's wonderful!

[cyril-sf]

@wagenet @dgeb

Any feedback on this PR?

[cyril-sf]

After a discussion with @wycats and @tomdale, this isn't the right approach. I will add a new bucket for erred records that can be retried within the same transaction.

[sapientpants]

@cyril-sf Is there anything I can help with on this?

I've been trying a number of approaches to be able to correct and resubmit after a server validation failure and have yet to find anything that works.

[cyril-sf]

@Pansapien thanks for the help. I'm finishing another PR right now and should work on fixing this one before the end of the week. Hopefully the code will be ready next week and will be good enough to be accepted. But my company really needs this too, so I won't give up.

[cyril-sf]

Let's summarize where I am with this:

1. A record in the invalid state is in the inflight bucket.
2. When one of its property is modified, didSetProperty will transition the record in the uncommitted state. With 2 effects:
   • When exiting the invalid state, we move the record from the inflight bucket to the clean one and we attach the record to the default transaction ( recordBecameClean )
   • When entering the uncommitted state, we move the record from the clean bucket to the dirtyType one and mark the record as dirty, preventing it from being moved to another transaction.

My solution was to move the record to the clean bucket when exiting the invalid state without modifying the transaction. It works in my case.

@wycats and @tomdale suggested to create a new bucket for erred records that could be retried (my understanding was that retry should be different from commit and take care only of erred records). I tried this approach by putting the record in an erred bucket when entering the invalid state and not doing anything on exit. We enter the uncommitted state from 2 different states:

• we followed the happy path and the record is in the clean bucket
• something went wrong and the record is in the erred bucket.
  which prevents us from using removeFromBucket (because we expect the record to be in the clean bucket.

I'm not sure to know how do you want to solve this problem.

[SweeD]

Is there anything new on this one? 😏

[ulisesrmzroche]

This is a feature that is very sorely needed. In the meantime, whats the alternative? How do I handle this scenario in my application?

[bobbus]

👍 for a way to solve this in current master

[seanrucker]

Desperately need a fix for this. @cyril-sf anything I can do to help?

[dmonagle]

Hi guys, I have a workaround that I have been using with success. It isn't (can't be!) the best way of doing things going foward. But I have created a mixin for these purposes.

Gist: https://gist.github.com/intrica/4773420

The original SO question that I created in order to solve this:

http://stackoverflow.com/questions/14741377/what-can-you-do-with-ember-data-models-when-in-the-error-state

[cyril-sf]

@seanrucker Sorry, I have been working on another PR for a while. I still plan to work on it once I'm done with the polymorphic associations.

[wulftone]

This is a pretty big one! Users often submit invalid data. I'm just learning ember-data, but quickly encountered this issue. Thanks, @intrica, for the gist! That was very informative.

[wulftone]

I have a super hacky fix for this in the meantime:

App.MyModel = DS.Model.extend({
  becameInvalid: function(model){
    model.send('becameValid');
  }
});

..at least for the Rails returning 422 because of validation errors issue.

Yeah....awesome! : P

[balinterdi]

I don't see why the record has to be taken out of its transaction upon the invalid -> uncommitted transition. In my case it causes a mess since the record thus gets added to the default transaction and when I submit the form (save gets called on the adapter) it submits all records in it.

@cyril-sf 's above commit seems all that's needed. Instead of calling recordBecameClean which removes the record from the transaction, just call recordIsMoving which just adds the record to the clean bucket.

I'm also curious to see what does putting once-invalid records to a separate bucket (called erred) buys us. Why not just put it into the created/updated one? I guess an equivalent question is how is retry different then commit?

[justinfaulkner]

@cyril-sf's commit helped fix my problem at http://stackoverflow.com/questions/15823481/ember-data-transactions-and-resubmitting-forms

[tomdale]

@cyril-sf What's the status of this?

[cyril-sf]

@tomdale I'm going to work on that.

If anyone else wants to work on it, I'd be happy to help. But this is a big issue and someone needs to tackle it.

[dev-inigmas]

@wulftone Thanks for sharing your hacky solution, btw. It really helped me move on from this situation quickly.

[balinterdi]

I'd happy to lend a hand but first I'd have to understand what's the desired behavior and the reasons behind it (see my comments above).

[ybart]

@cyril-sf What are the remaining tasks needed for the PR to become ready to be merged ? You previously wrote you were not sure about how the team wanted it to be done, is there an update on this specific point ?

[cyril-sf]

@balinterdi agreed.

@ybart This PR may be to specific, there are other areas that would need a robust error handling. Another frequent use case, is when you get a 500. You end up with a "zombie" record. The goal would be to list those use cases and design a solution that will lead us to fix them all, even if it is separated in several PR.

So the first step is really to list the error use cases.

[Emerson]

New to ember-data and ran into this today. Would be nice to see this merged, even if it's not the final solution (as mentioned in the above comment).

Update Thanks @wulftone for your hacky fix. Worked perfectly for me.

[sly7-7]

@cyril-sf could you check if this is resolved ?

[cyril-sf]

@sly7-7 The test that I have written doesn't pass in master with that commit.

I'll take a look at this code durig the weekend. Hopefully something good will come out of it.

[sly7-7]

This would be great, thanks a lot :)

[tchak]

@cyril-sf I think this is fixed by transactions refactoring. Could you confirm that?

[cyril-sf]

@tchak Confim! The record stayed attached to the transaction and can be attached to another one! Thanks!

[abobwhite]

@wulftone Your hack worked for me as well! Dang! I've been searching for a workaround for two days. How is it possible that this was overlooked? Obviously you need server side validation. According to the code, ember data expects a 422 for validation errors. But regardless whether it's a 422 or 500, the model is hosed, albeit in different ways. It should be easy to try to commit again. model.send('becameValid') goes against the grain in so many ways...a model shouldn't be valid if it's technically invalid. Plain a simple: a model in an invalid state should be able to commit.