No slugify of multiselect answers #181
Conversation
| if answer.body == "[]": | ||
| pass | ||
| elif "[" in answer.body and "]" in answer.body: |
There was a problem hiding this comment.
@Pierre-Sassoulas I don't fully understand what the idea was here. It looks to me like a "]" and a "[" anywhere in the answer.body would cause this to be initiated for example ("Subtitled [English]"). Maybe that's why you decided to slugify? To make sure there wouldn't be any "[" nor "]" in there?
There was a problem hiding this comment.
However, slugifying means that unique choices can be come none unique. If choices are something like "<40" and ">40" then both are translated into "40"
There was a problem hiding this comment.
Possibly, I remember this was a bit hacky, we're serializing / deserializing a list of answers here.
| unformated_choices = answer.body[1:-1].strip() | ||
| for unformated_choice in unformated_choices.split(settings.CHOICES_SEPARATOR): | ||
| choice = unformated_choice.split("'")[1] | ||
| initial.append(slugify(choice)) |
There was a problem hiding this comment.
Here I was wondering why the contents of answer.body are not just JSON or some other standardized method. That way one doesn't run risks of this mechanism breaking as one can fall back to existing json deserialization methods.
It's not quite json as of now because it's using single quotation marks rather than double quotation marks. But it's already quite close.
My code below doesn't change that but I think it's slightly more robust in some cases.
There was a problem hiding this comment.
This is a small sql database, so I suppose I wanted to avoid dumping a json.
There was a problem hiding this comment.
A standardized method (possibily json) would indeed be better than this custom parsing.
There was a problem hiding this comment.
@Pierre-Sassoulas Ok, but it wouldn't take up any more space in the database. Instead of
['one', 'two', 'three']
in JSON that would simple be:
["one", "two", "three"]
An advantage would be that it would automatically be able to read things like this as well:
["'one'", "t,w,o", "[three]"]
I think my code below can do most of that as well though. So maybe it's not needed.
I spent a day last week cleaning up a ~90k row dump turning everything back from the slugified version to the original options because loaddata wouldn't accept the data before realizing that I needed to override this code as well to actually make the original data work. That's the reason I raise this.
There was a problem hiding this comment.
Sounds like it's going to be hard to make a migration from slugified to the new value (as there is less information in slugified answer than the actual answers) so putting that in a breaking change for version 2.0.0 and removing all the migrations files seems in order, what do you think ?
There was a problem hiding this comment.
Or maybe create a "best effort" migration file ?
There was a problem hiding this comment.
It should be possible to make a data migration file in which one takes all the options, converts them using slugify and that way make a pretty good guess as to which of the original options is the right one. Two cases where I don't think that's possible:
- If the options have changed after some respondents answered. The slugified version will either correspond to none of the current choices or may even correspond to an incorrect one.
- Cases where two or more options turn into the same slugified value.
So yeah, probably major version change and some way to gracefully handle the two above situations telling users that "sorry, some data was lost and we can no longer figure out what your respondents actually answered".
Remove slugify.
This PR builds on top of #180 and removes the slugification of answers to multiselect questions. Without this applied, it's not possible to dumpdata/loaddata of pre-existing user data.