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.

Sign up for GitHub

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

Jump to bottom

MPDX-7946 use contact mutation when adding email #973

Merged
merged 5 commits into from
Aug 2, 2024
Merged

MPDX-7946 use contact mutation when adding email #973

merged 5 commits into from
Aug 2, 2024

Conversation

cjpastika
Copy link
Contributor

@cjpastika cjpastika commented Jul 5, 2024 •
edited by wrandall22
Loading

Description

MPDX 7946
Related to MPDX 7941

Will add a contact mutation when users click the + button after entering a valid email

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@cjpastika cjpastika added the Preview Environment Add this label to create an Amplify Preview label Jul 5, 2024
@dr-bizz
Copy link
Contributor

dr-bizz commented Jul 8, 2024

It looks like you've done the same thing here: #958.

You need to merge main into this branch

@cjpastika cjpastika force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch 2 times, most recently from 71cf81d to 633d1fa Compare July 8, 2024 19:54
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024 •
edited
Loading

Bundle sizes [mpdx-react]

Compared against b77c3de

No significant changes found

@cjpastika cjpastika force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch 3 times, most recently from b8bf6fd to db1a35a Compare July 9, 2024 19:24
@cjpastika cjpastika force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch from ada4713 to a62a556 Compare July 16, 2024 18:31
@wrandall22 wrandall22 force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch 2 times, most recently from 626f5f6 to 17b3df4 Compare July 19, 2024 16:00
@wrandall22 wrandall22 added the Waiting on another PR This PR requires approval of another to be completed label Jul 25, 2024
@wrandall22 wrandall22 force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch from 17b3df4 to 885fd4c Compare July 26, 2024 16:01
@wrandall22 wrandall22 removed the Waiting on another PR This PR requires approval of another to be completed label Jul 26, 2024
@wrandall22 wrandall22 requested a review from dr-bizz July 26, 2024 16:23
dr-bizz
dr-bizz requested changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Great work, there were some changes I thought needed to happen before we role this PR liv, which I've commented below. Feel free to disagree with my thoughts.

src/components/Tool/FixEmailAddresses/FixEmailAddressPerson.tsx Show resolved Hide resolved
src/components/Tool/FixEmailAddresses/FixEmailAddresses.tsx Outdated Show resolved Hide resolved
src/components/Tool/FixEmailAddresses/FixEmailAddresses.test.tsx
cache: InMemoryCache;
}

const addEmailAddress = async ({
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn.t have a component in a test like this. Ifn we want to test addEmailAddress we should import it rather than recreate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a component, it is a helper function to reduce code duplication.

src/components/Tool/FixEmailAddresses/FixEmailAddresses.test.tsx
userEvent.click(addButton);
};

it('should add an email address to the first person', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make the test like how the user would interact with the component. Such as types in new email and clicks "add", this should fire a GraphQL Mutation request we can test.

Copy link
Contributor

@wrandall22 wrandall22 Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That is what we are doing here, though I never found good ways to test what data got sent in the mutation, so I went with what got sent to the cache instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have updated the code to add some more tests and clean up the PR.

@wrandall22 wrandall22 requested a review from dr-bizz August 1, 2024 20:26
@dr-bizz dr-bizz force-pushed the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch from 9e12d3f to 5d87678 Compare August 2, 2024 15:30
@dr-bizz dr-bizz enabled auto-merge August 2, 2024 15:35
@dr-bizz dr-bizz merged commit 63b1170 into main Aug 2, 2024
17 checks passed
@dr-bizz dr-bizz deleted the MPDX-7946-Use-Contact-Mutation-When-Adding-Email branch August 2, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wrandall22 wrandall22 wrandall22 left review comments

@dr-bizz dr-bizz dr-bizz approved these changes

Assignees
No one assigned
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cjpastika @dr-bizz @wrandall22