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

Validations #238

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

Conversation

sbagroy986
Copy link
Contributor

MIME magic number file validations and Client-side form validation added

@@ -0,0 +1,2 @@
#Place all constants here
ACCEPTED_FILE_FORMATS = ["image/jpeg","image/png","image/svg+xml","image/gif"]
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this should be in config/initializers/0_glitterconfig.rb along with the rest of the application configuration options.

@sbagroy986
Copy link
Contributor Author

Done.

@@ -42,3 +42,5 @@
elsif Rails.env.production?
Glitter::Application.config.repo_dir="ENV['OPENSHIFT_DATA_DIR']/data"
end

ACCEPTED_FILE_FORMATS = ["image/jpeg","image/png","image/svg+xml","image/gif"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I wasn't completely clear earlier - I meant that the list of supported formats should be part of the Glitter application config hash - Glitter::Application.config.accepted_formats = [..]

@sbagroy986
Copy link
Contributor Author

Any other changes?

@EmilyDirsh
Copy link
Contributor

Looks good! Thanks! Just need to add some failing tests for this. If you need help with test writing, message @rohitpaulk - he can point you in the right direction.

@sarupbanskota
Copy link
Contributor

@sbagroy986 could you also please point me to a deployment I can try it out on?

AFAIK, many of the form elements are done in a semantic way, so for example if you enter an invalid email, newer browsers will detect it and tell you. Showing another set of errors along with it might be distracting, so I'm interested in checking out how you've dealt with it :)

@EmilyDirsh
Copy link
Contributor

@sbagroy986 could you fix the merge conflict and also, if you have a demo up somewhere for us to try out the changes, that would be very helpful! Thanks!

@sarupbanskota
Copy link
Contributor

could you pull from master and resolve any conflicts?

@sbagroy986
Copy link
Contributor Author

sorry i haven't been active lately. i'll try to make the required changes over the coming weekend!

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

Successfully merging this pull request may close these issues.

3 participants