search

shrinerb / shrine-mongoid

Mongoid integration for Shrine
http://www.mongoid.org/
MIT License
12 stars 5 forks source link

Embedded records support #3

Closed FunkyloverOne closed 5 years ago

FunkyloverOne commented 5 years ago

[ partly resolves #1 ]

Hey @janko-m,

there are no tests for #swap yet, I was trying to figure out how to properly test it, but then I gave up, and decided to leave it to you.

And you're going to change those methods anyway, to achieve proper "overriding" and "order independency" in plugins inclusion.

And thanks for all your help with this, I probably wouldn't make it alone :)

janko commented 5 years ago

Had time to take a better look at this, and it looks great, very nice work! 👍

I will try to merge this in the next few days and release a new version, then eventually I will fix Shrine so that we can lose the order dependency.

FunkyloverOne commented 5 years ago

Hey @janko-m, I've found a bug with #swap method, it's related to those nested attributes here #4.

If there are few different embedded associations with files, for some strange reason, not all of them could be "uploaded" using my workaround for nested attributes.

Everything works fine if I switch to this commit https://github.com/LinkUpStudioUA/shrine-mongoid/commit/2faf5f66468f1bc72062c526e8654b28d4b1da13, where #swap is not modified. (And backgrounding breaks of course :slightly_smiling_face:)

FunkyloverOne commented 5 years ago

Hey @janko-m,

I just realized that embedded records could be embedded into other embedded record, so embedding depth could be any, and this code currently supports only simplest scenario. So we should also cover those "recursive" embeds.

FunkyloverOne commented 5 years ago

OK, I've figured out what's the problem with #swap method. You see, we have adapted it for backgrounding plugin, and when we do not use backgrounding - there will be no need in reloading, which is breaking everything.

I believe we have to check if backgrounding plugin is included and then use #swap method accordingly. (e.g. reload only when we deal with backgrounding).

FunkyloverOne commented 5 years ago

Added (I believe dirty) check for backgrounding, so now nested attributes are working properly for embedded models, if cascade_callbacks are enabled on association.

janko commented 5 years ago

@FunkyloverOne Thanks for updating. Sorry, I'm still keeping this PR in mind and it will definitely get merged, I just wasn't able to find time yet.

FunkyloverOne commented 5 years ago

@janko-m no problem, it still needs "recursive" embeds support to be implemented, so it's not finished anyway. I don't really have time for it just yet too.

janko commented 5 years ago

@FunkyloverOne I'm impressed with your pull request, and I realized I'm not able to find time to properly review it. So I added you as a collaborator and gave you rubygems.org access, and you're free to merge the PR and push a new gem version if you want.

janko commented 5 years ago

Closing, as these changes won't be needed anymore in Shrine 3.0, see https://github.com/shrinerb/shrine-mongoid/issues/1#issuecomment-531529124.