-
Notifications
You must be signed in to change notification settings - Fork 1
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
Preferences - Connect Services Page #820
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pausing for now, but I'm impressed with the amount of time you must have spent on this!
pages/accountLists/[accountListId]/settings/integrations/index.page.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/integrations/index.page.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/integrations/integrationsContext.tsx
Outdated
Show resolved
Hide resolved
.../api/Schema/Settings/Preferences/Intergrations/Google/createGoogleIntegration/datahandler.ts
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Google/Modals/EditGoogleAccountModal.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Google/Modals/EditGoogleIntegrationForm.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Mailchimp/MailchimpAccordian.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Mailchimp/MailchimpAccordian.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Mailchimp/MailchimpAccordian.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/OrganizationAccordian.tsx
Outdated
Show resolved
Hide resolved
event.preventDefault(); | ||
try { | ||
if (!importFile) throw new Error('Please select a file to upload.'); | ||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this need fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this TODO
here as I wanted to test the file upload with a 100 MB file on the server.
src/components/Settings/integrations/Organization/OrganizationService.ts
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/OrganizationService.ts
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/Modals/OrganizationAddAccountModal.tsx
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/Organizations.graphql
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Prayerletters/Modals/DeletePrayerlettersModal.tsx
Outdated
Show resolved
Hide resolved
describe('Connected', () => { | ||
let prayerlettersAccount = { ...standardPrayerlettersAccount }; | ||
|
||
beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is copying the mock necessary? GqlMockedProvider
shouldn't modify the mock objects passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as changes we make to the PrayerLetter mock data persist onto the next test. Causing the later tests to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I missed that. What if we used getPrayerlettersAccount: [{ ...standardPrayerlettersAccount, validToken: false }]
in the first test and getPrayerlettersAccount: [standardPrayerlettersAccount]
in the others so that we don't have to make a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just that, out of the 3 tests, 2 are testing the delete functionality, so we need to reset the data to prevent test errors.
src/components/Settings/integrations/Prayerletters/PrayerlettersAccordian.tsx
Outdated
Show resolved
Hide resolved
...omponents/Settings/integrations/Organization/Modals/OrganizationImportDataSyncModal.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/Modals/OrganizationImportDataSyncModal.tsx
Outdated
Show resolved
Hide resolved
...pi/Schema/Settings/Preferences/Intergrations/Google/googleAccountIntegrations/datahandler.ts
Outdated
Show resolved
Hide resolved
pages/api/Schema/Settings/Preferences/Intergrations/Google/googleAccounts/datahandler.ts
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Chalkline/ChalklineAccordion.test.tsx
Outdated
Show resolved
Hide resolved
expect(image).toBeInTheDocument(); | ||
}); | ||
|
||
it('should send contacts to Chalkline', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should mock window.open
and waitFor
it to be called. I think the test is ending before the window.open
line gets hit in the component because it's in a setTimeout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having some trouble mocking this. Will spend some more time on this before moving on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to move on as I couldn't get window.open
to mock correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because the default timeout of waitFor
is 1000ms so it times out before window.open
is called? I got it to work with:
await waitFor(
() =>
expect(openMock).toHaveBeenCalledWith(
'https://chalkline.org/order_mpdx/',
'_blank',
),
{ timeout: 3000 },
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was it! I was so close, but so far lol
src/components/Settings/integrations/Google/GoogleAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/OrganizationAccordion.test.tsx
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/OrganizationAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Prayerletters/PrayerlettersAccordion.test.tsx
Outdated
Show resolved
Hide resolved
Ensured TS safety on showArticle function
Helpscout - ensuring the beacon shows and renders above modals
@canac I'm hoping this is the last review of this you'll have to do. you'll see that I've fixed all of the issues you raised apart from one (mocking window.open on ChalklineAccordion.test.tsx) I spent some time trying to mock this, but had to move on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an idea of what might fix the window.open
mock, which you are welcome to try or not, but I'm going to approve since that's such a minor thing. Incredible work with this! 🎆
Similar to #817, We've done a lot of work on Preferences. To prevent a 300-plus page PR I'm pushing certain changes into
main
which doesn't affect the user.This one is the largest PR I'll do. I have tried to split up the code into commits. Don't feel like you need to review this one in one sitting. I know it's going to take a while.
This PR is the connect services page in Preferences. I only made one page with this PR (Integrations page), but this page includes a bunch of GraphQL proxies and functionality.
I've built out the page, but I haven't given the user any way to navigate to this page.
Integrations page
The Key
Organization
Google
Mailchimp
Prayer Letters
Chalkline