Finished Landlord Messaging Feature #406
Conversation
|
Parsa Tehranipoor seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
[diff-counting] Significant lines: 1512. This diff might be too big! Developer leads are invited to review the code. |
There was a problem hiding this comment.
This PR is very comprehensive and well organized! The code is clear and readable, and the modal flow works smoothly. I've left comments on a few small issues—mainly some form validation bugs (wrong error states being checked) and also some error handling—that can be easily fixed. Overall it looks good!
| if (inquiryType == 'availability') { | ||
| setSubject('Inquiry for ' + aptName + ' Rental Availabilities'); | ||
| setMessage( | ||
| 'Dear ' + |
There was a problem hiding this comment.
You could consider using template literals for better readability here.
| try { | ||
| // Check if email is null | ||
| if (email === null) { | ||
| console.error('No contact information found'); |
There was a problem hiding this comment.
Adding an error toast here would let users know that an error occurred and that their email didn't go through.
There was a problem hiding this comment.
Great catch! We will be working to create an alternative modal that allows users to direct to the landlord sight. The toast message could also be a good approach.
| <Box style={{ flex: isMobile ? 1 : '0 0 206px' }}> | ||
| <Box style={{ display: 'flex' }}> | ||
| <Typography className={classes.secondTitleBox}>Name</Typography> | ||
| {subjectError && ( |
There was a problem hiding this comment.
You're checking subjectError for the name field validation here, but this should be nameError. The same issue occurs on lines 562 (should use phoneError) and 598 (should use dateError).
There was a problem hiding this comment.
Great job with implementing the feature, the design was spot on too. As pointed out, it will be great to add some form of notification to the user to indicate the absence of the landlord email. Some approach proposed include:
- A toast message
- An alternative modal (e.g. linking to the apartment/landlord website)
For the testings, please also include some notes about each of the image/image groups. This will help organize tests and clearly indicate requirement achievements.
Overall good job!
Merge main repository code to current branch
parsa-tehranipoor
changed the title
parsa-tehranipoor
changed the title
There was a problem hiding this comment.
-
Nice fix with that calendar popup, you resolved the problem elegantly and it turns out to be pretty neat with the centered modal style. Even though it is not the same as the design, it could be beneficial, especially under the mobile screen layout. It is nice that we do not need to switch out the UI choice or conduct upgrades, so this really shows how deep understanding or good research of the UI kit you are using is important.
-
Overall welldone with completing the feature, excited to see it in production after we resolve the node version issue. Good job!
Summary
This pull request is split into two; the first half consists of the work I completed last semester, and the second half consists of the work I have added since then.
The first part of this PR includes all of the code for this feature's frontend on both mobile and computer devices. This PR also consists of the backend route needed for sending an email out to the landlord using the CUAPTs team email.
The second part of this PR involves fixing multiple backend bugs as well as polishing up the frontend. A test plan for the entire feature, ensuring emails are sent properly, is also included.
Test Plan
The backend route was successfully tested using my friend's email in place of the landlord's email to make sure I didn't accidentally send any test emails to the landlord. My friend successfully received an email from the CUAPTS email and was able to respond back to my personal email by replying to the previous email.
For testing out the frontend, here are screenshots of what the display looks like for different cases