Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added failing test #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added failing test #18

wants to merge 2 commits into from

Conversation

Leooo
Copy link

@Leooo Leooo commented May 26, 2017

Added test for #17

@danielspaniel
Copy link
Owner

danielspaniel commented May 30, 2017

I was looking at your test.
It seems like you are wanting to:

model.set(prop, 'A')
// YOU: please pretend that this is saved without having to save the model so it does not show up in changed attributes anymore
// ME: um .. err .. you realize this is an illegal procedure, in fact it is so illegal a procedure that ember data has NO PUBLIC METHOD to allow it.
// YOU: ok .. fine .. I will use private method to set a dirty state to clean and wipe out the changedAttributes.
// ME: uh .. why you doing this? me confused. there is a way to do it and i would have to make a special method in change tracker to make it happen. but realize that this is not a normal situation and is extra credit. so you going to have to explain to me why you need it, or figure out how to do the PR yourself and convince me to add it. or suffer with private method hacking .. or rethink what you are doing because it might actually be not the greatest idea in the first place.
// YOU: well .. I appreciate the use of the term "illegal procedure", reminds me of the good old days in medical school. But see here Dan, we really must get to the bottom of this. I thought your method model.saveChanges() was supposed to save the changes. Is this not so?
// ME: Well .. now that is a fine point my good man Lionel. But change tracker 'saveChanges' is different than your idea of save changes! Change tracker uses that method to save the current dirty state to its private tracking store. So that method is like 'rememberChanges'.

@Leooo
Copy link
Author

Leooo commented May 30, 2017

My use case is that after saving an appointment model, my API doesn't properly returns back one of the attributes (the status), so I need to set this status attribute myself. Then I'm using the savedChanges method so that the attribute is not flagged as changed (thought that's what this method was about no?). In the appointment booking route, when user leaves the journey before confirming, I'm rolling back the appointment model and its relationship, and I don't want the status to be set back to a wrong value.

Let me know if you want me to make a PR with the code change.

Thanks

@danielspaniel
Copy link
Owner

That is the point I was trying to make Lionel. The model.saveChanges does NOT save the changes to ember-data model .. it saves the changes to tracker.
To accomplish what you want to achieve you could setup some code in the serializer to alter the payload ( add the status flag ).
I am not going to accept a PR that does what you want to achieve if that is to save changes to model without calling save because as I said before .. that is an illegal procedure.
But I can try and help you figure out an alternative ( more proper solution ) .. which the serializer idea is ( sort of )

@Leooo
Copy link
Author

Leooo commented May 30, 2017

yes I can add it in the serializer.

I don't want to save my model as it would send another request, just want to save the changes to tracker. If you think a PR is not the right thing to do then forget it. But it feels like the saveChanges name method is very confusing then, as I suppose most people would expect it to save the changes to tracker and reinit the changed flags (which it does for relationships and other kind of attributes), while it doesn't.

@danielspaniel
Copy link
Owner

danielspaniel commented May 30, 2017

you are very right about that Lionel, that name is 100% confusing. I need to change it from 'saveChanges' to => resetTrackerState ( or something like that.) because it only saves the current changes to tracker. It does not clear the changed attributes for the model. It does not reinit relationships or any attributes as you are thinking )

  /**
   * Save change tracker attributes
   *
   * @param {DS.Model} model
   */
  static saveChanges(model) {
    let metaInfo = this.metaInfo(model);
    Object.keys(metaInfo).forEach((key) => {
      Tracker.saveKey(model, key);  // saves changes to tracker ( and that is all it does ) 
    });
  }

@danielspaniel
Copy link
Owner

v0.7.4 adds the name saveTrackerChanges to the model

so model.saveChanges() is now => model.saveTrackerChanges()

not sure if this helps but let me know if it does and i will close this PR

@Leooo
Copy link
Author

Leooo commented Aug 29, 2018

it doesn't, did you try adding the test to master?

@danielspaniel
Copy link
Owner

I did not add that test @Leooo , I think I am still confused as to what that test is supposed to show.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants