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: improve seed data and validity #4099

Merged
merged 8 commits into from
Jun 5, 2024
Merged

fix: improve seed data and validity #4099

merged 8 commits into from
Jun 5, 2024

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented May 21, 2024

Pull Request Template

Description

I ran into a lot of issues testing duplicate problems locally because of some invalid and missing seed data - this PR fixes a handful of problems related to seed data and a few bugs with how that data was showing up in the applications table. I added a comment to each fix. It also types two application fields with an enum that were previously just a set of strings the frontend was sending to the backend.

How Can This Be Tested/Reviewed?

Run yarn setup:dev and check out the applications table on a listing to see more applications with more data and more variety.

Submit an application and fill out alternate contact and household member data to ensure it comes through as expected on the partners side to ensure the new typing places nicely.

Checklist:

  • My code follows the style guidelines of this project
  • I have added QA notes to the issue with applicable URLs
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have run yarn generate:client and/or created a migration if I made backend changes that require them
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described
  • Commits made across packages purposefully have the same commit message/version change, else are separated into different commits

Reviewer Notes:

Steps to review a PR:

  • Read and understand the issue, and 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
  • Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
  • Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

householdMember: householdMembers,
multiselectQuestions,
}),
for (let j = 0; j <= APPLICATIONS_PER_LISTINGS - 1; j++) {
Copy link
Collaborator Author

@emilyjablonski emilyjablonski May 21, 2024

Choose a reason for hiding this comment

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

Previously, all applications had the same household size and same household members, and one more application than the actual APPLICATIONS_PER_LISTING

Copy link
Collaborator

Choose a reason for hiding this comment

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

real nit pick: could we change this to for (let j = 0; j < APPLICATIONS_PER_LISTINGS; j++) {
just a lil less code

create: addressFactory(),
}
: undefined,
orderId: index,
Copy link
Collaborator Author

@emilyjablonski emilyjablonski May 21, 2024

Choose a reason for hiding this comment

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

Without this new index, we were previously unable to view individual household members on an application in partners

phoneNumber: undefined,
emailAddress: undefined,
agency: undefined,
address: { create: addressFactory() },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is technically inaccurate but we're sending a strange empty-but-with-id address on alternate contact through the frontend on actual application submission and that felt like a bigger problem

birthDay: randomBirthDay(),
birthYear: randomBirthYear(),
sameAddress: YesNoEnum.yes,
relationship: relationshipKeys[randomInt(relationshipKeys.length)],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now including relationship

@@ -6,7 +6,7 @@ export const jurisdictionFactory = (
listingApprovalPermissions?: UserRoleEnum[],
): Prisma.JurisdictionsCreateInput => ({
name: jurisdictionName,
notificationsSignUpUrl: null,
notificationsSignUpUrl: 'https://www.exygy.com',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allows the notifications block to show up in dev

Copy link

netlify bot commented May 22, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit fffa375
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/665fa09af920580007cea782
😎 Deploy Preview https://deploy-preview-4099--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.

@@ -492,7 +492,7 @@ export function getColDefs(maxHouseholdSize: number, countyCode: string) {

const householdCols = []

for (let i = 0; i < maxHouseholdSize; i++) {
for (let i = 0; i < maxHouseholdSize - 1; i++) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously on the applications table in partners, we were always showing an additional set of household member columns that never had data

@@ -562,7 +562,7 @@ export function getColDefs(maxHouseholdSize: number, countyCode: string) {
width: 125,
minWidth: 100,
valueFormatter: ({ value }) => {
if (!value) return ""
if (value.length < householdIndex) return ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, we were showing n/a just in these columns for otherwise empty household members

dataTestId="alternateContact"
/>
</Grid.Row>
{application.alternateContact?.type !== AlternateContactRelationship.noContact && (
Copy link
Collaborator Author

@emilyjablonski emilyjablonski May 22, 2024

Choose a reason for hiding this comment

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

If there is no alternate contact, don't show the address section for it

birth: t("application.household.member.dateOfBirth"),
sameResidence: t("application.add.sameResidence"),
workInRegion: t("application.details.workInRegion"),
name: "t.name",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was throwing console errors - it's a little odd but the function these are passed into just takes the string which it passes into t()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternate contact didn't exist

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just added missing data

@emilyjablonski emilyjablonski added the 1 review needed Requires 1 more review before ready to merge label May 22, 2024
@emilyjablonski emilyjablonski marked this pull request as ready for review May 22, 2024 03:01
@@ -0,0 +1,7 @@
export enum AlternateContactRelationship {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we check with product if we should be making this and the HouseholdMemberRelationship enums readable in the CSV export?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilyjablonski can you also make a ticket to change the db to be enums instead of strings as well

I think that work needs to be captured separately as the roll out for it will require migrations and db work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And that CSV issue looks existing, but I'll look for a ticket!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

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

Looks good!
some questions and nits

api/prisma/constants.ts Outdated Show resolved Hide resolved
householdMember: householdMembers,
multiselectQuestions,
}),
for (let j = 0; j <= APPLICATIONS_PER_LISTINGS - 1; j++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

real nit pick: could we change this to for (let j = 0; j < APPLICATIONS_PER_LISTINGS; j++) {
just a lil less code


export const alternateContactFactory =
(): Prisma.AlternateContactCreateWithoutApplicationsInput => {
const relationshipKeys = Object.values(AlternateContactRelationship);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this paired with line 12 is so smart!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I stole it from Morgan haha

@@ -0,0 +1,7 @@
export enum AlternateContactRelationship {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilyjablonski can you also make a ticket to change the db to be enums instead of strings as well

I think that work needs to be captured separately as the roll out for it will require migrations and db work

@YazeedLoonat YazeedLoonat added needs changes The author must make changes and then re-request review before merging and removed 1 review needed Requires 1 more review before ready to merge labels May 29, 2024
@emilyjablonski emilyjablonski added 1 review needed Requires 1 more review before ready to merge and removed needs changes The author must make changes and then re-request review before merging labels Jun 3, 2024
@emilyjablonski
Copy link
Collaborator Author

@YazeedLoonat So sorry, I re-requested a review, but realized I never actually pushed the commit lol

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

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

LGTM

@YazeedLoonat YazeedLoonat 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 Jun 5, 2024
@emilyjablonski emilyjablonski merged commit adab010 into main Jun 5, 2024
20 checks passed
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 6, 2024
emilyjablonski added a commit to housingbayarea/bloom that referenced this pull request Jun 6, 2024
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 11, 2024
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 13, 2024
emilyjablonski added a commit to housingbayarea/bloom that referenced this pull request Jun 27, 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.

None yet

3 participants