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

Unique Resident IDs #200

Merged
merged 13 commits into from
Dec 3, 2023
Merged

Unique Resident IDs #200

merged 13 commits into from
Dec 3, 2023

Conversation

kevin-pierce
Copy link
Contributor

@kevin-pierce kevin-pierce commented Nov 12, 2023

Notion task link

Unique Resident ID

Implementation description

  • Added new response type for editing / adding functions of resident client
  • Added error toast on error response on FE
  • Added helper to type narrow on error response type for result
  • Added logic to resident routes to call get_residents prior to editing / updating residents

Steps to test

Create Resident

  1. Create a resident
  2. Attempt to create another resident with the same initials + room number (Expect: error toast, no change to residents table state)
  3. Attempt to create another resident with different initials + same room number OR same initials + different room number OR different initials + different room number (Expect: success toast, new resident added to list of residents)

Edit Resident

  1. Create two resident unique residents
  2. Attempt to edit one of the residents such that the initials + room number match those of the other resident (Expect: error toast, no change to residents table state)
  3. Attempt to edit the same resident with no changes (Expect: success toast)
  4. Attempt to edit a resident and set their initials + room number to be another non-existent combination (Expect: success toast)

What should reviewers focus on?

  • Optimizations around how we can avoid making the duplicate check calls on every addition + update of a resident (I was thinking of something involving the frontend and how we can check what fields were modified ; accordingly, we send back a flag / some status that indicates that we do / do NOT need to perform this check)

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR
  • If I have made API changes, I have updated the REST API Docs
  • IF I have made changes to the db/models, I have updated the Data Models Page
  • I have documented this PR in the Production Release Page
  • I have updated other Docs as needed

Copy link

github-actions bot commented Nov 12, 2023

Visit the preview URL for this PR (updated for commit 63eaf16):

https://blueprintsupportivehousing--pr200-kevin-unique-residen-dk93zcfe.web.app

(expires Sun, 10 Dec 2023 17:47:08 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

@kevin-pierce kevin-pierce marked this pull request as ready for review November 12, 2023 21:00
fmt_resident_id = resident.get("initial") + str(resident.get("room_num"))
try:
res = residents_service.get_residents(False, 1, 10, fmt_resident_id)
if len(res["residents"]) > 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding a new resident, we simply want to check for a result length greater than 0 (if there's even one other resident that has the same ID, then this is a duplicate resident => reject)

fmt_resident_id = updated_resident.get("initial") + str(updated_resident.get("room_num"))
try:
res = residents_service.get_residents(False, 1, 10, fmt_resident_id)
if len(res["residents"]) == 1 and res["residents"][0]["id"] != resident_id:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When editing a resident, there are two cases for which the result of get_residents could be >=1 in size:

  1. There was no change to the resident ID (i.e. a resident with ID AA123 has their building num changed ; ID remains AA123 and so when we get residents with the ID fmt_resident_id = AA123, then we get 1)

  2. We changed another resident to have a pre-existing resident ID (there are residents AA123 and AA321 ; resident AA123 has their room number changed to 321, resulting in two duplicate entries ; thus when we fetch the residents with the ID AA321, we end up with a result of 1)

We want case #2 to be marked as INVALID but case #1 to be VALID. As such, our checking logic must check for:

  • If there's already a result being returned
  • If the result has the same database ID as us (if it's the same, then we're editing the same record, meaning this change is valid ; if the database IDs are different, then this means that there is a different resident with the same formatted ID as us)


if (axiosErr.response && axiosErr.response.status === 409) {
return {
errMessage: "Resident with the specified user ID already exists."
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we're propagating the error message displayed in the toast through the API client

getRecords(1);
countResidents();
setUserPageNum(1);
setShowAlert(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We move the call to here since the showAlert state controls the visibility of a success alert ; we only want the success state to be displayed if we get back a boolean result with value true

Copy link
Contributor

@braydonwang braydonwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I tested creating and editing residents on the frontend and everything works as expected, and I didn't find any real problems related to the code

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THE EXPLANATIONS ARE 💯 , one thing I think is worth changing is keeping the modal open (and not clearing whatever was entered) when a duplicate resident is entered

Also I'm thinking since we have a few more error types/messages now it might be worth having them in their own shared files:

  • Can have a file ErrorTypes.ts (under /types) which can hold the ErrorResponse here and the one defined in https://github.com/uwblueprint/supportive-housing/pull/202/files
  • Under helpers we have a file called errors.ts which can hold everything in residentError.ts and authError.ts

Just so it makes things more reusable if we ever need to use the functionality in the future.

@kevin-pierce kevin-pierce merged commit 129fba0 into main Dec 3, 2023
3 checks passed
@kevin-pierce kevin-pierce deleted the kevin/unique-resident-ids branch December 3, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants