Moved input existence validations to client (#3352) #3362
Conversation
BrokenEagle
changed the title
app/assets/javascripts/uploads.js
Outdated
| if (($("#upload_file").val() === "") && ($("#upload_source").val() === "")) { | ||
| error_messages.push("Must choose file or specify source"); | ||
| } | ||
| if (!$("#upload_rating_s")[0].checked && !$("#upload_rating_q")[0].checked && !$("#upload_rating_e")[0].checked) { |
There was a problem hiding this comment.
It's possible to specify the rating in the tag string using the rating: metatag. This case would have to be checked too.
app/assets/javascripts/uploads.js
Outdated
| @@ -26,6 +27,27 @@ | |||
| } | |||
| } | |||
|
|
|||
| Danbooru.Upload.initialize_submit = function() { | |||
| $("#submit-button").click(function(e) { | |||
There was a problem hiding this comment.
This won't trigger if you submit the upload by pressing the enter key instead of clicking the submit button.
This could be fixed by binding to the form submit event instead ($("#form").submit(function (e) ...)) then saying e.preventDefault() in the error branch (prevent the submission from going through on error, rather than submitting it on success).
BrokenEagle
force-pushed
the
feat-client-input-validation
branch
from
9bac691
to
38478bc
Compare
BrokenEagle
force-pushed
the
feat-client-input-validation
branch
from
38478bc
to
f91e11b
Compare
|
Made the changes as suggested by evazion. Reverted the submit button back to the way it was, but kept the ID to make it more uniquely addressable, plus left out the |
|
I've rebased and deployed to Testbooru. |
|
I just checked, and I don't see it on Testbooru. |
|
Sorry, I messed up the branch. The correct version should be deployed now. |
|
I ran a series of tests on Testbooru, using uploads 75-82. I somewhat labeled them with tags, but will clarify them below. Before submitting the request for each one, I tested each iteration with both the rating and file/source not present, only one of each present, then both present. Only the ones where both were present went through as expected and created upload records. For all but the last test, I used bogus sources or files to avoid generating too many new posts.
That last one went through successfully and created post #53. After thinking about it, I did one more test with the Javascript off.
The above went through fine, however when testing the various fail conditions, the program wouldn't process the upload as expected as the server-side validations are still in effect, but it also wouldn't give any error conditions when it failed since that was removed from the ERB view. The question brought up by this is, should we support users that don't have Javascript enabled for the site? Or should program behavior always be tailored to the expectation that Javascript is running? |
|
I think it's safe to assume users, particularly ones who upload, will have Javascript enabled. I'll do one final rebase and deploy to Testbooru again. |
|
I tested it again, and the validations still work as expected. There weren't as many upload records though since I started blocking the IP before every series of tests, which works well at testing the Javascript if it doesn't need to query the server and you don't want to actually create a record. |
This adds in the third validation from #3352. As explained on that issue, doing it this way is quicker and more responsive to the user as all the information is already available at the client.
I took out the
error_messages_forsince the view ERB and Javascript code will cover all of those cases, but I left the original validation error pushes in the uploads model because the user will get those if they try to upload from the API. The button is left as an<input>with avalue="Submit"to take advantage of existing CSS that styles that button. Thename="commit"was left out as it's not sent anyways as a result of using the trigger action with "submit", which is done for the Return key anyways so there shouldn't be an issue.