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

problem with response and ajax #163

Closed
jimblue opened this issue Jul 1, 2017 · 31 comments
Closed

problem with response and ajax #163

jimblue opened this issue Jul 1, 2017 · 31 comments
Assignees

Comments

@jimblue
Copy link
Contributor

jimblue commented Jul 1, 2017

Hi,

I'm working on an Ajax form that use your plugin and I'm having some issues.

I've follow the Grav tutorial for the base and it works great. The form is processed and the response message is properly append to <div class="contact-response"></div>.

As the basic worked I tried to make some test with js to understand how your plugin works.
The objective was to catch the status and the message from the server for every case.

Here is the script I use:

$(document).ready(function(){

let contactForm = $('#contact')

contactForm.on('submit', function (event) {
    event.preventDefault()
    $.ajax({
      url: contactForm.attr('action'),
      type: contactForm.attr('method'),
      data: contactForm.serialize(),
      dataType: 'html',
      success: (message, status) => {
        console.log('ajax success:\n\n- status: ' + status + '\n\n- message: ' + message)
      }
    })
  })
});

To see all cases scenario I've forcethe form to return an error by changing the value of the form-nonce input or by adding a value to honeypot input.

In the end that script reveals two problems:

  • first: the status from response is alway equal to success. The status never return error as it should. (however I see you are handling that in classes/form.php)
    Having the good status return to jswould be very useful to create better User XP

  • second: the message from response looks good on success:

<div class="alert notices green"><p>Thank you . I will contact you shortly.</p></div>
  • but on error it returns the complete html page with the error message append inside {{ page.content }}:
<html>
<body>
// page content
<div class="alert notices red"><p>Oops there was a problem, please check your input and submit the form again.</p></div>
</body>
</html>

Hope everything is clear, let me know if you need more informations.

Thank you

@jimblue jimblue changed the title problem with error handling server side problem response and ajax Jul 1, 2017
@jimblue jimblue changed the title problem response and ajax problem with response and ajax Jul 1, 2017
@jimblue
Copy link
Contributor Author

jimblue commented Jul 1, 2017

I've just make more tests and it appears that with JSON workflow using form.json.twig I've got the same issues. It's even worse as the response message always contain the full html code of the page with the error message append inside {{ page.content }} like describe in first comment

@jimblue
Copy link
Contributor Author

jimblue commented Jul 19, 2017

Hi! Any news about this?

@rhukster
Copy link
Member

Sorry i've been busy with getting Grav 1.3 and Admin 1.5 released. I'll try to take a look at this during the week.

@rhukster rhukster self-assigned this Jul 24, 2017
@rhukster rhukster added the bug label Jul 24, 2017
@jimblue
Copy link
Contributor Author

jimblue commented Jul 24, 2017

Hi @rhukster!
No worries about this, I was just wondering if you missed the post.
Thanks for answer!

@flaviocopes
Copy link
Contributor

@jimblue this PR #172 should handle this problem by returning a 500 error when calling the .json page endpoint. Can you test it?

@rhukster
Copy link
Member

I'm not sure returning 500 error is the correct response. Calling a form with .json extension for submission seems a valid option, just any response should also be returned as json.

@jimblue
Copy link
Contributor Author

jimblue commented Aug 9, 2017

Hi guys, sorry I've been in a hurricane the last ten days.

@flaviocopes this will not return the appropriate error message for each case. Also it will not resolve the bug. (the returned message will still be the full HTML code of the page)

@rhukster yes that's the expected behaviour.

@rhukster
Copy link
Member

rhukster commented Sep 7, 2017

I think i've fixed the issue with the whole HTML page being returned on error. I was able to replicate this and fix it for the HTML version of the Ajax form. However, I'm not sure how to accurately replicate your JS test. Can you provide me with the full page including the the form, and anything custom you've done so i can test it?

Also can you test latest fix???

@jimblue
Copy link
Contributor Author

jimblue commented Sep 9, 2017

Hi @rhukster !

Thanks for the fix, I just tried the last update and it seems to work great for the html message 👍

There is only one thing to improve, actually the json response from the form always return a status of success, even if there is an error.

Here is the form:

---
title: Contact
form:
    name: contact
    template: form-messages
    fields:
        -
            name: firstname
            label: false
            placeholder: 'First Name'
            autofocus: true
            autocomplete: true
            type: text
            validate:
                required: true
        -
            name: lastname
            label: false
            placeholder: 'Last Name'
            autocomplete: true
            type: text
            validate:
                required: true
        -
            name: email
            label: false
            placeholder: Email
            autocomplete: true
            type: email
            validate:
                required: true
        -
            name: phone
            label: false
            placeholder: Phone
            autocomplete: true
            type: tel
        -
            name: message
            label: false
            placeholder: Message
            type: textarea
            validate:
                required: true
        -
            name: honeypot
            type: honeypot
    buttons:
        -
            type: submit
            value: Submit
    process:
        -
            email:
                subject: 'Message from {{ form.value.firstname|e }} {{ form.value.lastname|e }}'
                body: [{ content_type: text/html, body: '{% include ''emails/message_admin_receipt.html.twig'' %}' }, { content_type: text/plain, body: '{% include ''forms/data.txt.twig'' %}', charset: iso-8859-1 }]
        -
            save:
                dateformat: Ymd-His-u
                extension: txt
                body: '{% include ''forms/data.txt.twig'' %}'
        -
            message: 'Thank you {{ form.value.firstname|e }}. I will contact you shortly.'
---

here is the twig template:

{% extends 'partials/base.html.twig' %}

{% block content %}

    {{ page.content }}

    {% include "forms/form.html.twig" %}

    <div class="contact-response"></div>

{% endblock %}

and there is the js to test success, error:

$(document).ready(function(){

  let contactForm = $('#contact')
  let responseOutput = $('.contact-response')

  // FORM SUBMISSION
  contactForm.on('submit', function (event) {
    // PREVENT DEFAULT FORM SUBMISSION
    event.preventDefault()

    // SUBMIT THE FORM VIA AJAX
    $.ajax({
      url: contactForm.attr('action'),
      type: contactForm.attr('method'),
      data: contactForm.serialize(),
      dataType: 'html',
      // ON AJAX SUCCES
      success: (message, status) => {
        responseOutput.html(message)
        console.log(status)

        // IF JSON RETURN 'success' as statut
        if (status === 'success') {
          console.log('this is a SUCCESS !!!')
        }

        // IF JSON RETURN 'error' as statut
        if (status === 'error') {
          console.log('this is an ERROR !!!')
        }
      }
    })
  })
})

Let me know if you need more informations

@rhukster
Copy link
Member

rhukster commented Oct 2, 2017

Ok, here's the problem.. I've implemented a quick solution that basically allows a status code to be set for a form, then in the partials/form-messages.html.twig i can set said code if a message is 'red', ie an error. However, if i set this to 400 as a good error call should be, the browser doesn't render the html response. I think only a 2xx message will render, but that's not really accurate.

I could set it to something a bit ambiguous like 207 which means multi-status, and then you could inspect the code directly in the JS and look for that value. Anyway i'll create a feature, and you guys can take a look.

@rhukster
Copy link
Member

rhukster commented Oct 2, 2017

Please check out the feature branch above.. It should work with standard Grav as long as you have undefined_functions: true in system.yaml.

@rhukster
Copy link
Member

rhukster commented Oct 2, 2017

ajax test-form grav 2017-10-02 17-36-59

@jimblue
Copy link
Contributor Author

jimblue commented Oct 31, 2017

Working well with ajax now!

@jimblue
Copy link
Contributor Author

jimblue commented Oct 31, 2017

@rhukster don't know if you read my comment about having on option to change red class? should I create a feature request?

@rhukster
Copy link
Member

Yah new issue would be good

@Sogl
Copy link
Contributor

Sogl commented Nov 3, 2017

@jimblue @rhukster First of all, thank you guys for deep explanation and examples.

I have the same problem. I use noty to handle my form submit messages and latest Form plugin 2.10.0.

My form code:

form:
    name: send-form
    template: form-messages
    #stop sending twice
    refresh_prevention: true

    fields:
        name:
            label: Name
            placeholder: Your name
            outerclasses: 'six columns'
            type: text
            validate:
                required: true

        mail:
            label: Email
            type: email
            placeholder: my@mail.com
            outerclasses: 'six columns'
            validate:
              rule: email
              required: true

        message:
            label: Message
            placeholder: Small message
            outerclasses: 'twelve columns'
            type: textarea
            validate:
                required: true

        honeypot:
            type: honeypot

    buttons:
        submit:
            type: submit
            value: Send

    process:
        message: 'Thank you!'

Here is my js code:

var form = $('#send-form');
   form.submit(function(e) {
       // prevent form submission
       e.preventDefault();

       // submit the form via Ajax
       $.ajax({
           url: form.attr('action'),
           type: form.attr('method'),
           dataType: 'html',
           data: form.serialize(),
           success: function(result, status) {

                console.log(result);
                console.log(status);

               new Noty({
                   theme: 'metroui',
                   layout: 'topCenter',
                   type: 'success',
                   text: result,
                   timeout: 4000
               }).show();
           }
       });
   });

And I see success status in my console when I fill honeypot field (Are you a bot? message) and when I try to re-submit my form (This form has already been submitted. message).

Just changed form plugin to ajax-response-codes branch and faced the same problem 😞

Is this problem still in development?

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

Hi @Sogl !

The ajax-response-codes branch is working well, the problem come from your js code.

I think you just miss understand what success mean inside jQuery ajax function.

It's just to verify the connection to the target server is a success. That doesn't mean Grav return a success. That's two different things.

In other world if Ajax connection is success you'll have access to the Ajax response.

Inside this response (coming from Grav) you can found:

  • the returned status
  • the returned code
  • the returned message

What @rhukster did in ajax-response-codes branch is to return an appropriate code that you can test.

Here is a testing script that should help:

  var form = $('#contact');
  var responseWrapper = $('.contact-response');

  // FORM SUBMISSION
  form.on('submit', function (event) {
    // PREVENT DEFAULT FORM SUBMISSION
    event.preventDefault();

    // SUBMIT THE FORM VIA AJAX
    $.ajax({
      type: form.attr('method'),
      url: form.attr('action'),
      data: form.serialize(),
      dataType: 'html',
      success: function success(data, textStatus, jqXHR) {
        console.log('AJAX SUCCESS (connected to target server):');
        console.log(' - status: ' + textStatus);
        console.log(' - code: ' + jqXHR.status);
        console.log(' - data: ' + $.trim(data));

        if (jqXHR.status === 200) {
          console.log('\n=> GRAV FORM SUCCESS (form is submited)');
        }

        if (jqXHR.status === 207) {
          console.log('\n=> GRAV FORM ERROR (form is not submited)');
        }

        responseWrapper.html(data);
      },
      error: function error(jqXHR, textStatus, errorThrown) {
        console.log('AJAX ERROR (not connected to target server):');
        console.log(' - status: ' + textStatus);
        console.log(' - code: ' + jqXHR.status);
        console.log(' - data: ' + $.trim(errorThrown));
      }
    });
  });

That said you're write the new branch doesn't return the good status if there is an error.
@rhukster can you correct that so people could test the ajax response with the returned code or the returned status?

Thank you 😄

@rhukster
Copy link
Member

rhukster commented Nov 3, 2017

@jimblue code block didn't come through...

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

@rhukster just updated... resfresh!

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

@rhukster, after thinking a lot about it... I'm sorry but returning a code of 207 is a mistake.
The return HTTP status code must be 200 as the request is a success at this point. (https://en.wikipedia.org/wiki/List_of_HTTP_status_codes)

Knowing that it's definitively better to guess if Grav form return an error or a success from the status not the code.

To make it work in the ajax-response-codes branch the status must return accurately error or success and the code must always return 200.

Ajax verification will be more accurate with this markup.

Here is a script to make testing easier (open your browser console):

var form = $('#contact');

// FORM SUBMISSION
form.on('submit', function (event) {
  // PREVENT DEFAULT FORM SUBMISSION
  event.preventDefault();

  // SUBMIT THE FORM VIA AJAX
  $.ajax({
    type: form.attr('method'),
    url: form.attr('action'),
    data: form.serialize(),
    dataType: 'html',
    success: function success(data, textStatus, jqXHR) {
      console.log('AJAX SUCCESS (connected to target server):');
      console.log(' - status: ' + textStatus);
      console.log(' - code: ' + jqXHR.status);
      console.log(' - data: ' + $.trim(data));

      // CHECK IF THE RESQUEST IS A SUCCESS
      if (jqXHR.status >= 200 && jqXHR.status < 400) {
        console.log('\n=> REQUEST SUCCESS (reached target server and form is submited)');

        // CHECK GRAV FORM RESPONSE STATUS
        if (textStatus === 'success') {
          console.log('\n=> GRAV FORM SUCCESS (form has no error)');
        }

        // CHECK GRAV FORM RESPONSE STATUS
        if (textStatus === 'error') {
          console.log('\n=> GRAV FORM ERROR (form has an error)');
        }
      } else {
        console.log('\n=> RESQUEST ERROR (reached target server but returned an error)');
      }
    },
    error: function error(jqXHR, textStatus, errorThrown) {
      console.log('AJAX ERROR (not connected to target server):');
      console.log(' - status: ' + textStatus);
      console.log(' - code: ' + jqXHR.status);
      console.log(' - data: ' + $.trim(errorThrown));
    }
  });
});

Sorry for all this confusion. It's probably because the markup in PHP / AJAX / HTTP is the same (success, status, error...) but doesn't point to the same things...

@Sogl
Copy link
Contributor

Sogl commented Nov 3, 2017

@jimblue Ok, thanks for the explanation... but now we still can't get error or success message (not code) from Grav? And maybe jqXHR.status must be jqXHR.code because it returns code?

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

@Sogl jqXHR.status is XHR code. (jqXHR.code doesn't exist...)
Yes actually you can't get the return status from Grav.
And even if you could the return status is not accurate for now.
You'll have to wait a bit more for a final fix and a merge.

@rhukster
Copy link
Member

rhukster commented Nov 3, 2017

BTW, i've now merged this branch into develop, so it will be released as part of Form plugin 2.10.0

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

But what do you think about last conclusion?
Don't understand why did you merge it...

@rhukster
Copy link
Member

rhukster commented Nov 3, 2017

sorry had already merged before you had commented (or at least before i read your comment). I will take a look at this before I release.

@rhukster rhukster reopened this Nov 3, 2017
@rhukster rhukster added enhancement and removed bug labels Nov 3, 2017
@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

Ok I'm preparing something for you to make it even more easier if you can wait 30min.

@rhukster
Copy link
Member

rhukster commented Nov 3, 2017

yah, i'm knee deep in some contract work anyway right now..

@Sogl
Copy link
Contributor

Sogl commented Nov 3, 2017

@jimblue So... for me is better to just return status from Grav, I could paste it straight as Noty parameter. For now I did a quick workaround checking output div classes:

var form = $('#send-form');
   form.submit(function(e) {
       // prevent form submission
       e.preventDefault();

       // submit the form via Ajax
       $.ajax({
           url: form.attr('action'),
           type: form.attr('method'),
           dataType: 'html',
           data: form.serialize(),
           success: function(result, status, jqXHR) {

                new Noty({
                   theme: 'metroui',
                   layout: 'topCenter',
                   text: result,
                   timeout: 5000
               }).show();

               if ($(result).hasClass("green")) {
                   //close modal window
                   $.modal.close();
               }
           }
       });
   });

And some sass magic:

.noty_body {
    padding: 0 !important;
    color: #fff;
    .notices {
        padding: 1.25em;
        &.green {
            background: #66d181;
        }
        &.red {
            background: #f64b39;
        }
    }
}

p.s. And I found why form plugin didn't return codes. I used old form-messages.html.twig from my template without new branch changes 😄

@jimblue
Copy link
Contributor Author

jimblue commented Nov 3, 2017

Ok I've been testing this all around and I've found a definitive fix.

@rhukster please follow this method in Grav to make AJAX work for everybody once for all:

  • The form response must return JSON data with a custom HTTP status code
    (should have a look to https://www.slimframework.com/docs/objects/response.html#returning-json)
  • The HTTP status code must always be 200
  • The JSON data should contain the response status and the response message
  • the response status should be a string: success or error
  • the response message should only contain the message (no HTML like actually)

–––––––

Exemple in php for response $data:

$data = json_encode(array('status' => 'success', 'message' => 'thanks for contacting us'));

OR

$data = json_encode(array('status' => 'error', 'message' => 'something happened, please try again'));

––––––––

To help you with this, I've just written a script full of comment and console.log.
You should use google chrome console to have colourful debugging.

var form = $('#contact');

// FORM SUBMISSION
form.on('submit', function (event) {
  // PREVENT DEFAULT FORM SUBMISSION
  event.preventDefault();

  // SUBMIT THE FORM VIA AJAX
  $.ajax({
    type: form.attr('method'),
    url: form.attr('action') + '.json',
    data: form.serialize(),
    dataType: 'json',

    // AJAX REQUEST => TRANSFER COMPLETE
    success: function success(gravResponse, ajaxStatus, xhr) {
      var httpCode = xhr.status;

      console.log('%c AJAX SUCCESS (The transfer is finished):', 'color: #bada55');

      // RESPONSE HTTP CODE => SUCCESS
      if (httpCode >= 200 && httpCode < 400) {

        // THE STATUS RETURNED IN GRAV RESPONSE (success or error)
        var gravResponseStatus = gravResponse.status;

        // THE MESSAGE RETURNED IN GRAV RESPONSE (idealy just the message without html)
        var gravResponseMessage = gravResponse.message;

        console.log('%c \t=> HTTP CODE SUCCESS (' + httpCode + ')', 'color: #bada55');

        // IF GRAV FORM RESPONSE STATUS IS SUCCESS
        if (gravResponseStatus === 'success') {
          console.log('%c \t\t=> GRAV FORM SUCCESS (form has no error)', 'color: #bada55');
          console.log('\t\t\t=> GRAV FORM STATUS (' + gravResponseCode + ')');
          console.log('\t\t\t=> GRAV FORM MESSAGE (' + gravResponseMessage + ')');
        }

        // IF GRAV FORM RESPONSE STATUS IS ERROR
        if (gravResponseStatus === 'error') {
          console.log('%c \t\t=> GRAV FORM ERROR (form has an error)', 'color: #da5555');
          console.log('\t\t\t=> GRAV FORM STATUS (' + gravResponseCode + ')');
          console.log('\t\t\t=> GRAV FORM MESSAGE (' + gravResponseMessage + ')');
        }
        // RESPONSE HTTP CODE => ERROR
      } else {
        console.log('%c \t=> HTTP CODE ERROR (' + httpCode + ')', 'color: #da5555');
      }
    },

    // AJAX REQUEST => TRANSFER FAILED
    error: function error(xhr, ajaxStatus, errorThrown) {
      console.log('%c AJAX ERROR (An error happened while file was transfering)', 'color: #da5555');
    }
  });
});

@jimblue
Copy link
Contributor Author

jimblue commented Nov 4, 2017

Hi @rhukster

Sorry forgot this yesterday...
Here is the form coming with the testing script above:

---
title: Home
form:
    name: contact
    template: form-messages
    fields:
        -
            name: name
            label: false
            placeholder: 'Name'
            default: 'Bob'
            autofocus: true
            autocomplete: true
            type: text
            validate:
                required: true
        -
            name: honeypot
            type: honeypot
    buttons:
        -
            type: submit
            value: Submit
    process:
        -
            message: 'Thank you {{ form.value.name|e }}. I will contact you shortly.'
---

@jimblue
Copy link
Contributor Author

jimblue commented Dec 2, 2017

Main response Ajax problem fixed.

But still need to correct the returned status of the response in #209 #213

@jimblue jimblue closed this as completed Dec 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants