Change vocabulary around text answer review

[niklasmohrin]

In #1705, we started by changing the names in the frontend, this renames things in the code and database.

Note: Let's not forget that we must think about logging when doing these kinds of changes. As TextAnswer is not a LoggedModel, we don't need to do anything in this case.

[richardebeling]

changes look straight forward so far.

Regarding "deleted" / "published": To me, the past tense implies this already happened, but I think the meaning is: "On result publishing, the text answers are deleted", so maybe we want "delete" and "publish" instead? ("undecided" is fine).

[niklasmohrin]

I agree, what kept me back so far is that we would need a verb for "private". Possible words include "conceal", "protect" or "classify". @he3lixxx and I are in favor of "protect" and also changing "private" to "protected", what do you think @janno42 ?

[janno42]

”Protect“ doesn't really convey the meaning by itself. While not as nice from the one-word-perspective, I would prefer something like ”make private“. What do you say?

[richardebeling]

We forgot to update the "HI" values in evap/results/fixtures/minimal_test_data_results.json -- which is a bit embarrassing because the PR touched the lines, it just didn't update the values 😅

[niklasmohrin]

Hmm, why didn't anything break? Did we just never find these rows when filtering for specific states, or did Django automatically ignore them?

[richardebeling]

I would have expected django to error out on model validation while running loaddata inside the test. Turns out it doesn't validate model fields (I don't know what else there is to validate on the model -- you'd have to run clean_fields or full_clean to get a validation error, but that doesn't happen during loaddata). get_results_impl filters out any text answers with other states than private or public, so the "critical" text answers are never loaded back from the DB for any result operation. If any textanswer with another state was loaded, our methods would fail assertions.

I was thinking earlier today of adding a CheckConstraint to all models that have fields with choices. We currently have 15 of those. Might be worth it, a few more lines, one migration, I wouldn't expect it to have any noticable performance impact...

[niklasmohrin]

I was thinking earlier today of adding a CheckConstraint to all models that have fields with choices. We currently have 15 of those. Might be worth it, a few more lines, one migration, I wouldn't expect it to have any noticable performance impact...

I guess that would work, but it also sounds a bit unsatisfying and we would have to remember to update constraints when updating models in the future. Maybe we can ask Django people for a fix / new feature?

[richardebeling]

I agree, if django doesn't want to automatically add database constraints, I'd at least like to have a enforce_in_db parameter for the CharField or something that adds the correct check for me. However, I didn't find any such thing when searching for it yesterday.

There have been multiple issues on the django bugtracker about choices not being enforced, usually closed as wontfix or invalid with a note that you can always add checks manually if you want. See their tickets 26131 and 32726. Maybe we should make a new one that proposes the additional parameter for fields.

Anyway, it doesn't seem like we would get this in the near future.

[niklasmohrin]

Okay, maybe CheckConstraint is not a bad idea in the mean time then. I would be interested to see if we can come up with something usable, ideally something where we don't need to duplicate the field names. I am imagining something like

class MyModel(Model):
    field = ...

    @add_choices_constraints
    class Meta:
        ...

# gets transformed into something like

class MyModel(Model):
    field = ...

    class Meta:
        @classmethod
        @property
        def constraints(cls):
            return [
                CheckConstraint(...)
                for field in get_all_django_fields(MyModel)
                if field.has_choices()
            ]

or maybe the add_choices_constraints decorator should go on the model itself. I am not sure if that would interfere with Django model magic though, because we would alter the model class after its definition is finished.

Or maybe, this is overkill and we want to track this manually. What do you think?