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.

Sign up for GitHub

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

Jump to bottom

_get_changed_fields fix for embedded documents with id field. #925

Merged
merged 1 commit into from Apr 9, 2015
Merged

_get_changed_fields fix for embedded documents with id field. #925

merged 1 commit into from Apr 9, 2015

Conversation

elephanter
Copy link
Contributor

When id field presented in Embedded document, changes inside this document was ignored in method _get_changed_fields because it's id was added to inspected array before processing it fields.
Also modify unittests for _delta method, which fails without code modification.

Review on Reviewable

@landscape-bot
Copy link

Code Health
Repository health increased by 0.14% when pulling 58b51dd on elephanter:fix__get_changed_fields into cbd2a44 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

6 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 58b51dd on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

5 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 378c3f1 on elephanter:fix__get_changed_fields into * on MongoEngine:master*.

@MRigal
Copy link
Member

MRigal commented Apr 3, 2015

I also had this problem, but got used to it, always first setting .id to None after copying a document for example.

But I think it is not such a great behaviour and your fix is welcome.

It may be highlighted somewhere in the documentation, since it could surprise some users...

@landscape-bot
Copy link

Code Health
Repository health increased by 0.14% when pulling 378c3f1 on elephanter:fix__get_changed_fields into cbd2a44 on MongoEngine:master.

@thedrow
Copy link
Contributor

thedrow commented Apr 8, 2015

@MRigal Care to issue a PR with the documentation change before I merge this?

@MRigal
Copy link
Member

MRigal commented Apr 8, 2015

I think actually it should be a bit more than that. It's for example not clear which behaviour will happen when modifying the id.

I think it should create a new document, but it could trick some user that may think it would update the document. I don't know it is so cool to make it silently, and in any case, I would add specific test that "documents" the behaviour and make sure it won't change.

@MRigal
Copy link
Member

MRigal commented Apr 8, 2015

This proposal may actually take a completely new interest / scale when looking at it from the new difference in PyMongo 3.0 between update_one() and replace_one(). We may have a bit more to do here.

I'm fine to make a PR for the doc, but I would at least like some comment on my previous comment

@elephanter
Copy link
Contributor Author

@MRigal I don't understand. What do you mean with "should create a new document"?
This bug affects only embedded document where can be id field (in my case with type StringField).

@MRigal
Copy link
Member

MRigal commented Apr 8, 2015

Sorry @elephanter , I should not review PR just based on Github's source code extract, but sometimes looking at the whole file...
I thought you were generally modifying the way looking at id fields, without seeing it was just for embedded document.

Then, it is perfect, good job, and IMHO mergeable ! 👍

@thedrow
Copy link
Contributor

thedrow commented Apr 8, 2015

@MRigal I activated Reviewable on this repository a while ago.
So this is good to merge?

@MRigal
Copy link
Member

MRigal commented Apr 8, 2015

Yes, I should have used it ! I will pay more attention. For me, it is good to merge!

@thedrow
Copy link
Contributor

thedrow commented Apr 8, 2015

@DavidBord @yograterol @rozza Any objections?

@yograterol
Copy link
Contributor

@thedrow I restarted the travis failed job, if built this PR will can merge.

@thedrow
Copy link
Contributor

thedrow commented Apr 8, 2015

@yograterol We have some build problems. That job always fails.

@yograterol
Copy link
Contributor

@thedrow yep, but I say about this job https://travis-ci.org/MongoEngine/mongoengine/jobs/56994446

@yograterol
Copy link
Contributor

done @thedrow 👍

@DavidBord
Copy link
Contributor

@thedrow seems legit, but only after adding the change to changelog.rst, the author to AUTHORS and squashing the commits

@thedrow
Copy link
Contributor

thedrow commented Apr 9, 2015

@elephanter See @DavidBord's comment.

@elephanter
Copy link
Contributor Author

@thedrow Ok, sorry ), it's my first pull request)

@landscape-bot
Copy link

Code Health
Repository health increased by 0.14% when pulling d45b59e on elephanter:fix__get_changed_fields into cbd2a44 on MongoEngine:master.

@thedrow
Copy link
Contributor

thedrow commented Apr 9, 2015

No problem but now there are manual conflicts to resolve.
If you can squash all the commits into one commit and rebase against the current master that would be lovely.

@elephanter
_get_changed_fields fix for embedded documents with id field.
f77f45b 
removed commented out piece of code

added author and record to changelog
@elephanter
Copy link
Contributor Author

@thedrow ok, guess I did it right )

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@thedrow
Copy link
Contributor

thedrow commented Apr 9, 2015

Let's see what Travis says :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

7 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 73.04% when pulling f77f45b on elephanter:fix__get_changed_fields into 103a287 on MongoEngine:master.

thedrow added a commit that referenced this pull request Apr 9, 2015
@thedrow
Merge pull request #925 from elephanter/fix__get_changed_fields
8fea2b0 
_get_changed_fields fix for embedded documents with id field.
@thedrow thedrow merged commit 8fea2b0 into MongoEngine:master Apr 9, 2015
@thedrow
Copy link
Contributor

thedrow commented Apr 9, 2015

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@elephanter @landscape-bot @coveralls @MRigal @thedrow @yograterol @DavidBord