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

Fixed many2many empty relationships update case #112

Closed
wants to merge 2 commits into from

Conversation

carlesnunez
Copy link

The next PR fix a bug related with the update of relationships that doen't have the data. For example:

If we have a model called books that have alot of authors and:

  • We create books with an array of authors but we don't create the authors at the same time
  • We update the books with the same authors id's but keeping without have the authors created

Then the update function tries to do .add with id's that are not linked without any record and throw an error.

I propose the fix that follow in that PR

@carlesnunez
Copy link
Author

@tommikaikkonen any thought about that?

@tiii
Copy link
Collaborator

tiii commented Mar 27, 2017

I did not try this, but just from the naming of the variables I would expect that you no longer can update a relation from no entities to some entities?

I submitted a fix for that about 2 months ago, which also includes some tests for the bug: #86

… ID and we want to update instead of create again
@carlesnunez
Copy link
Author

What do u mean when you say "No entities to some entities?"

I am using this on a heavy app and it's working really well.

@constantx
Copy link

I think i'm running into this issue too.

@carlesnunez
Copy link
Author

I have a forked package with this fixed for testing called redux-orm-kerad until that pr is merged or someone brings light. Maybe it will help u

@tiii
Copy link
Collaborator

tiii commented May 2, 2017

@carlesnunez thanks for getting back to me on this!

I created a test case to better express what I mean:

it('updating related many-to-many entities from none to some works', () => {
    const { Book, Genre } = session;
    const book = Book.create({
        name: 'My new Book',
        genres: [],
    });
    expect(book.genres.toRefArray().map(row => row.id))
        .to.deep.equal([]);

    const addGenre = Genre.withId(2);

    book.update({ genres: [addGenre] });
    expect(book.genres.toRefArray().map(row => row.id))
        .to.deep.equal([2]);
});

With your changes this fails with:

1) Integration Immutable session updating related many-to-many entities from none to some works:

      AssertionError: expected [] to deeply equal [ 2 ]
      + expected - actual

      -[]
      +[
      +  2
      +]

The newly created book has no genres at first. If you try to add some later on the change will be discarded since currentIds.length > 0 will always return false.

I think that #86 fixed the error we are describing and this is now merged into master. Please let me now if I got you right on this!

Considering that you opened another PR (#124) based on this functionality, maybe this PR here could be closed?

@carlesnunez
Copy link
Author

Hello! Close that issue nad i'll check the new redux-orm version. Is it as a new npm version too?

@tiii
Copy link
Collaborator

tiii commented May 8, 2017

Unfortunately there is no new npm release yet. @tommikaikkonen any plans on releasing a new rc?

@tiii tiii closed this May 8, 2017
@NathanBWaters
Copy link
Collaborator

Hi @tiii
We'll be releasing a new npm rc tonight with #86 in it!

@NathanBWaters
Copy link
Collaborator

Released. 👍

@NE-SmallTown
Copy link

I notice that the the releases panel doesn't update,what happened?

@NathanBWaters
Copy link
Collaborator

@NE-SmallTown hmm great note. There's a step I must have missed. I'll fix that up. 👍
@tommikaikkonen probably should have taken you up on your step-by-step offer on how you do releases. :) I'll get that from you on Thursday

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.

None yet

5 participants