tuttle-dev / tuttle

🪰 Tuttle - time and money management for freelancers
GNU General Public License v3.0
62 stars 12 forks source link

UI: Form fields should describe the accepted input (data type, format) #169

Closed clstaudt closed 1 year ago

clstaudt commented 1 year ago

Forms should more clearly describe which input is accepted and validate it.

clstaudt commented 1 year ago

@vlad-ed-git

The model classes should contain all the necessary information, including a field description. How can the UI code reuse them?


class Contract(SQLModel, table=True):
    """A contract defines the business conditions of a project"""

    id: Optional[int] = Field(default=None, primary_key=True)
    title: str = Field(description="Short description of the contract.")
    client: Client = Relationship(
        back_populates="contracts", sa_relationship_kwargs={"lazy": "subquery"}
    )
    signature_date: datetime.date = Field(
        description="Date on which the contract was signed",
    )
    start_date: datetime.date = Field(
        description="Date from which the contract is valid",
    )
    end_date: Optional[datetime.date] = Field(
        description="Date until which the contract is valid",
    )
    # Contract n:1 Client
    client_id: Optional[int] = Field(
        default=None,
        foreign_key="client.id",
    )
    rate: condecimal(decimal_places=2) = Field(
        description="Rate of remuneration",
    )
    is_completed: bool = Field(
        default=False, description="flag marking if contract has been completed"
    )
    currency: str  # TODO: currency representation
    VAT_rate: Decimal = Field(
        description="VAT rate applied to the contractual rate.",
        default=0.19,  # TODO: configure by country?
    )
    unit: TimeUnit = Field(
        description="Unit of time tracked. The rate applies to this unit.",
        sa_column=sqlalchemy.Column(sqlalchemy.Enum(TimeUnit)),
        default=TimeUnit.hour,
    )
    units_per_workday: int = Field(
        description="How many units of time (e.g. hours) constitute a whole work day?",
        default=8,
    )
    volume: Optional[int] = Field(
        description="Number of units agreed on",
    )
    term_of_payment: Optional[int] = Field(
        description="How many days after receipt of invoice this invoice is due.",
        default=31,
    )
    billing_cycle: Cycle = Field(
        sa_column=sqlalchemy.Column(sqlalchemy.Enum(Cycle)),
        description="How often is an invoice sent?",
    )
``
vlad-ed-git commented 1 year ago

@clstaudt updated, pending review

clstaudt commented 1 year ago

@flamesjames care to test and review?

flamesjames commented 1 year ago

@clstaudt yes, I would like this one as well! What would be the end result you are looking for? Documentation of all of the test cases and their results? Anything else required?

clstaudt commented 1 year ago

@flamesjames Here you can basically take the role of a new user of the app, enter user data and check if the forms give you proper hints on what to fill in. These should also be consistent with the data fields defined in tuttle/model.py.

Comment here and/or create a (draft) Pull Request from the dev branch to propose changes.

flamesjames commented 1 year ago

@clstaudt sounds good! Thank you.

flamesjames commented 1 year ago

@clstaudt would you like documentation provided for the tests or not necessary?

clstaudt commented 1 year ago

@flamesjames what do you mean by documentation?

flamesjames commented 1 year ago

@clstaudt sorry - for some projects, I had to list my test cases in a word doc with the results of each test. If not, I can comment here once the fields are tested thoroughly..

clstaudt commented 1 year ago

@flamesjames Please no Word docs. Just test and comment here.

flamesjames commented 1 year ago

Hello @clstaudt - I have completed my testing on the fields; comments below:

Auth Screen

Profile Screen

Preferences Screen

Create New Contact Screen

Create New Client Screen

Create New Contract Screen

Create New Project Screen

Misc. Suggestions

👍🏾

clstaudt commented 1 year ago

Thank you for the thorough review @flamesjames! Let me turn this into tickets.

clstaudt commented 1 year ago

Name: It might be helpful to add verbiage for whether a full name should be entered or just a first name. I feel the use of a person’s name varies from app to app, so a user might think twice when seeing only “your name” as I did. If both first and last name are significant for Tuttle, it also might be suggested to break these apart into two separate text fields for data capture (only if the distinction is needed somewhere else in the app). If not necessary, one text field for name is fine.

-> #201

clstaudt commented 1 year ago

Not having a “Save” button on the “Cloud provider” screen in the Preferences section kinda threw me off, although you are saving the user a button click.

202

clstaudt commented 1 year ago

@vlad-ed-git Would you like to triage those comments, i.e. make tickets (in ToDo if really urgent because it throws testers off, in backlog otherwise) or decide "wontfix"?

The validation messages always showing right above “Client’s Name” could be slightly confusing. It would be better if validation messages could be near the field that failed validation.

It might be better UX to show required fields with a red asterisk or some way to clearly tell the user which info is required or not. Reason I say this is because when creating a new contract, after typing in a Title and choosing a client and clicking “Create Contract”, it just says “Failed to save the contract. Verify the info and try again” but doesn’t tell you which fields are causing that error message.

Upon selecting an Invoicing Contact from the dropdown, are the fields that are below that supposed to auto-populate with that contact’s info? That is not happening.

It may be a good idea to show an example format of the “Rate of remuneration” to let the user know how the data should be entered. I was also able to enter a with more than 2 decimal places.

I was also able to enter a large number with many decimal places for VAT rate.

clstaudt commented 1 year ago

Street Name/Number: I feel that the verbiage “Street Name” and “Street Number” could confuse some folks - “Address Line 1” and “Address Line 2” OR “Street Address” and “Apt/Bldg/Unit #” may be better as these seem more universal.

@flamesjames I think this is our first internationalization issue: Expectations differ depending on your location. Can there be a one size fits all solution? Do we need to customize the form based on country?

flamesjames commented 1 year ago

Street Name/Number: I feel that the verbiage “Street Name” and “Street Number” could confuse some folks - “Address Line 1” and “Address Line 2” OR “Street Address” and “Apt/Bldg/Unit #” may be better as these seem more universal.

@flamesjames I think this is our first internationalization issue: Expectations differ depending on your location. Can there be a one size fits all solution? Do we need to customize the form based on country?

@clstaudt - that's a great point, I can do some more research on this topic to see what our options are and get back to you.

clstaudt commented 1 year ago

@flamesjames It's 2023, can I make an overly optimistic wish? A Python library that parses any address from anywhere in the world from text and stores it in a flexible data structure so that it just works.

vlad-ed-git commented 1 year ago

Similar to my comment on the name field under #201 . Is there any point where we need for example, the street number itself (in the features of the app) and not a full address? If not (and I can't think of a reason we would) , this should just a single address field. And it shouldn't be separated (as we do now). Doing this makes this an internationalization issue unnecessarily.

Here is a link on best practices link

vlad-ed-git commented 1 year ago

Would you like to triage those comments, i.e. make tickets (in ToDo if really urgent because it throws testers off, in backlog otherwise) or decide "wontfix"?

Yes. I will comment here , then create necessary issues for the fixes.

The validation messages always showing right above “Client’s Name” could be slightly confusing. It would be better if validation messages could be near the field that failed validation.

Will fix this. #209

It might be better UX to show required fields with a red asterisk or some way to clearly tell the user which info is required or not. Reason I say this is because when creating a new contract, after typing in a Title and choosing a client and clicking “Create Contract”, it just says “Failed to save the contract. Verify the info and try again” but doesn’t tell you which fields are causing that error message.

I agree with specifying required fields. But will mark the optional fields, instead of the required ones.

Upon selecting an Invoicing Contact from the dropdown, are the fields that are below that supposed to auto-populate with that contact’s info? That is not happening.

Fixed.

@clstaudt you made some changes in these form dropdowns that introduced this issue. This was fixed on latest merge. The reason a drop-down item is prefixed with an id is because we need a way to get the object associated with the selected item (and hence we need to make the displayed item unique). I saw that you removed the prefix setting, but then didn't setup an alternative (I can't think of one myself), so I reverted that change. To illustrate, consider a drop-down that shows contacts. A contact name (or title) which is what we display in the drop-down for user to select, is not a unique field. We can't rely on it, as is, to figure out which contact exactly was selected. This is why I write #4. Christian Paul, with the 4 being the id, so this way I know which Christian Paul (of the **possibly*** hundreds) this is.

It may be a good idea to show an example format of the “Rate of remuneration” to let the user know how the data should be entered. I was also able to enter a with more than 2 decimal places.

@clstaudt this is an example of checks that should be added either as a method of a model e.g. is_valid() OR as part of the data source create/update method. Will fix this together (*which would fix the one below). #210

I was also able to enter a large number with many decimal places for VAT rate. Same as above

clstaudt commented 1 year ago

@vlad-ed-git sorry, wasn't aware that the "#id - " is required. Okay with reverting it for now. But unhappy that it is required: Database IDs are not meaningful to the user and should not be displayed. Other apps aren't doing that. Let's see if an alternative comes up.

clstaudt commented 1 year ago

@clstaudt this is an example of checks that should be added either as a method of a model e.g. is_valid() OR as part of the data source create/update method.

@vlad-ed-git Here we are probably not using the sqlmodel library's full potential yet. sqlmodel = sqlalchemy (an ORM) + pydantic (a library for data validation). Some required reading in order to avoid reinventing the wheel.

vlad-ed-git commented 1 year ago

@clstaudt We can go ahead and close this as all the fixes have been delegated to respective issues.