Error when emptying phonetic variation field

[Woseseltops]

[susanodd]

I found the error and I can fix this. It was sluiced through to be set to None rather than the empty string.

I'm looking for other "fall through" cases that could cause problems. Seeing as we want to avoid explicit lists of field names in the code, In addition to 'phonetVar' the same code also happens with 'mouthG', 'mouthing', and 'phonOth'.

But of these, 'phonOth" doesn't give an error because null=True is set in the model.

phonOth = models.TextField(_("Phonology Other"), null=True, blank=True) mouthG = models.CharField(_("Mouth Gesture"), max_length=50, blank=True) mouthing = models.CharField(_("Mouthing"), max_length=50, blank=True) phonetVar = models.CharField(_("Phonetic Variation"), max_length=50, blank=True,)

I suppose the best way to solve this is to add "null=True" to all of these fields. Except the fields without null=True also differ on type. (CharField instead of TextField)

Does anybody recall why the types were chosen for these fields? An empty character string isn't really a None value.

What about the Frequencies section? It seems the Edit also allows these to be edited.

I'll go ahead and fix it with code. Then @Woseseltops can see if the type is important.

[Woseseltops]

Ah okay so this is about the difference between a value with no content and no value (once explained to me as: the difference between an empty bus and no bus :) ). In the case of a field like phonetVar, this would mean the difference between 'no info on phonetic variation for this gloss' and 'no phonetic variation for this gloss'. In the Signbank interface, however, we don't make this distinction... we only show an empty phonetVar.

I would say it's cleanest if we would be consistent in how we indicate this empty phonetVar. Apparently, we have been doing this with empty strings so far, so maybe let's stick to that and don't allow null values? In that case, the solution to this would be to figure how it's possible that emptying the phonetVar in the interface results in the code wanting to suddenly store a null value. What's your opinion @susanodd ?

[susanodd]

I encountered something with these fields in the CSV import/export routinges. The four "text" fields under phonology, the CSV update has to compare a potentially new value to the existing value and it was not recognising that it was the same.

[susanodd]

This bug has been fixed in the previous commit. I just wanted to double check with @Woseseltops.

The default fall-through code had set fields to None if the user had erased something or chosen nothing. The revised code avoids setting the fields to None and instead sets them to empty string.

I changed the code to do this so that a migration is not needed. (e.g. An alternative solution would be to add null=True to the other three fields, then the default code that sets to None would work.)

(The downside is that now there is a hard-coded list to catch these fieldnames to avoid the general case.)

[Woseseltops]

The revised code avoids setting the fields to None and instead sets them to empty string.

Sound good @susanodd ! In which commits did you change this? I had a look but I can't find them.

[susanodd]

This is the new test update.py (line 365):

            # special value of 'notset' or -1 means remove the value
            if (value == 'notset' or value == -1 or value == '') 
                        and field not in ['phonetVar', 'mouthG', 'mouthing', 'phonOth']:
                gloss.__setattr__(field, None)
                gloss.save()
                newvalue = ''

If the field is one of phonetVar...phonOth. that code (which sets the field to None) is not executed. Instead the else clause goes (line 372), whereby the first part is skipped (since field is not idgloss). So the code here is executed (line 383-384):

                setattr(gloss,field,value)
                gloss.save()

Here, the value can be the empty string for the phonetVar, mouthG, mouthing, phonOth.

What was causing the error before was that the code above that sets the field to None was being done. But the fields are not allowed to be Null.

[Woseseltops]

Okay thanks @susanodd , looks good.

Now we're improving things, maybe this part ['phonetVar', 'mouthG', 'mouthing', 'phonOth'] can also be made dynamic? Maybe replace it with something along the lines of [f.name for f in Gloss._meta.get_fields() if not f.null] ?

[susanodd]

Okay, that looks much cleaner.

[susanodd]

The code has been revised as you suggested, @Woseseltops. Testing based on model fields has the side-effect of catching other errors that people hadn't noticed yet. Namely trying to empty the same fields in Morpheme Detail Viiew, and the fields useInstr and iconImg.

[susanodd]

This has been fixed, @Woseseltops.

[Woseseltops]

Looks good.