Using kind of enums for document states

Today, once again, a customer stumbled upon some code that didn’t work because the selection field we use for the state use the word cancelled while their code use the word canceled (AFAIK they’re both valid english words). It’s not a big problem because in theory those issues should be catched with good tests, nevertheless it’s something that we can fix.

I remember a discussion we had with @ced about replacing the states by the python enums but it’s not possible because they wouldn’t work with our way of extending the objects.

But I thought what if we drop some of the requirements that python impose on itself about the enums? And I wrote this small piece of python that could be used for this purpose:

class DuplicationError(ValueError):
    pass


class InvalidOperationError(TypeError):
    pass


class States:

    def __init__(self, **initial_states):
        self.__states = {}
        if initial_states:
            for name, string_value in initial_states.items():
                setattr(self, name, string_value)

    def __getattr__(self, name):
        if name in self.__states:
            return name
        raise AttributeError(name)

    def __setattr__(self, name, value):
        if name == '_States__states':
            return super().__setattr__(name, value)

        if name in self.__states:
            raise DuplicationError(name)
        for existing_value in self.__states.values():
            if existing_value == value:
                raise DuplicationError(value)
        self.__states[name] = value

    def __delattr__(self, name):
        raise InvalidOperationError(name)

    def __iter__(self):
        return iter(self.__states.items())


sale_state = States(
    draft="Draft", quotation="Quotation", processing="Processing")
sale_state.done = "Done"

assert sale_state.draft == 'draft'

for state in sale_state:
    print(state)

sale_state.toto
# raise AttributeError

sale_state.done = "It's over"
sale_state.finish = "Done"
# raise a DuplicationError

The main issue I have with that is that it constraints the database value of every state to be a valid python variable name (ie it can not start with a number, but since python 3 it can start by almost any other unicode codepoint).

As I said it’s not a very big problem and to be honest I am not even sure it’s worth the time to do a patch (hence this discussion).

I do not think it brings value to try to mimic the Python Enum. And I do not see how it will work with the modularity.
Also I found that it will be quite expensive to have to change all the Selection fields without bringing new feature to the user.

So I’m wondering if we could not have on Selection field a dynamic attribute (of type string) that ensures the attribute requested is in the selection list.
The code will look like:

Invoice.search([('state', '=', Invoice.state.selection_draft)])

It should work also with spaces by using getattr.

1 Like

It’s the goal of the __setattr__, in a __setup__ call a new module could set a new attribute. But it could be something else completely.

Indeed.

Wouldn’t it be better to make something like:

Invoice.search([('state', '=', Invoice.state.states.draft)])

That way the __getattr__ code reside in the states object instead of being in the field.
It seems a better separation of concerns.

How could it? I think this requirement doom the whole idea.

I do not see why there should be a states attribute on Selection field.
But also I think it will make code more complicated to use a separate object for testing that the requested string is in the selection. It will probably require to nested scope or instantiation on each call.

For me the code will look like:


class Selection(Field):
   ...

   def __getattr__(self, name):
       if name.startswith('selection_'):
           value = name[len('selection_'):]
           if isinstance(self.selection, (tuple, list)) and value not in dict(self.selection):
               raise AttributeError("Unknown selection %s in %s" % (name, self.name)) 
           return value
       else:
           return super().__getattr__(name)

And to use with spaced value, it could be called:

Model.search([('state', '=', getattr(Model.state, 'selection_value with space'))])

On a second though I’m not sure that we tackle the problem the right way. Indeed the problem happens only when searching against a Selection field value because when writing value we already enforce that the value is in the selection. Also solution based on code syntax/type will not work for RPC calls.

So maybe the best would be to check in _domain_value of Selection that the value is in the selection. And maybe it should be earlier (or add the Model as parameter) to allow to call the selection method if it is a class method. Of course it will be enforced only for = or in operators (and not for ilike).

As I said: separation of concerns.
I don’t think it’s the responsibility of the field to do this.

No, just a reference to the field.

OK, I haven’t thought about getattr but omg this is ugly :slight_smile:.

The use case was something like:

if previous.sepa_mandate.state == 'canceled': …

(we use both canceled and cancelled in our modules, the version with two l is more used though).

So, no searching is not the only way to stumble on the problem.

So this reference will be passed by nested scope or instantiation (or argument but it is the same).
I do not see the point to separate something that requires only the field definition.

So we should implement both solutions.

This has been harmonized.

It’s an argument or course. And it’s not the same as a nested scope. The end result looks like it’s the same but it’s not the same at all.

From my pov it’s not a question of what you need but of what are your responsibilities. I don’t think it’s the responsibility of the field to ensure that. Their concern is about transferring data from/to the database and defining constraints on it.

But it’s a matter of taste, you could see that feature as “accessing the data the right way” or as “exposing an interface to the valid data”.

I prefer the second because I like to cut stuff in smaller pieces (but I agree that I don’t think this object will ever evolve).

Indeed I grepped their source code :man_facepalming:

I think it will be better to get the value from the class Model instead of the Field like that we can also check the selection with classmethod (which is quite common when we share the same selections between models).
Of course the Model method/accessor can still call a method on the Selection field and passing itself.

Maybe we could for example automatically define a class attribute as uppercase version of the Selection field on the Model like: Invoice.STATE.draft.