Skip to content
This repository has been archived by the owner on Dec 5, 2018. It is now read-only.

parameter validation for title, text_top, and text_bottom in app/models/... #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josephzang
Copy link

...captioned_image.rb

@jrunning
Copy link
Contributor

jrunning commented Nov 7, 2013

I don't understand this PR. Firstly, its implementation allows only single characters:

irb> expr = %r{\A[A-Za-z0-9 _-?!]\Z}
irb> expr.match("hello")
nil
irb> expr.match("h")
0

Secondly, why restrict input at all? What if a Russian or Mexican or French Canadian animal shelter wants to use the app? What about a Chinese one? What if a user wants to put a ☺ in their caption?

If anything we might validate to only accept printable characters, but otherwise I don't see a reason to restrict what characters users can put in their images.

@benjaminoakes
Copy link
Contributor

Thanks for the contribution @josephzang. @jrunning has a point, it probably doesn't make sense to restrict input too much. However, if the API we're using doesn't work with certain character ranges, it could make sense.

@josephzang
Copy link
Author

Oh jeez. Sorry about that. I can't even run it in irb -- the ?! and space break it. I forgot the asterisk at the end to let it match zero or many characters, and I didn't consider the language thing. I obviously didn't test it, but that's partly because I wasn't able to get the heroku-cedar box to boot in vagrant, even after deleting and redownloading the file from dropbox a few times. Do either of you have any ideas?

@benjaminoakes
Copy link
Contributor

The Vagrant box isn't entirely necessary, just maybe easier in a "repeatable" kind of way. It's known not to work that well with Windows.

If you can get Ruby, etc running on your machine, that can work just fine as well. I'd look into rvm.

@josephzang
Copy link
Author

I'm on Ubuntu 12.04 and had vagrant running the precise32 box in the docs, but oh well I guess. I do have an environment set up with RVM, but I don't have PostgreSQL installed. I suppose I'll do that then -- vagrant looked really cool is all.

@benjaminoakes
Copy link
Contributor

It should work fine on Ubuntu 12.04... that's what I'm running. The instructions in the README are exactly what I ran to test it. Note that you might need a newer version of Vagrant. The one in the Ubuntu repositories for 12.04 is ancient.

I can maybe walk you through it sometime too, if you'd like. :)

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

Successfully merging this pull request may close these issues.

3 participants