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

Add failing test when changing parent and unloading original parent #5571

Merged
merged 1 commit into from Apr 20, 2019
Merged

Add failing test when changing parent and unloading original parent #5571

merged 1 commit into from Apr 20, 2019

Conversation

lsg-braymon
Copy link
Contributor

@lsg-braymon lsg-braymon commented Aug 17, 2018

Bug wherein changing the parent of a child then unloading the original parent sets the child association to null.
Setup:

  // This is the association set up
  class Child extends DS.Model {
    @belongsTo('parent')
  }

  class Parent extends DS.Model {
    @hasMany('child')
  }
  // Assume here that `child` is already associated to Parent 1
  const parent1 = this.store.peekRecord('parent', 1)
  const child = this.store.peekRecord('child', 1)

  const parent2 = this.store.peekRecord('parent', 2)
  set(child, 'parent', parent2) // We are not commiting to the database yet at this point

  // Delete Parent 1
  parent1.unloadRecord()
  child.parent // Returns null. Parent 2 is expected

Current work around:

// Assume here that `child` is already associated to Parent 1
  const parent1 = this.store.peekRecord('parent', 1)
  const child = this.store.peekRecord('child', 1)

  const parent2 = this.store.peekRecord('parent', 2)
  const payload = {
    data: {
      type: 'child',
      id: child.id,
      relationships: {
        parent: {
          data: { type: 'parent', id: parent2.id }
        }
      }
    }
  }
  this.store.pushPayload(payload) // Instead of set(child, 'parent', parent2)

  // Delete Parent 1
  parent1.unloadRecord()
  child.parent // Returns Parent 2

@runspired
Copy link
Contributor

At first glance this looks good, will want to poke around at it locally before merging to gain more confidence that this is the correct fix.

@lsg-braymon
Copy link
Contributor Author

@runspired Got it. Thank you.

@lsg-braymon
Copy link
Contributor Author

Hi @runspired just wanted to ask if there are updates on this bug. Thank you!

@runspired
Copy link
Contributor

@lsg-braymon sadly I didn't spend the time to review this yet, thanks for the reminder! Will attach some labels so I don't forget :P

@runspired runspired added the code-review Tracks PRs that require a code-review label Mar 25, 2019
@richard-viney
Copy link

I work with @lsg-braymon and have just rebased this PR against the latest master and added some more detail into the commit message.

Is there anything we can do to help with getting this fix included as part of an upcoming release?

Thanks!

…rent of a child record

Fixes an issue where changing the parent of a child record then unloading the original parent incorrectly resets the child association to null. See PR #5571.
@runspired runspired merged commit 88b37e2 into emberjs:master Apr 20, 2019
@richard-viney
Copy link

Thanks heaps, is this change a candidate for inclusion in the current 3.10 betas?

@runspired
Copy link
Contributor

runspired commented Apr 20, 2019

@richard-viney yes, but I haven’t gotten publishing to work correctly with the monorepo just yet. Need to tweak the tsconfig some more so that prepublish works. If you have a moment to help on that this weekend I’d love it cause I don’t have much time ❤️

TL;DR npm prebublish command needs to work correctly in each individual package and is broken in a few atm.

@richard-viney
Copy link

Sure thing, I'll take a look later today

@runspired
Copy link
Contributor

@richard-viney the beta branch has some work towards it to bring back to master. Trying to release last night was an adventure 😂

@runspired
Copy link
Contributor

@richard-viney ended up with a little time to poke at it and think I resolved the issues :) Thank you for the offer, if you do have some time and want to contribute I left some notes in the fix-pr of some things we need to still do to the prepublish infra prior to our next stable release.

#6050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review Tracks PRs that require a code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants