Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ui/Model: Add field validation / format methods #210

Open
vlad-ed-git opened this issue Jan 30, 2023 · 10 comments · Fixed by #207
Open

Ui/Model: Add field validation / format methods #210

vlad-ed-git opened this issue Jan 30, 2023 · 10 comments · Fixed by #207
Assignees

Comments

@vlad-ed-git
Copy link
Collaborator

vlad-ed-git commented Jan 30, 2023

Fields, such as Vat Rate under Contract, should have validation (and formatting where necessary) mechanisms to validate and format the user input according to the expected standard. E.g. keep decimals fixed, allow only numbers, etc

@clstaudt
Copy link
Contributor

clstaudt commented Jan 30, 2023

Working on using sqlmodel/ pydantic for validation #207

Code example - note the pattern

try:
    model_instance = Model.validate(
       dict(
          spam="Spam",
          eggs="Eggs",
       )
    )
except ValidationError:
  ...
class TestContact:
    def test_valid_instantiation(self):
        contact = Contact.validate(
            dict(
                first_name="Sam",
                last_name="Lowry",
                email="sam.lowry@miniinf.gov",
                company="Ministry of Information",
            )
        )
        assert store_and_retrieve(contact)

    def test_invalid_email(self):
        with pytest.raises(ValidationError):
            Contact.validate(
                dict(
                    first_name="Sam",
                    last_name="Lowry",
                    email="27B-",
                    company="Ministry of Information",
                )
            )

@clstaudt
Copy link
Contributor

clstaudt commented Jan 30, 2023

@vlad-ed-git Tell me whether I am right about this: In order to be serious about decoupling view and data model, view code should not be concerned with data validation. More specifically, there should be nothing in the view code that duplicates any of the definitions in model, or adds any definitions, e.g.:

  • whether a field is optional or not
  • what the data type of a field is
  • what a valid string format for a field is
  • ...

I believe there is currently some code in the view that tries to do data validation, e.g.

        if not self.contact.first_name or not self.contact.last_name:
            self.on_error_callback("First and last name cannot be empty")
            return

Goal: Remove all code of this kind from the view. Let pydantic / sqlmodel do validation via Model.validate method.

Check out pydantic validators.

@vlad-ed-git
Copy link
Collaborator Author

@clstaudt
Some definitions, such as whether a field is optional or not, are hinted on the View. And after all, any change to add or remove a field from a model will require a corresponding change in the Ui. For example. if we stop asking the user for their name and deleted that field from the User model, then the name form field must be removed from the Ui.
*Also the input type defines the keyboard type (though not as meaningful for desktop apps) but still, if a number is expected, then the text field input's keyboard type is set to Number (this is why on your phone when you are typing an email in a form, the displayed keyboard is different from when you are typing a phone number for example). Flet being cross-platform offers the same mechanism, though, for our desktop app, it is not as useful (perhaps not useful at all).
But apart from these, yes, validation is best done on the model, so you are correct. We should replace the current data validation checks on the view.

@vlad-ed-git
Copy link
Collaborator Author

vlad-ed-git commented Jan 30, 2023

@clstaudt One more thing, consider a required field Rate: Rate of enumeration. To be valid it should be 1. not empty, 2. numeric (not alphanumeric or just letters), and perhaps some other check like the number of decimal places.
Therefore the Ui (actually its corresponding Intent class) must know if a field is invalid then what exactly is the issue so it can display the appropriate error message. The implementation of validate must take this into account and not raise generic errors.

@vlad-ed-git
Copy link
Collaborator Author

@clstaudt By the way, there was an issue in which you mentioned something about having a Ui form that is generated from the model. This is something that Django does. I do not know if there is a library for it (and even if there is, I doubt it will convert python to flet's flutter) but we could implement this ourselves in the future.

@clstaudt
Copy link
Contributor

clstaudt commented Jan 30, 2023

Flet being cross-platform offers the same mechanism, though, for our desktop app, it is not as useful (perhaps not useful at all).

We build a desktop app with no mobile version planned. Seems like there's no need to spend time on the keyboards.

By the way, there was an issue in which you mentioned something about having a Ui form that is generated from the model. This is something that Django does. I do not know if there is a library for it (and even if there is, I doubt it will convert python to flet's flutter) but we could implement this ourselves in the future.

I bet ChatGPT does well in generating form code from model code, if it is prompted with good examples.

@clstaudt
Copy link
Contributor

Therefore the Ui (actually its corresponding Intent class) must know if a field is invalid then what exactly is the issue so it can display the appropriate error message. The implementation of validate must take this into account and not raise generic errors.

@vlad-ed-git How about the view inspecting the ValidationError and handling / displaying all of its errors?

from tabulate import tabulate

def print_validation_errors(ve: ValidationError):
    errors = ve.errors()
    table = []
    for error in errors:
        field_name = error.get('loc')[0]
        error_message = error.get('msg')
        table.append([field_name, error_message])
    print(tabulate(table, headers=["Field Name", "Error Message"]))

try:
    client = Client.validate(dict(name="Ministry of Information"))
except ValidationError as ve:
    print_validation_errors(ve)

@vlad-ed-git
Copy link
Collaborator Author

vlad-ed-git commented Jan 30, 2023

@clstaudt I am not sure about the rest of that code, but if the validation error is such that this part works, then great:

    errors = ve.errors()
    table = []
    for error in errors:
        field_name = error.get('loc')[0]
        error_message = error.get('msg')

@clstaudt
Copy link
Contributor

The rest of the code is just for example output. This is working:

    def test_missing_name(self):
        """Test that a ValidationError is raised when the name is missing."""
        with pytest.raises(ValidationError):
            Client.validate(dict())

        try:
            client = Client.validate(dict())
        except ValidationError as ve:
            for error in ve.errors():
                field_name = error.get("loc")[0]
                error_message = error.get("msg")
                assert field_name == "name"

@clstaudt
Copy link
Contributor

clstaudt commented Apr 8, 2023

@vlad-ed-git Can we review this together?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending Review
Development

Successfully merging a pull request may close this issue.

2 participants