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

MPDX-7893 - Allowing users to add offline organizations to their accounts #936

Merged
merged 2 commits into from
May 1, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Apr 30, 2024

Description

Fixing error introduced in #891

Currently, on Connect Services, users aren't able to add offline organizations due to the disableNewUsers returning as NULL from GraphQL.

The issue was caused by a FormIk client error. I've enabled disableNewUsers to be nullable and added a test to ensure NULL is treated the same way as TRUE.

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

@dr-bizz dr-bizz requested review from caleballdrin and canac April 30, 2024 21:25
Copy link
Contributor

Bundle sizes [mpdx-react]

Compared against 427565b

No significant changes found

Copy link
Contributor

@caleballdrin caleballdrin 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. thanks for fixing this and adding the test

@dr-bizz dr-bizz added On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview labels Apr 30, 2024
Copy link
Contributor

Preview branch generated at https://fix-add-organization-modal.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all these bugs! I left an optional suggestion that I must have missed in previous code review.

@@ -153,7 +153,7 @@ export const OrganizationAddAccountModal: React.FC<
name: yup.string().required(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to move this schema outside of the component? I don't think it uses anything from the component closure. Having it inside the component means it will be unnecessarily recreated on every render.

@dr-bizz dr-bizz merged commit 8753f06 into main May 1, 2024
18 checks passed
@dr-bizz dr-bizz deleted the fix-add-organization-modal branch May 1, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants