Add tenant validation to match (current) backend requirements

[DaveTrost]

What issue is this solving?

No issue in Git. Some errors were noted during new tenant submission by @NickSchimek, @aedwardg and @DaveTrost in a Slack convo. This PR fixes those errors

Any helpful knowledge/context for the reviewer?

Is a npm install necessary? No
Any special requirements to test? Submit a new tenant, try to submit with/without a selected property and with/without lease dates. Note: because of the default values set by calendar modal, it is possible to submit a tenant without specifying lease dates, but at least the error text now directs users that the lease dates are required.

Please make sure you've attempted to meet the following coding standards

• Code builds successfully
• Code has been tested and does not produce errors
• Code is readable and formatted
• There isn't any unnecessary commented-out code
• There aren't any unnecessary commits or files changed (shouldn't touch the /stashed folder unless the issue requests it)

If you're having trouble meeting this criteria, feel free to reach out on Slack for help!

[aedwardg]

Looks good to me.

Although the unit number is not currently required on the backend, the other lease fields on this page (occupants and start/end dates) are. I think until the backend provides functionality for unhoused tenants, these changes will be super useful.

Particularly with the upcoming demonstration, this will prevent accidentally omitting lease details and having the site hang (no start/end dates) or receiving a 400 error for "" being an invalid integer (no occupants) 😅

[NickSchimek]

OK, take my review with a grain of salt. However, one thing I know for sure is that unitNum should not be required as it could be an ADU. See codeforpdx/dwellinglybackend#158 (comment)

[aedwardg]

@NickSchimek even though occupants isn't required on the backend, if you submit the form without a value for occupants, you get a 400 error that says "" is not a valid integer. This has a similar effect to requiring that field.

Is this maybe an issue with what the frontend is submitting for blank fields?

[DaveTrost]

@NickSchimek and @aedwardg - I totally agree with what you both are saying. This PR is not changing the app to work the way it is meant to work. Instead, this code prevents errors during the demo.

I did have some interesting problems when submitting partial data. I didn't investigate the Python to the point of understanding (again, not the point of this PR) but I think sharing and documenting them here will help us make a better solution in the long run. So, here's what I tried (experiment results from Postman):

Success

• occupants: null - this works. Thank you @aedwardg for pointing this out.
• unitNum: null - this also works
• omit dateTimeStart and dateTimeEnd from the data - This works

Failure

• dateTimeStart: null - Yields a 400 error "Field may not be null."
• dateTimeEnd: null - Yields a 400 error "Field may not be null."

Interesting Dependencies

• propertyID: null and dateTimeStart/End omitted from the data (ie keys are not present in the JSON) - this works
• propertyID: null and dateTimeStart/End included in the data - this yields "propertyID": ["Field may not be null."]. AND a followup request yields a new error: "message": "A tenant with this first and last name already exists"
  • I think this means the lease creation failed, and the tenant creation continued. I would prefer to prevent committing the tenant entry to the db if bad lease data accompanies the tenant.

[DaveTrost]

Okay. I made some more changes. Still, this is far from perfect. The form needs custom validation - specifically to ensure that lease and property information are provided in conjunction with the other. It needs to be valid to submit a tenant without lease or property information. It also needs to be valid to include lease or property information with the tenant. For this case, both pieces of information are required.
Note: the UI for the calendar modal does not have a "clear" option, so once the user starts to enter lease dates in the calendar, there's no going back to the starting point of the form to submit a tenant without a lease/property

[NickSchimek]

@NickSchimek even though occupants isn't required on the backend, if you submit the form without a value for occupants, you get a 400 error that says "" is not a valid integer. This has a similar effect to requiring that field.

Is this maybe an issue with what the frontend is submitting for blank fields?

Well, that is a valid validation. An empty string is certainly not an integer.

It is not uncommon for frontends to send an empty string when a field has not been filled in by the user.

We can either have the frontend only send the data that was entered - this would fix the issue without having to make any changes in the backend.

or

In the backend - we will need to setup the marshmallow schemas to not validate this field when it is an empty string and set it to None.

Either way is fine with me.

Making the field required only patches the problem for someone else to discover and fix down the road.

[NickSchimek]

Looks like there is a long standing discussion on empty strings in the Marshmallow repo:
marshmallow-code/marshmallow#1381

[NickSchimek]

@aedwardg Looks like @DaveTrost already fixed the issue by setting the values to null instead of empty string. Nice work @DaveTrost

[aedwardg]

Great job getting this all fixed so quickly, @DaveTrost!

I only have the one suggested change. Other than that, everything seems to work the way it should now as far as I can tell 😄

[aedwardg]

When you try to submit a new tenant w/no lease through the UI, this line will cause the site to hang if a property is not selected. The console error I received was a Formik TypeError, "propertySelection[0] is undefined"

I was able to get rid of this error and successfully create an "unhoused" tenant by changing the line to:

Suggested change

	propertyID: propertySelection[0].key,
	propertyID: propertySelection[0] ? propertySelection[0].key : null,

I'll leave it to your discretion whether this is the best way to handle this, but we definitely should not have to select a property to create a tenant.

[NickSchimek]

I agree. Property should be considered as part of the lease form. Tenants should not have a direct relationship with property, they're only assigned a property through a lease.

[aedwardg]

@DaveTrost thanks for all the hard work on this! Your most recent commit does a good job handling the empty property field when no lease info is filled out. 🥳

I did notice something else 👇 but I think it should probably be addressed in a different issue/PR so we can get this one merged.

---

If you select the lease dates, you get prompted to select a property - this is correct behavior. However, the user is not blocked from hitting the Save button. So, the post request goes through and a tenant gets created successfully, but the lease creation fails (as it should -- propertyID can't be null in a lease).

I don't know the best way to handle this from a UI/UX perspective, so I think maybe it should be discussed by people with more frontend knowledge. Personally, I wouldn't be opposed to allowing this behavior (successful tenant creation, failed lease creation) so long as appropriate popups were shown. Users could always go in later and add a lease for the tenant.

For example:

• User could be prompted on on hitting Save that proceeding w/lease dates but no propertyID will result in tenant creation, but no lease.
• Allow user to choose whether to go back or proceed anyway.
• If they proceed, show a green success popup for tenant and a red fail popup for lease.

Alternatively, you could just disable the save button once lease dates are filled out until either:

• a property is selected; or
• the dates are reset.

Again, I'm no expert in UI/UX, so this is probably better left to a separate frontend issue.

[NickSchimek]

Thanks @DaveTrost !! This is a big improvement.