search

ylecuyer / survey-gizmo-ruby

Gem to use the Survey Gizmo API
MIT License
23 stars 26 forks source link

Fix escaping to resolve duplicate option issue #75

Closed kjhenner closed 8 years ago

kjhenner commented 8 years ago

Survey Gizmo's API returns a key containing a literal \" for 'other' options, so the regex to match this needs to be \\\" to properly escape the escape character.

apurvis commented 8 years ago

Hm my only issue would be that i'm not sure it always returns that literal \?

I'm going to check my stuff though you may very well be right about this and it might have always been a bug in which case great catch

apurvis commented 8 years ago

👍 nice catch; checked my output and it is indeed like this.

cc: @jarthod maybe worth pushing a release here as this is a nice bugfix.

apurvis commented 8 years ago

(also if you could bump the minor version number on this branch that would be great)

jarthod commented 8 years ago

Looks good to me

jarthod commented 8 years ago

released 6.2.0

apurvis commented 8 years ago

Wait a minute - I installed the gem and tried to run my import and promptly crashed out with this:

ruby/2.1.0/gems/survey-gizmo-ruby-6.2.0/lib/survey_gizmo/api/answer.rb:32:in `initialize': Can't recognize pattern for [question(5), option("10017-other")] => test - you may have to parse your answers manually. (RuntimeError)

so... this might not actually be a valid fix, even though it seemed like it. @kjhenner -

  1. what version of the API are you hitting? v4 or v5?
  2. what version of the Virtus gem is installed in your app? I have 1.05
kjhenner commented 8 years ago

@apurvis I have 1.05 of virtus, but I'm hitting v4 of the API. Seems like that could be the issue. I'll have a look.

Edit: Oh, never mind, just looked back at the readme and v4 is supposed to be what's supported.

apurvis commented 8 years ago

yeah i am also hitting v4. it might be the case that SG is inconsistent... while I'm not 100% sure i'm not getting dupes, 100% of my responses (tens of millions of rows) work with the way it was before and now nothing does.

what version of faraday and faraday middleware are you using? i have faraday 0.9.2 and faraday_middleware 0.10.0

kjhenner commented 8 years ago

@apurvis Yeah, I have the same versions of those as well. Could you try doing an API call with a different tool and see what the raw JSON looks like? I'm definitely still getting "[question(33), option(\"10125-other\")]", but I might try some of our older or newer surveys to see if there's any chance they changed the format or something.

Edit: Well it seems consistent with everything I'm looking at. No idea why it's keeping those literal escape characters in for me, but I get the feeling it's something that will make me feel dumb when I figure it out.

apurvis commented 8 years ago

i actually did that already and I saw the \" in the responses, which is why i 👍 this PR.

so what i am thinking (and why i'm asking about virtus and faraday middleware) is that at some point in the stack, that string is getting evaled or some such, resulting in a string like [question(5), option("10017-other")] instead of [question(5), option(\"10017-other\")]

apurvis commented 8 years ago

what version of ruby are you using? i'm on 2.1.6

apurvis commented 8 years ago

also FWIW the fact that this didn't crash out for you before with the Can't recognize pattern for [question(5), option("10017-other")] error means that the old regex was passing whatever responses you were getting.

Seems strange/unlikely that both the old and the new regex will pass the output you are getting from SG? any ideas w/r/t how that could be possible?

kjhenner commented 8 years ago

I was using 2.0.0, but I just tried 2.1.6 with the same result.

And yeah, that's a good point. I'll dig around and see if I can track down where that key is/isn't matching.