hasgeek / baseframe

Baseframe for Hasgeek projects
BSD 3-Clause "New" or "Revised" License
11 stars 17 forks source link

Supply context for forms #112

Open jace opened 8 years ago

jace commented 8 years ago

Forms often require additional context to be able to do validations correctly.

For example, Lastuser's profile form needs to validate the provided username against usernames already taken in the database. This requires two pieces of context: the User model, and the current user's record so we exclude that record when checking if the username is already taken. This means context cannot be hardcoded into the form. The form may have a permanent reference to the User model, but needs the view to tell it what user's record is being edited.

WTForms provides for one crucial piece of context: the object being edited as the obj parameter to the form constructor. We've extended this in Baseframe's Form class to also expect model and parent. However, forms may sometimes require even more context. Our approach has been to stick these attributes into the form in the view, and to expect them to be present in the form's validators. Sometimes it is the other way: the validator sticks in a new attribute and the view reads it (example in Hasjob: validator and view).

This approach makes it hard to reuse a form without being aware of such expectations, and so this calls for a formal API. I propose adding two attributes to the form class, __expects__ and __returns__, both of which contain a list of strings naming (a) parameters that must be present in the constructor failing which an exception is raised, and (b) attributes guaranteed to be present on the form instance after validation (default value None).

jace commented 7 years ago

QuerySelectField requires either a query on the instance or a query_factory in the form definition that returns a query. The factory gets no parameters and hence can't return a custom query based on context. query itself should not be set in the view, for the sake of automatic view construction. The form should do it instead since it already receives context.

Since there is no way to define an appropriate query in a base class, I propose this be part of the convention around form construction.

Update: This particular issue was addressed in #145.

jace commented 6 years ago

Python 3.x's __init_subclass__ can be used to validate the contents of __expects__ and __returns__:

    def __init_subclass__(cls, **kwargs):
        super().__init_subclass__(**kwargs)
        if {'edit_obj', 'edit_model', 'edit_parent', 'edit_id'} & set(cls.__expects__):
            raise TypeError("This form has __expects__ parameters that are reserved by the base form")

        if set(cls.__dict__.keys()) & set(cls.__expects__):
            raise TypeError("This form has __expects__ parameters that clash with field names")

However:

  1. Baseframe is not yet compatible with Python 3.x
  2. There is no solution for Python 2.x other than moving the validation into __init__, which is inefficient, or creating a new metaclass with the added complexity of remaining compatible with WTForms's existing metaclass.
jace commented 6 years ago

Since we now have Py3 support and intend to upgrade our apps, the __init_subclass__ method can be added.

jace commented 5 years ago

There is no validation of __returns__ yet, and this needs to be addressed. We can't analyse code to confirm each item is being set, but we can ensure a default value None is always present:

  1. __init_subclass__ can do this in the class itself (Py3 only), or
  2. validate can set default values before returning (Py2+Py3, but slower)

Given the non-existent use so far, we can take the Py3-only solution.

In addition, __init_subclass__ is currently using cls.__dict__ but this may not contain fields defined in base classes unless the WTForms metaclass explicitly copies them (like SQLAlchemy does). This needs to be tested.