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 support for File attachment #24

Merged
merged 1 commit into from
Apr 4, 2014

Conversation

djcp
Copy link
Contributor

@djcp djcp commented Mar 28, 2014

Implemented during a pair session with @seanpdoyle, this adds the ability for Formulaic to attach files to a form when given a File object.

@calebhearth
Copy link
Owner

Looks great.

@calebhearth
Copy link
Owner

Does this work in a js environment? I feel like there might be issues with that.


form.fill

expect(input(:user, :avatar).value).to eq file.path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebthompson no, we didn't test in a js: true feature

We did however, out of curiosity, do some manual testing in chrome. According to jQuery, the contents of $("#user_avatar").val() is a filename string.

Having said that, @djcp and I noticed that during file-attachment, Chrome does some sandboxing magic. It translates the path of an attached file to something along the lines of C:\faked\<filename>, which could have some interesting side effects in capybara-webkit.

It could possibly cause the current version of tests to fail, since file.path could be translated to a path using the sandboxed file system.

@djcp any thoughts?

@djcp
Copy link
Contributor Author

djcp commented Mar 29, 2014

We bound this to the File class, thinking we'd be less likely to run into sync issues related to threads. You'd be more encouraged to use a persisted file as your fixture rather than a tempfile or IO handle that would probably work in the same thread but not across a JS browser thread. Pretty sure this will work OK as designed, but there's probably some additional checks we could implement to, say, ensure it's reading a file that's been persisted to storage.

Do you have a demo project of some sort that exercises the formulaic gem, similar to http://github.com/thoughtbot/paperclip_demo ?

@calebhearth
Copy link
Owner

I'm happy with this. Merge at will.

Implemented by @seanpdoyle and @djcp while pairing, this adds support
for file attachments. Files should be persisted to the filesystem to
avoid problems related to threading.
@djcp djcp merged commit f4e4bae into calebhearth:master Apr 4, 2014
@seanpdoyle
Copy link
Contributor

👍

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.

3 participants