Checks whether a fieldchoice is in use before deleting it in the admin

[susanodd]

DId Handshape FieldChoice #A get deleted?

[susanodd]

Probably need a safeguard so that if a fieldchoice was deleted that the field gets set to None? (See discussion in Issue #326.)

[ocrasborn]

• Yes, indeed, #A got deleted (as well as all the other ones with prefix # or =).
• Indeed, field should be reset to none or empty in such a case. But only after the user confirms that this may happen: this issue is to create a warning for the user upon deleting something that is in use, and s/he should be given the option of cancelling or proceeding. In the latter case the field should be reset for the relevant records.

[henrinie]

Hey, one question: Why would you delete a FieldChoice? Could this entire issue be solved by not letting anyone delete them?

[vanlummelhuizen]

@henrinie And leave redundant field choices? How to indicate that they are not to be used?

@ocrasborn, I talked to @susanodd about removing all Handshap field choices, since we now have a Handshape class. This will make dealing with handshape less redundant and less error prone.

[ocrasborn]

@henrinie : Field choices are not cast in stone, our insights develop over time.

[henrinie]

It is nice to get some discussion going @ocrasborn @vanlummelhuizen

And leave redundant field choices? How to indicate that they are not to be used?

I think you could for example change the FieldChoice.field to "NotUsedAnymore". What would happen is that you will not lose the choices made with FieldChoice, but it would not show in the choices list.

Field choices are not cast in stone, our insights develop over time.

I totally understand this, I am not actually suggesting that you should not be able to adjust them. Just that it might not be a good idea to delete them. There might exist solutions to this problem that don't require deleting the FieldChoice objects, and it might be good to consider them too.

After looking at how the FieldChoice is used, I am a little confused. I battled with FieldChoice model when I first started working on FinSL-signbank. I changed it to be a ForeignKey for fields it was used for, but you seem to use it as a CharField, which seems fine since you store the reference to machine_value. So if you somehow end up deleting a FieldChoice that was used, you will lose the choices made using it. One advantage for using FieldChoices as ForeignKeys is probably that at least in django-admin you would be protected with a warning when trying to delete a FieldChoice that was used somewhere.

Btw, since the method build_choice_list (https://github.com/Signbank/NGT-signbank/blob/master/signbank/dictionary/models.py#L23) seems to be initiated in the model fields (and evaluated when the application is started), do you have the problem that when you change FieldChoices, you have to reload the application for the changes to take place (restart/reload the webserver directly, or by touch wsgi.py)?

[ocrasborn]

do you have the problem that when you change FieldChoices, you have to reload the application for the changes to take place (restart/reload the webserver directly, or by touch wsgi.py)?

Yes, we do.

[ocrasborn]

See also #340

[Woseseltops]

Current behavior in the admin:

• If a field choice is in use, the delete button is hidden on the field choice detail page.
• If you try to remove a field choice from the field choice list view, you get a nice and pretty error... but the field choice is gone anyway (it's a bit like saying 'by the way, shouldn't have done that').

The last thing could be fixed, I imagine, but I'm not sure if it's worth the effort @ocrasborn ?

[ocrasborn]

Not sure either. I think an alternative function (#487) already gives the system manager something to work with here, especially if we would expand that to some more fields. I'll take away the high priority label, but leave this for someday.

[susanodd]

When working on the Analysis pages, I came across a gloss with a non-existent Handshape. (duim_E, which is a minimal pair of testonno4) In the Phonology and Minimal Pairs list, the non-existent Handshape code is shown in red.

[vanlummelhuizen]

leave this for someday

Today is someday ;) I cannot reproduce the behavior described by @Woseseltops. So, deleting field choices within admin is done without a fail-safe. @ocrasborn change priority?

[susanodd]

This may be influenced by #340.

[susanodd]

I'm working on this.

[susanodd]

This is nearly done. I discovered that something is missing from the migrations for the class Definition. That class relies on a choice list for NoteType. In the class definition code, everything is as you'd expect. But in the (dynamic) _meta information (which we are using to avoid hard coding), Django does not recognize that there is a choice list for Notes.

I inspected the migrations and it looks like the "build" on the choice lists is only in the initial migration. And that initial choice list does not have a numerical first entry.

So in spite of the code working, Django does not recognize the type of the choice list for this class. (Django can't tell that this is a choice list, in spite of it being used as a choice list and having entries in the database corresponding to the choices.)

It looks like a migration was never made for NoteType field choices for this class.

[susanodd]

This issue has been completed. It has been pushed to github.

The has_delete_permission method of FieldChoice of the Django admin now checks whether the field choice is in use. If so, this method returns False and Django does not let you delete it.

To avoid hard coding, this is implemented generically based on meta info from the models for Gloss and Handshape.

There are also field choices used in models Definition, MorphologyDefinition, OtherMedia, and Morpheme. Each has one field that has choices. Django somehow does not recognise that as a choice list. So that code (a one-field lookup dictionary) has been hard coded rather than generated from the model meta data.

I've tested this all by hand, which is a bit tedious. Next up is some unit tests to automate it.

[susanodd]

@Woseseltops, I've implemented your generic choice list comparison based on the _meta information. (Remember? To compare the actual choice lists to see which field has that choice list, based on _meta information.)

For model Handshape, I come across something strange. The meta information choice lists (field choices) are different than the choice lists built from FieldChoice data. E.g., the field JointConfiguration in the _meta choice list from the Handshape model is missing two of the choices. This means that the generic "match" can't be found because the choices for e.g., hsFingConf (meta information choices from Django) does not match JointConfiguration. However, both of these should be identical because field hsFingConf in the Handshape model was built (by Django) using JointConfiguration. (Recall, the build_choice_list is used in the models for fields with choices. We decided to compare the field category -- e.g., FieldChoice category JointConfiguration -- with the build_choice_list built by Django, in order to make the coding generic).

The database has this information stored, as expected.

So the question is, where is Django getting its _meta information from that it's incorrect? It's just weird.

This behaviour was also showing up for model Definition, as mentioned in the previous comment by me (see above). Now I'm implementing the test for Handshape field choice deletion when in use.

[susanodd]

I kind of suspect some migrations aren't complete. But I don't know how to go about confirming and/or solving this.

[susanodd]

I generated a migration and didn't delete the rows specifying choices, so the field choice lists are up to date. The new migration solves the above problems.

[Woseseltops]

Note to myself: I want to verify that the migration by @susanodd won't be a problem for the ASL Signbank

[susanodd]

I finished the unit tests on FieldChoiceAdmin has_delete_permission for all the categories of field choices. This has been pushed to github.

[Woseseltops]

Problem encountered by @susanodd :

• A field choice belongs to a particular field choice category. Gloss fields can have choices only from one category. For example, the field 'domhndsh' has glosses only from the field choice category 'Handshape'.
• When deleting a field choice, we should (1) check the category of this field choice, then (2) check which fields use choices from this category, then (3) check if there are glosses which have a matching value in these fields.
• Problem is that there is no way to (2)

Solution 1 was to use Gloss._meta.fields[x].choices. By comparing the resulting list of choices you should be able to deduce which category this is... except that whatever is in this list is not put there dynamically from the database, but with a migration. This means that there should be a new migration each time the database is changes, which is undesirable.

Proposed new solution 2:

In models.py, add extra info the fields like this:

    handedness = models.CharField(_("Handedness"), blank=True,  null=True, choices=build_choice_list("Handedness"), max_length=5)
    handedness.field_choice_category = 'handedness'

In other places in the code, you can go:

    matching_fields = []
    category_of_interest = 'handedness'

    for field in Gloss._meta.fields:

        try:
            if field.field_choice_category == category_of_interest:
                matching_fields.append(field)
        except AttributeError:
            continue

This way we can at least have all configuration of the field choice category at 1 place in the code.

[Woseseltops]

The solution above is a dynamic solution to know which fields are using a particular field choice category (so you can go from field choice category to a list of fields). An extra advantage of this solution is that you can easily go the other way around (so from a field to which field choice category it is using):

Gloss._meta.get_field('handedness').field_choice_category

[susanodd]

Okay, I'll rewrite the code again to use this solution. Thanks, @Woseseltops.

[susanodd]

Implemented Solution 2. Pushed to github. Unit tests to guard against deletion of in use Field Choices are implemented and successful.

[susanodd]

@Woseseltops I removed the questionable parts of the migration you reminded yourself to look at. I only left the OtherMedia choices because the old values in its domain were weird. You might have a look at some of the choices actually in the Django meta data.

[susanodd]

@Woseseltops, I figured out why tests on my server were working whereas on the live database they did not. I had modified create_test_db.py, but forgot to put it on github. It now clears additional tables that are dependent on glosses, because the glosses are removed.

[susanodd]

@Woseseltops, can you check if this works for you now with the new create_test_db?

[susanodd]

I haven't worked on this issue for a long time. As far as I can tell, it's working okay. But the original issue seems to have evolved into other issues.