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

fix: household member delete + order #4510

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ColinBuyck
Copy link
Collaborator

@ColinBuyck ColinBuyck commented Dec 16, 2024

This PR addresses #977

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

This PR updates the deleteModal open logic to handle an orderId=0. When a user applies online, the first household member is set to an OrderId of 0 but when added via the partner's side, it adds it with an orderId of 1. This PR also updates the ordering on the partner's side so that a user sees the household members in the order they added them (public side is already demonstrating this behavior). However, I stopped short of refactoring the orderId to be used more consistently to avoid any risks in conflicting with existing data. Instead, that work is captured here: #4511

Note: I had originally assumed the orderId was null causing this issue but querying production showed no household members with null orderIds so this should resolve the issue.

How Can This Be Tested/Reviewed?

This can be tested a few ways. On the public side, sign in and apply to a listing while adding household members, editing and deleting household members. Then submit, and do the same on an autofill application. Ensure that the household members behave as expected and maintain the order at which they were added.

Then on the partner's side, check these same applications and click delete to ensure the modal opens for each of them. Also, check that the order updates and saves correctly when deleting a household member.

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for partners-bloom-dev ready!

Name Link
🔨 Latest commit b41fa73
🔍 Latest deploy log https://app.netlify.com/sites/partners-bloom-dev/deploys/6760a912e8358200086f137c
😎 Deploy Preview https://deploy-preview-4510--partners-bloom-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit b41fa73
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/6760a9120a8270000888e1ef
😎 Deploy Preview https://deploy-preview-4510--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -170,7 +170,7 @@ const FormHouseholdMembers = ({
</Drawer>

<Dialog
isOpen={!!membersDeleteModal}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: When the modal number was set to zero (when app submitted on public side) this would be false

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for partners-bloom-dev ready!

Name Link
🔨 Latest commit 9a0a5dd
🔍 Latest deploy log https://app.netlify.com/sites/partners-bloom-dev/deploys/6768bc22030a6b0008fdbd10
😎 Deploy Preview https://deploy-preview-4510--partners-bloom-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 9a0a5dd
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/6768bc223f53850008f5e72c
😎 Deploy Preview https://deploy-preview-4510--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ColinBuyck ColinBuyck marked this pull request as ready for review December 16, 2024 23:01
@ColinBuyck ColinBuyck added the 1 review needed Requires 1 more review before ready to merge label Dec 16, 2024
@emilyjablonski
Copy link
Collaborator

Seeing this in the issue: This appears to be related to the script Doorway uses to upload paper apps. The specific app they had an issue with was missing a specific field which wouldn't happen if creating the application from the UI. However, we should use the id rather than the ordinal number here here to avoid this happening in the future. Just want to make sure that was accommodated?

@ColinBuyck ColinBuyck force-pushed the 977/household-member-delete branch from 6e3cfe9 to 9a0a5dd Compare December 23, 2024 01:25
@ColinBuyck
Copy link
Collaborator Author

Good call out @emilyjablonski! I think there was an issue with my query checking the production db because as discussed in workshop... there are certainly household members with null order ids. I just pushed a new change that updates the order ids when they are pulled into the Paper Application Form and sorts them so they display in order that the user added them. While I don't love it, the reason I took this path rather than using the id is because a freshly added household member will not have an id until the paper app has been submitted so users would be unable to edit or delete household members while working on the paper application form.

Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

🦓

@ludtkemorgan ludtkemorgan added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed Requires 1 more review before ready to merge labels Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants