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

Update for activegraph #20

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Update for activegraph #20

wants to merge 12 commits into from

Conversation

hng
Copy link

@hng hng commented Apr 8, 2021

WIP: I've started updating the gem to use the activegraph gem. This worked out fine so far. All tests are green.

However I had and still have some problems with integration into a rails app, I hope I may get some help here:

  1. Uploading via a create route did not work (update/edit worked fine), which was caused by the uploader getting set to nil everytime the entity with the mount was initialized. I've removed the after_initialize callback and now this works.
  2. I still have this problem: When creating an entity via a form and that forms file_field has no file selected for upload a SanitizedFile object is still created and the path without the filename (as it doesn't exist) is saved as the url of the upload. The model has a path to directory saved than as the upload. This causes problems, e.g. when editing and uploading a new file an error occurs because carrierwave tries to delete the directory.

Some other questions:

  • Should the gem be renamed to carrierwave-activegraph (devise-activegraph was renamed). I haven't done this yet as I think the maintainers should decide on that.
  • The version depending code in uploader_converter.rb may not be needed anymore? I have just changed it so that it works.

Would be great to get this forward! This would close #19

@hng hng marked this pull request as draft April 8, 2021 13:04
@hng
Copy link
Author

hng commented Apr 11, 2021

I think I have fixed problem 2). The force_retrieve_#{column} callback gave back a store path even if the identifier (file) was nil (i.e. when no file was uploaded) this resulted in the store path without a filename being returned, which caused multiple errors. I've fixed it by checking #{column}_identifier for nil before retrieving from the store.

@hng hng marked this pull request as ready for review April 12, 2021 19:01
@klobuczek
Copy link
Member

@hng Great!. Let's GitHub actions as CI so we have here automated builds. You can look up how activegraph is set up for actions if you need to. And yes we should rename the gem. Once you do that I can rename the github project as well.

@hng
Copy link
Author

hng commented Apr 19, 2021

I've done the renaming. I've also added the activegraph github action for tests.

One test is failing and it is connected to the orce_retrieve_#{column} problem I've described above. We need to figure out why this test was added and I've it can be deleted or whether we need to fix something.

@hng hng changed the title WIP: Update for activegraph Update for activegraph May 12, 2021
@hng
Copy link
Author

hng commented May 12, 2021

I think this should be ready to merge now. As the previous version didn't seem to work (at least not for me) there is probably not a lot to loose.

Some caveats:

  • CI tests currently only run on ruby and jruby-head as there is a problem with the deep_merge method in jruby 9.2.14: NoMethodError for Concurrent::Hash#deep_merge! jruby/jruby#6547
  • I had to change the way force_retrieve_#{column} works, which now causes update_column to half work (when doing a find after using update_column), before it did not work even after getting the node fresh from the database.

TODO for maintainers:

  • rename gem to carrierwave-activegraph
  • create a new release/version?

@weedySeaDragon
Copy link

Any action on this? Would love to use it.

@hng
Copy link
Author

hng commented Jun 20, 2022

@weedySeaDragon I'd love to help getting this merged. Pinging @klobuczek

We've been using my fork for a year now and had no problems so far.

@weedySeaDragon
Copy link

I forked your fork @hng . I just needed a version of the gem for ruby 3+ and neo4j 4+.

FYI I had to change the DatabaseCleaner w.r.t. the constraints and indexes to deal with neo4j 4+ so that the tests would pass.
The changes I made clearly broke the tests for earlier versions of neo4j. Since that's not what I need, I didn't do anything about it, but obv would need to so it handles all versions.

I added a wee bit of text to the README - mostly to have things documented for myself. :-)

Glad to know it's been working well. I now have it incorporated into a site I'm just starting, so haven't yet had the chance to put it to use. Will before long, though.

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.

Update for ActiveGraph
3 participants