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 for #659 #661

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Open

Conversation

uwej711
Copy link
Contributor

@uwej711 uwej711 commented Sep 18, 2015

so there a two conditions to make that test fail

  1. add an preUpdate Listener to the parent class
  2. make sure the parents ChildrenCollection is initialized
    without either one of these conditions the test passes!

@uwej711
Copy link
Contributor Author

uwej711 commented Sep 21, 2015

when we restrict preUpdate to fields only, there remains one failing test (see travis). Not sure, if this is the way to go. But I see no other simple solution for #659. I think it would be safer anyway to restrict the possible changes to a documents fields as there a no other changesets computed after the event. But that currently leaves us with no way to reset the order of children (maybe this should be allowed).

@lsmith77 lsmith77 added review and removed wip/poc labels Sep 23, 2015
@uwej711
Copy link
Contributor Author

uwej711 commented Sep 30, 2015

I think this needs some discussion ...

@dbu
Copy link
Member

dbu commented Oct 9, 2015

events during the flush seem to be a really tricky beast. do you see a way to allow field updates and reorderings, but no renaming? or could we store the renaming operation in a way that the second changeset calculation can check if something was renamed and not get confused?

could we look at object identity rather than paths? uuid would only be something for 2.0 as we would change everything to have a uuid.

@uwej711
Copy link
Contributor Author

uwej711 commented Oct 9, 2015

I can try to look into the reordering but I fear this will not be an easy thing to fix.

@dbu
Copy link
Member

dbu commented Oct 10, 2015 via email

@uwej711
Copy link
Contributor Author

uwej711 commented Oct 11, 2015

That's already done here, with the one test about reverting the reordering of the children in preUpdate failing, i.e. that would be a bc break of some kind.

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

Successfully merging this pull request may close these issues.

3 participants