Closed halbow closed 3 months ago
Thanks for the effort on this @halbow. I can see you're trying to make a PR that can be accepted but which doesn't impact existing users but I wonder if we shouldn't just make this the default?
I mean, when would you want to pull from RWS and not use UTF-8 encoding if it contains characters not covered by ISO-8859-1 which it almost certainly will because Rave allows unicode input? The unicode=True parameter is weird, like you might only optionally want the full input the user entered?
Anyway, that's a bit of a rant and I appreciate the effort put in on this. @glow-mdsol do you have any thoughts?
@iansparks I think it would make a good default, If you agree I can modify the PR to change the default requests !
For the Audit record data, without the unicode=True
, we were able to decode unicode data in the Query but not the value of the clinical data (the value was replaced by question mark in our case). With this extra argument, we were able to decode everyting correctfully
Thanks @halbow, your flexibility on this is appreciated. Lets see what @glow-mdsol has to say
I agree, I tried to do something like this (albeit much less elegantly) a year or two back. I think your approach should be the default.
Thanks @glow-mdsol . Well that's an endorsement @halbow do you want to go ahead with a more aggressive making-this-the-default PR? Much appreciated!
@iansparks I will do that no problem ! However I noticed that some studies send a 400 Bad request when receiving the query parameter unicode=True ! According to the documentation it's only available after Rave EDC 2019.2.0 but I'm not sure how to check that as the version number is different from what I get with VersionRequest and BuildVersionRequest. I can make the parameter optional in the meantime
Ok, I'll ask around internally to see if I can provide an enumeration for this check.
Ok, it looks like the build version for 2019.2.0 is "5.6.5.607" - if you use the BuildVersionRequest
route and verify the value is after this you should be good. Can you try that?
In my case it's working for 5.6.5.724 but not for 5.6.5.725 I have a IIS Error 400 Bad request when there's the unicode query parameter :confused:
Can you email me with the URLs you are using? I can't see from Rave Classic or RaveEDC why they would differ
Sorry I didn't had much time to do follow-up on this ! In the end it looks like the difference is in the Rave adapter V1 or V2: https://learn.mdsol.com/api/rws/operational-data-model-odm-adapter-95587281.html Do you think there's a way to detect the adapter used or it's better to make this arg optional ?
I'll close this PR as it has been stale for quite some time
Hello :wave:
As discussed in https://github.com/mdsol/rwslib/issues/115 , here's a proposal to improve the default encoding of RWS library based on what I did locally to solve the issue.
Here's a few points of attention:
unicode=True
query parameter for the audit record data to be correctly sent from Rave