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: Form fields should describe the accepted input (data type, format) #169

Closed
clstaudt opened this issue Jan 22, 2023 · 23 comments
Closed
Assignees
Labels
backend documentation Improvements or additions to documentation good first issue Good for newcomers UX/UI

Comments

@clstaudt
Copy link
Contributor

clstaudt commented Jan 22, 2023

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

@clstaudt clstaudt converted this from a draft issue Jan 22, 2023
@clstaudt clstaudt moved this from Backlog to Todo in Tuttle Dev Jan 22, 2023
@clstaudt
Copy link
Contributor Author

@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?",
    )
``

@clstaudt clstaudt added documentation Improvements or additions to documentation good first issue Good for newcomers backend UX/UI labels Jan 22, 2023
@vlad-ed-git vlad-ed-git moved this from Todo to Pending Review in Tuttle Dev Jan 23, 2023
@vlad-ed-git
Copy link
Collaborator

vlad-ed-git commented Jan 23, 2023

@clstaudt updated, pending review

@clstaudt
Copy link
Contributor Author

@flamesjames care to test and review?

@flamesjames
Copy link

@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
Copy link
Contributor Author

@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
Copy link

@clstaudt sounds good! Thank you.

@flamesjames
Copy link

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

@clstaudt
Copy link
Contributor Author

@flamesjames what do you mean by documentation?

@flamesjames
Copy link

@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
Copy link
Contributor Author

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

@flamesjames
Copy link

flamesjames commented Jan 27, 2023

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

Auth Screen

  • 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.

  • 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.

  • Street Number: Should this field be considered required? (if this has the same meaning as “Apt/Bldg/Unit #”.

  • Validations on all of the required fields occurred correctly if a user tried to move forward without filling a required field out.

Profile Screen

  • The same validations noted on the Auth Screen were working on the profile screen.

Preferences Screen

  • The dropdown pickers on the Cloud and Locale screens were descriptive.

Create New Contact Screen

  • Street and Street No appear here as well (see note above about that verbiage).
  • Other than that, fields were validated correctly.

Create New Client Screen

  • 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.
  • 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.
  • There were address validations on the “Invoicing Contact” fields, however, I was able to submit without entering a Country for one of them.

Create New Contract Screen

  • 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.

Create New Project Screen

  • The fields/hints were good here. However, I selected a Contract from the dropdown but it was still throwing the “Please specify the contract” validation error message.

Misc. Suggestions

  • 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.
  • The textfield labels get pretty small after entering values (and they are small to begin with). I feel slightly increasing the font size would be useful.
  • After adding a Contact on the Contacts screen, the validation displayed in red “You have not added any contacts yet” does not go away. Same thing happened on My Clients screen.
  • 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.

👍🏾

@clstaudt
Copy link
Contributor Author

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

@clstaudt
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

clstaudt commented Jan 28, 2023

@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
Copy link
Contributor Author

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
Copy link

flamesjames commented Jan 28, 2023

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
Copy link
Contributor Author

clstaudt commented Jan 28, 2023

@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
Copy link
Collaborator

vlad-ed-git commented Jan 28, 2023

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
Copy link
Collaborator

vlad-ed-git commented Jan 28, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@github-project-automation github-project-automation bot moved this from Pending Review to Done in Tuttle Dev Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend documentation Improvements or additions to documentation good first issue Good for newcomers UX/UI
Projects
Status: Done
Development

No branches or pull requests

3 participants