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

validate agent option json on form submit #1434

Merged
merged 2 commits into from
Apr 26, 2016

Conversation

Enfop
Copy link
Contributor

@Enfop Enfop commented Apr 17, 2016

Hi
when i tried to save invalid json for the agent_options my changes got lost. I realized that i could validate my json by using the editor toggle.
The PR adds the same behaviour for the form submit if the agent_options texarea is present.

@cantino
Copy link
Member

cantino commented Apr 19, 2016

Great idea @Enfop!

There appears to be a bug on Agents that use our form builder, though. Try it with the Basecamp agent. I think it binds the events too early.

Maybe this would work better?

  $('form.agent-form').submit (e) ->
    if $('textarea#agent_options').length
      try
        JSON.parse $('#agent_options').val()
      catch err
        e.preventDefault()
        alert 'Sorry, there appears to be an error in your JSON input. Please fix it before continuing.'

@Enfop
Copy link
Contributor Author

Enfop commented Apr 20, 2016

Uhh sorry about that. Didn't checked the agents which are form_configurable. Your suggestion worked great and fixed the issue. If it's ok i will add some feature specs to ensure the right behaviour.

@cantino
Copy link
Member

cantino commented Apr 21, 2016

That would be great!

@Enfop
Copy link
Contributor Author

Enfop commented Apr 23, 2016

It seems that Poltergeist doesn't support checks for javascript alerts
teampoltergeist/poltergeist#50.
There is this workaround
https://gist.github.com/michaelglass/8610317

It works but i'm not sure if there is a better solution.

@cantino
Copy link
Member

cantino commented Apr 23, 2016

It may not be worth trying to feature test that the alerts show up. What do you think? Is it worth it?

I'd like us to bring in JS unit testing at some point -- Jasmine or Mocha, perhaps -- which would test this more easily.

@dsander
Copy link
Collaborator

dsander commented Apr 23, 2016

I think it's ok to include the workaround, development and test dependencies do not concern me very much. We use alert popups in a few views, this can come in helpful when we add feature tests for them as well.

@cantino
Copy link
Member

cantino commented Apr 24, 2016

That's reasonable. @Enfop, can you merge master back into your PR?

@Enfop
Copy link
Contributor Author

Enfop commented Apr 24, 2016

Sure.

check for textarea#agent_options after form submit
@Enfop Enfop force-pushed the validate_agent_options_json branch from b7d016b to 66fe418 Compare April 24, 2016 23:17
trailing new lines

fix specs
@Enfop Enfop force-pushed the validate_agent_options_json branch from 66fe418 to ada5b02 Compare April 25, 2016 00:04
@Enfop
Copy link
Contributor Author

Enfop commented Apr 25, 2016

not sure why travis build is failing. Local tests work fine.

@dsander
Copy link
Collaborator

dsander commented Apr 25, 2016

That was just a random failure, looks good to me.

@cantino cantino merged commit 9d2626d into huginn:master Apr 26, 2016
@cantino
Copy link
Member

cantino commented Apr 26, 2016

Looks great, thanks @Enfop!

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